You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Divij Vaidya <di...@gmail.com> on 2022/08/01 16:49:35 UTC
Last sprint to finish line: Replace EasyMock/Powermock with Mockito
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. https://github.com/apache/kafka/pull/12459
2. https://github.com/apache/kafka/pull/12465
3. https://github.com/apache/kafka/pull/12418
Regards,
Divij Vaidya
Re: Last sprint to finish line: Replace EasyMock/Powermock with Mockito
Posted by Matthew Benedict de Detrich <ma...@aiven.io.INVALID>.
I will assign myself to 14132 and 14133, thanks for the detailed notes on gotchas for the migration.
Regards
--
Matthew de Detrich
Aiven Deutschland GmbH
Immanuelkirchstraße 26, 10405 Berlin
Amtsgericht Charlottenburg, HRB 209739 B
Geschäftsführer: Oskari Saarenmaa & Hannu Valtonen
m: +491603708037
w: aiven.io e: matthew.dedetrich@aiven.io
On 4. Aug 2022, 19:27 +0200, dev@kafka.apache.org, wrote:
>
> https://github.com/apache/kafka/pull/12465
Re: Last sprint to finish line: Replace EasyMock/Powermock with Mockito
Posted by Christo <ch...@yahoo.com.INVALID>.
Hello!
Apologies for the delay in my reply. I have started working on migrating the TaskManagerTest to Mockito, but have not raised a pull request yet. It is on the list in https://issues.apache.org/jira/browse/KAFKA-14133 and I will add you as a reviewer the moment I publish it.
Best,Christo
Re: Last sprint to finish line: Replace EasyMock/Powermock with Mockito
Posted by Guozhang Wang <wa...@gmail.com>.
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 <ch...@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 <
> divijvaidya13@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 <di...@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
Re: Last sprint to finish line: Replace EasyMock/Powermock with Mockito
Posted by Christo <ch...@yahoo.com.INVALID>.
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
On Thursday, 4 August 2022, 18:27:17 BST, Divij Vaidya <di...@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 <di...@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
>
>
Re: Last sprint to finish line: Replace EasyMock/Powermock with Mockito
Posted by Divij Vaidya <di...@gmail.com>.
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 <di...@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. https://github.com/apache/kafka/pull/12459
> 2. https://github.com/apache/kafka/pull/12465
> 3. https://github.com/apache/kafka/pull/12418
>
> Regards,
> Divij Vaidya
>
>