Thanks everyone for the great effort! Just one quick question regarding the Streams tests: I looked over https://github.com/apache/kafka/pull/12492/files, https://github.com/apache/kafka/pull/12505/files and https://github.com/apache/kafka/pull/12465/files, but I did not find the one covering `TaskManagerTest` that has a mixed usage of Mockito and EasyMock. Do you have a PR covering that class and if yes, could you point me to that one?
Guozhang On Mon, Aug 15, 2022 at 11:48 AM Christo <christo_lo...@yahoo.com.invalid> wrote: > Hello! > Following Divij's example I wanted to give an update on the progress being > made. > With a combined effort from Yash Mayya, Matthew de Detrich and myself we > are 3/4 through providing pull requests for moving the remaining EasyMock > tests to Mockito (https://issues.apache.org/jira/browse/KAFKA-14133). > Across those and other pull requests we have received insightful feedback > from Bruno Cadonna, Chris Egerton, Dalibor Plavcic and Ismael Juma on how > things can be improved. Thank you very much! > Current pull requests we are trying to get to a resolution from oldest to > newest are: > * https://github.com/apache/kafka/pull/12409* > https://github.com/apache/kafka/pull/12418* > https://github.com/apache/kafka/pull/12459* > https://github.com/apache/kafka/pull/12465 > * https://github.com/apache/kafka/pull/12473 > * https://github.com/apache/kafka/pull/12492* > https://github.com/apache/kafka/pull/12505* > https://github.com/apache/kafka/pull/12509 > Best,Christo <https://github.com/apache/kafka/pull/12509Best,Christo> > > On Thursday, 4 August 2022, 18:27:17 BST, Divij Vaidya < > divijvaidy...@gmail.com> wrote: > > Hi everyone > > To provide you with quick updates on the progress. > > Open PRs (pending review): > > 1. Streams - https://github.com/apache/kafka/pull/12449 > 2. Streams - https://github.com/apache/kafka/pull/12465 > 3. Streams - https://github.com/apache/kafka/pull/12459 > 4. Connect - https://github.com/apache/kafka/pull/12484 > 5. Connect - https://github.com/apache/kafka/pull/12473 6. Connect - > https://github.com/apache/kafka/pull/12409 > 7. Connect - https://github.com/apache/kafka/pull/12472 > > Open tasks (pending an owners): > > 1. https://issues.apache.org/jira/browse/KAFKA-14132 (need owners for > separate individual tests) > 2. https://issues.apache.org/jira/browse/KAFKA-14133 > > > General guidance to reduce code review churn when working on these test > conversions: > > 1. Please use @RunWith(MockitoJUnitRunner.StrictStubs.class) since it > provides many benefits. > 2. Please do not perform JUnit 5 migration in the same PR as Mockito > conversion to keep the changes few and easy to review. We will follow up > with a blanket JUnit5 conversion (similar to this > <https://github.com/apache/kafka/pull/12285>) when Mockito migration is > complete. > 3. Please use @Mock annotation to mock (Chris Egerton has added this > comment on various PRs, hence calling it out) > 4. Note that @RunWith(MockitoJUnitRunner.StrictStubs.class) verifies the > invocation of declared stubs automatically. If the stubs are not invoked, > the test throws a UnnecessaryStubbingException. Note that this doesn't > seem > to work for `mockStatic` and I would suggest to explicitly verify stub > invocations over there. > 5. As a reference, you can use the merged PR from Chris Egerton here: > https://github.com/apache/kafka/pull/12409 6. Add a verification step > in the description that the test has > successfully run with the command `./gradlew connect:runtime:unitTest` > (or > equivalent for the module you are changing the test for). Additionally, > you > can add the code coverage report using `./gradlew streams:reportCoverage > -PenableTestCoverage=true -Dorg.gradle.parallel=false` to verify that no > test assertion has been accidentally removed during the change. > > > *Chris*, would you like to add anything else to the general guidance above > which would help reduce the code review churn? > > -- > Divij Vaidya > > > > On Mon, Aug 1, 2022 at 6:49 PM Divij Vaidya <divijvaidy...@gmail.com> > wrote: > > > Hi folks > > > > We have been trying to replace EasyMock/Powermock with Mockito > > <https://issues.apache.org/jira/browse/KAFKA-7438> for quite a while. > > This adds complications for migrating to JDK 17 & Junit5. Significant > > contributions have been made by various folks towards this goal and the > > finish line is almost in sight. > > > > Let's join forces this week and get the task done! > > > > I and Christo(cc'ed) will be spending time converting the straggler tests > > during this week. > > > > At this stage, we are missing a shepherd to help us wrap up this task. > *Could > > we please solicit some code review bandwidth from a committer for this > week > > to help us reach the finish line?* > > > > Current pending PR requests: > > 1. KAFKA-13036: Replace EasyMock and PowerMock with Mockito for > RocksDBMetricsRecorderTest by divijvaidya · Pull Request #12459 · > apache/kafka > > > 2. KAFKA-12950: Replace EasyMock and PowerMock with Mockito for > KafkaStreamsTest by divijvaidya · Pull Request #12465 · apache/kafka > > 3. KAFKA-13414: Replace PowerMock/EasyMock with Mockito in > connect.storage.KafkaOffsetBackingStoreTest by clolov · Pull Request #12418 > · apache/kafka > > > > Regards, > > Divij Vaidya > > > > > -- -- Guozhang
