Skip to content

[fix][test] Replace PulsarService Mockito spy solution for overriding getters in tests#19323

Merged
lhotari merged 1 commit into
apache:masterfrom
lhotari:lh-replace-PulsarService-Mockito-spy
Jan 25, 2023
Merged

[fix][test] Replace PulsarService Mockito spy solution for overriding getters in tests#19323
lhotari merged 1 commit into
apache:masterfrom
lhotari:lh-replace-PulsarService-Mockito-spy

Conversation

@lhotari

@lhotari lhotari commented Jan 25, 2023

Copy link
Copy Markdown
Member

Fixes #13620
Fixes #16444
Fixes #16427

Motivation

Mockito is not thread safe and there's a problem with mocking PulsarService which is described in #16821 description:

I found the cause of the problem: while executing doReturn(pulsarResources).when(pulsar).getPulsarResources(), Meta Store Thread also accesses variable PulsarService.getPulsarResources() asynchronously in logic: notification by zk-watcher(Concurrent access will be problematic if the object is being mock bound).

Unfortunately, the solution made in PR #16821 didn't fix the problem. A better solution is needed to fix the problem.

Modifications

Replace the Mockito spy based PulsarService used in a few unit tests with a subclass of PulsarService which reduces the use of Mockito. Mockito spies can be continued to be used, but the getters are replaced with overridden methods in a new class TestPulsarService which extends PulsarService.
The test code is also simplified by moving the required collaborator objects and resources to the TestPulsarService.Factory class which contains a close() method to close and release the allocated resources.

Documentation

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

@lhotari lhotari added this to the 2.12.0 milestone Jan 25, 2023
@lhotari lhotari self-assigned this Jan 25, 2023
@lhotari lhotari changed the title [fix][test] Replace PulsarService Mockito spy that has a thread safety issues [fix][test] Replace PulsarService Mockito spy that has thread safety issues Jan 25, 2023
@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Jan 25, 2023
@lhotari

lhotari commented Jan 25, 2023

Copy link
Copy Markdown
Member Author

/pulsarbot rerun-failure-checks

@lhotari lhotari force-pushed the lh-replace-PulsarService-Mockito-spy branch from dafd673 to 7e86291 Compare January 25, 2023 13:02
@lhotari lhotari requested review from coderzc and shibd January 25, 2023 13:16
@lhotari lhotari changed the title [fix][test] Replace PulsarService Mockito spy that has thread safety issues [fix][test] Replace PulsarService Mockito spy solution for overriding getters Jan 25, 2023
@lhotari lhotari changed the title [fix][test] Replace PulsarService Mockito spy solution for overriding getters [fix][test] Replace PulsarService Mockito spy solution for overriding getters in tests Jan 25, 2023

@nicoloboschi nicoloboschi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lhotari lhotari requested a review from merlimat January 25, 2023 13:48

@eolivelli eolivelli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very good.
And I agree that we were (still are?) using too much Mockito.spy() with all the problems that we know about it

@lhotari lhotari merged commit b3432f4 into apache:master Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

3 participants