kafka icon indicating copy to clipboard operation
kafka copied to clipboard

KAFKA-12950: Replace EasyMock and PowerMock with Mockito for KafkaStreamsTest

Open divijvaidya opened this issue 3 years ago • 3 comments

This PR build on top of https://github.com/apache/kafka/pull/11017. I have added the previous author's comment in this PR for attribution. I have also addressed the pending comments from @chia7712 in this PR.

Notes to help the reviewer:

  1. Mockito has mockStatic method which is equivalent to PowerMock's method.
  2. When we run the tests using @RunWith(MockitoJUnitRunner.StrictStubs.class) Mockito performs a verify() for all stubs that are mentioned, hence, there is no need to explicitly verify the stubs (unless you want to verify the number of times etc.). Note that this does not work for static mocks.

Testing

Validated that test is run successfully as part of ./gradlew streams:unitTest

Code coverage

Before

Screenshot 2022-08-02 at 19 02 42

After

Screenshot 2022-08-02 at 19 02 49

divijvaidya avatar Aug 01 '22 16:08 divijvaidya

@ijuma Please review when you get a chance. This would help us take a step forward in getting rid of easymock/powermock dependency.

divijvaidya avatar Aug 02 '22 16:08 divijvaidya

@cadonna requesting your review on this PR related to streams.

divijvaidya avatar Aug 23 '22 16:08 divijvaidya

@cadonna please review when you get a chance. You can also refer to previous PR https://github.com/apache/kafka/pull/12465 and note that the comments provided there have been addressed.

divijvaidya avatar Sep 05 '22 14:09 divijvaidya

@cadonna this is ready for your review whenever you get a chance.

divijvaidya avatar Sep 27 '22 16:09 divijvaidya

Rebased from latest commits in trunk

divijvaidya avatar Oct 26 '22 14:10 divijvaidya

Failures unrelated

bbejeck avatar Oct 27 '22 18:10 bbejeck

Merged #12465 into trunk

bbejeck avatar Oct 27 '22 18:10 bbejeck

Thanks for the contribution @divijvaidya !

bbejeck avatar Oct 27 '22 18:10 bbejeck

@divijvaidya So can we close #11017 after this is merged?

dengziming avatar Oct 31 '22 02:10 dengziming

@dengziming yes please. It can be closed.

divijvaidya avatar Oct 31 '22 11:10 divijvaidya