Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[improve][test] Support decorating topic, subscription, dispatcher, ManagedLedger and ManagedCursors instances in tests #23892

Merged
merged 2 commits into from
Jan 25, 2025

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Jan 24, 2025

Motivation

In some Pulsar tests, there's a need to inject special behaviors such as delays, race conditions or failures.
The current solutions require using reflection and replacing instances, which results in hard to maintain test code.

Modifications

  • Add a solution for Pulsar broker tests that allows to decorate persistent topics, subscriptions, dispatchers
    managed ledger factory, managed ledger and managed cursor instances.
  • Refactor production code so that there are hooks for intercepting the instances in subclasses
  • Add a helper methods for creating a Mockito Spy decorator
    • for a dispatcher instance
    • for a ManagedCursorImpl instance

example usage for spying on a dispatcher and cursor instance:

    @Override
    protected void doInitConf() throws Exception {
        super.doInitConf();
        BrokerTestInterceptor.INSTANCE.configure(conf);
    }

    @AfterMethod(alwaysRun = true)
    protected void resetInterceptors() throws Exception {
        BrokerTestInterceptor.INSTANCE.reset();
    }

    @Test
    public void testSomething() throws Exception {
        BrokerTestInterceptor.INSTANCE.applyDispatcherSpyDecorator(PersistentDispatcherMultipleConsumers.class,
                spy -> {
                    doAnswer(invocation -> {
                        List<Entry> entries = invocation.getArgument(0);
                        log.info("intercepted readEntriesComplete with {} entries", entries.size());
                        return invocation.callRealMethod();
                    }).when(spy).readEntriesComplete(any(), any());
                }
        );

        BrokerTestInterceptor.INSTANCE.applyCursorSpyDecorator(spy -> {
            doAnswer(invocation -> {
                log.info("intercepted asyncReadEntriesWithSkipOrWait");
                return invocation.callRealMethod();
            }).when(spy).asyncReadEntriesWithSkipOrWait(anyInt(), anyLong(), any(), any(), any(), any());
            doAnswer(invocation -> {
                log.info("intercepted asyncReplayEntries");
                return invocation.callRealMethod();
            }).when(spy).asyncReplayEntries(any(), any(), any(), anyBoolean());
        });
   ...

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@lhotari lhotari added this to the 4.1.0 milestone Jan 24, 2025
@lhotari lhotari self-assigned this Jan 24, 2025
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 24, 2025
@lhotari lhotari force-pushed the lh-add-broker-test-interceptor branch from 4a80067 to c448596 Compare January 24, 2025 19:39
@lhotari lhotari changed the title [improve][test] Add solution for intercepting topic, subscription and dispatcher instances in tests [improve][test] Support decorating topic, subscription, dispatcher, ManagedLedger and ManagedCursors instances in tests Jan 24, 2025
@codecov-commenter
Copy link

codecov-commenter commented Jan 24, 2025

Codecov Report

Attention: Patch coverage is 86.66667% with 6 lines in your changes missing coverage. Please review.

Project coverage is 74.22%. Comparing base (bbc6224) to head (c448596).
Report is 868 commits behind head on master.

Files with missing lines Patch % Lines
...ker/service/persistent/PersistentSubscription.java 84.84% 3 Missing and 2 partials ⚠️
...org/apache/pulsar/broker/service/TopicFactory.java 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23892      +/-   ##
============================================
+ Coverage     73.57%   74.22%   +0.65%     
+ Complexity    32624    32244     -380     
============================================
  Files          1877     1854      -23     
  Lines        139502   143640    +4138     
  Branches      15299    16318    +1019     
============================================
+ Hits         102638   106619    +3981     
+ Misses        28908    28648     -260     
- Partials       7956     8373     +417     
Flag Coverage Δ
inttests 26.64% <51.11%> (+2.06%) ⬆️
systests 23.17% <60.00%> (-1.15%) ⬇️
unittests 73.75% <86.66%> (+0.90%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...kkeeper/mledger/impl/ManagedLedgerFactoryImpl.java 59.04% <100.00%> (-22.34%) ⬇️
...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java 82.11% <100.00%> (+1.45%) ⬆️
...ache/pulsar/broker/ManagedLedgerClientFactory.java 86.20% <100.00%> (+2.06%) ⬆️
...sar/broker/service/persistent/PersistentTopic.java 79.69% <ø> (+1.23%) ⬆️
...org/apache/pulsar/broker/service/TopicFactory.java 0.00% <0.00%> (ø)
...ker/service/persistent/PersistentSubscription.java 76.67% <84.84%> (-0.02%) ⬇️

... and 1021 files with indirect coverage changes

@lhotari lhotari merged commit 2a9d4ac into apache:master Jan 25, 2025
56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/test doc-not-needed Your PR changes do not impact docs ready-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants