[improve][test] Refactor TestPulsarService to PulsarTestContext and add support for starting#19337
Conversation
…dd support for starting - Some Pulsar unit tests use a PulsarService that isn't started - Some Pulsar unit tests start the PulsarService and use less mocking - Support both modes in PulsarTestContext - Refactor OwnerShipForCurrentServerTestBase to use this new mode
michaeljmarshall
left a comment
There was a problem hiding this comment.
Overall, LGTM. I left some comments/questions.
| PulsarTestContext.startableBuilder() | ||
| .config(conf); | ||
| if (i > 0) { | ||
| testContextBuilder.reuseMockBookkeeperAndMetadataStores(pulsarTestContexts.get(0)); |
There was a problem hiding this comment.
Why are we getting the first one here?
There was a problem hiding this comment.
PulsarTestContext holds the mock bookkeeper and metadata store (mockzookeeper) references. The "reuseMockBookkeeperAndMetadataStores" can be used to share the mock bookkeeper and zookeeper from another context.
| @ToString | ||
| @Getter | ||
| @Builder(builderClassName = "Builder") | ||
| public class PulsarTestContext implements AutoCloseable { |
There was a problem hiding this comment.
A Javadoc explaining how to use this class would be valuable. Also, are you able to point out what of this class is new? I see that it is pretty similar to the TestPulsarService, and I know your PR description indicates that it is an improvement on TestPulsarService.
There was a problem hiding this comment.
I agree. I can improve the documentation in a follow-up PR that I have in progress.
| /** | ||
| * Subclass of PulsarService that is used for some tests. | ||
| * This was written as a replacement for the previous Mockito Spy over PulsarService solution which caused | ||
| * a flaky test issue https://github.com/apache/pulsar/issues/13620. | ||
| */ |
There was a problem hiding this comment.
I see this was copied from TestPulsarService. Is there any new context we need in order to document how to use this class?
There was a problem hiding this comment.
I'll update the docs in a follow-up PR.
michaeljmarshall
left a comment
There was a problem hiding this comment.
LGTM. Thanks for addressing my questions.
Motivation
This is a follow up on previous PRs (#19326, #19323) to refactor Pulsar unit tests to reduce mocking which causes flakiness since Mockito isn't thread safe.
There are 2 types of Pulsar unit tests that use a PulsarService:
With the changes in this PR, the test framework will be suitable for use across a broader set of Pulsar unit tests and we can get rid of more of the problematic usages of Mockito.
Modifications
This PR refactors the previous TestPulsarService to PulsarTestContext which is more maintainable.
It supports both modes of PulsarServices, startable and non-startable.
OwnerShipForCurrentServerTestBase is used as piloting the new mode. More tests will be converted after this PR has been merged.
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: lhotari#138