RE: Last sprint to finish line: Replace EasyMock/Powermock with Mockito

[Christo Lolov]

Hello!

Below you will find the latest state of the Mockito migration.

81% or 39/48 of streams-related tests have been migrated.
The last 5 pull requests which are in need of reviews are:
* https://github.com/apache/kafka/pull/12449
* https://github.com/apache/kafka/pull/12524
* https://github.com/apache/kafka/pull/12777
* https://github.com/apache/kafka/pull/12607
* https://github.com/apache/kafka/pull/12739 (waiting on author)
Once they are merged we can move Streams to JUnit 5!

60% or 9/15 of connect-related tests have been migrated.
The 2 pull requests which are in need of reviews are:
* https://github.com/apache/kafka/pull/12728
* https://github.com/apache/kafka/pull/12781

Best,
Christo

RE: Last sprint to finish line: Replace EasyMock/Powermock with Mockito

[Christo Lolov]

Hello!

This email summarises the current state of Kafka's Mockito migration.

The JIRA tickets used to track the progress are 
https://issues.apache.org/jira/browse/KAFKA-14132 
 and 
https://issues.apache.org/jira/browse/KAFKA-14133 
.

—

Breakdown of https://issues.apache.org/jira/browse/KAFKA-14133 

19/46 ~ 41% are merged
27/46 ~ 59% are in review

A list of pull requests awaiting a review from a committer:
https://github.com/apache/kafka/pull/12739 

https://github.com/apache/kafka/pull/12505 

https://github.com/apache/kafka/pull/12524 

https://github.com/apache/kafka/pull/12818 


—

Breakdown of https://issues.apache.org/jira/browse/KAFKA-14132 

7/17 ~ 41% are merged
6/17 ~ 35% are in review
4/17 ~ 24% are in progress

A list of pull requests awaiting a review from a committer:
https://github.com/apache/kafka/pull/12728 

https://github.com/apache/kafka/pull/12821 


—

A list of pull requests which have been merged since the last update:
https://github.com/apache/kafka/pull/12527 

https://github.com/apache/kafka/pull/12725 

https://github.com/apache/kafka/pull/12823 


A big thank you to Shekhar Prasad Rajak (who recently joined our effort), 
Matthew de Detrich, Dalibor Plavcic, and everyone who has provided reviews over 
the last month!

Best,
Christo

Re: Last sprint to finish line: Replace EasyMock/Powermock with Mockito

[Christo Lolov]

Hello,

I have not been able to make a lot of progress on the Mockito migration myself, 
but Yash and Divij opened and merged a PR each.

The following PRs have made it into trunk:

https://github.com/apache/kafka/pull/12615
https://github.com/apache/kafka/pull/12677
https://github.com/apache/kafka/pull/12492

Thank you to Yash and Divij for authoring them and to Chris and Bruno for the 
reviews!

The following PRs are in progress:

In need of reviewer attention - RA
In need of author attention - AA

https://github.com/apache/kafka/pull/12409 (RA)
https://github.com/apache/kafka/pull/12418 (AA)
https://github.com/apache/kafka/pull/12465 (RA)
https://github.com/apache/kafka/pull/12505 (AA)
https://github.com/apache/kafka/pull/12524 (RA)
https://github.com/apache/kafka/pull/12527 (RA)
https://github.com/apache/kafka/pull/12607 (AA)

A summary of the Mockito migration:

https://issues.apache.org/jira/browse/KAFKA-14133 
 - a little under 1/3 of all 
classes using EasyMock have been moved and merged; a bit over 1/3 are in PRs 
and around 1/3 are remaining.

https://issues.apache.org/jira/browse/KAFKA-14132 
 - a little under 1/3 of all 
classes using PowerMock have been moved and merged; a bit over 2/3 are 
remaining.

Best,
Christo

Re: Last sprint to finish line: Replace EasyMock/Powermock with Mockito

[Guozhang Wang]

Thanks Christo for the updates. As we are adding new unit tests we are also
keen on using the new Mockito packages and so far I'd like to say it's much
easier to use :) would chime in to help on reviewing some of the PRs as
well.


Guozhang

On Tue, Sep 6, 2022 at 11:02 PM Christo Lolov
 wrote:

> Hello!
>
> This is the (roughly) bi-weekly update on the Mockito migration.
>
> Firstly, the following PRs have been merged since the last email so thank
> you to the writers (Yash and Divij) and reviewers (Dalibor, Mickael, Yash,
> Bruno and Chris):
>
> https://github.com/apache/kafka/pull/12459 <
> https://github.com/apache/kafka/pull/12459>
> https://github.com/apache/kafka/pull/12473 <
> https://github.com/apache/kafka/pull/12473>
> https://github.com/apache/kafka/pull/12509 <
> https://github.com/apache/kafka/pull/12509>
>
> Secondly, this is the latest list of PRs that are in need of a review to
> get them over the line:
>
> https://github.com/apache/kafka/pull/12409 <
> https://github.com/apache/kafka/pull/12409>
> https://github.com/apache/kafka/pull/12418 <
> https://github.com/apache/kafka/pull/12418> (I need to respond to the
> comments on this one, so the first action is on me)
> https://github.com/apache/kafka/pull/12465 <
> https://github.com/apache/kafka/pull/12465>
> https://github.com/apache/kafka/pull/12492 <
> https://github.com/apache/kafka/pull/12492>
> https://github.com/apache/kafka/pull/12505 <
> https://github.com/apache/kafka/pull/12505> (I need to respond to
> Dalibor’s comment on this one, but the overall PR could use some more eyes)
> https://github.com/apache/kafka/pull/12524 <
> https://github.com/apache/kafka/pull/12524>
> https://github.com/apache/kafka/pull/12527 <
> https://github.com/apache/kafka/pull/12527>
>
> Lastly, I am keeping https://issues.apache.org/jira/browse/KAFKA-14133 <
> https://issues.apache.org/jira/browse/KAFKA-14133> and
> https://issues.apache.org/jira/browse/KAFKA-14132 <
> https://issues.apache.org/jira/browse/KAFKA-14132> up to date, so if
> anyone has spare bandwidth and would like to assign themselves some of the
> unassigned tests their contributions would be welcome :)
>
> Best,
> Christo



-- 
-- Guozhang

RE: Last sprint to finish line: Replace EasyMock/Powermock with Mockito

[Christo Lolov]

Hello!

This is the (roughly) bi-weekly update on the Mockito migration.

Firstly, the following PRs have been merged since the last email so thank you 
to the writers (Yash and Divij) and reviewers (Dalibor, Mickael, Yash, Bruno 
and Chris):

https://github.com/apache/kafka/pull/12459 

https://github.com/apache/kafka/pull/12473 

https://github.com/apache/kafka/pull/12509 


Secondly, this is the latest list of PRs that are in need of a review to get 
them over the line:

https://github.com/apache/kafka/pull/12409 

https://github.com/apache/kafka/pull/12418 
 (I need to respond to the comments 
on this one, so the first action is on me)
https://github.com/apache/kafka/pull/12465 

https://github.com/apache/kafka/pull/12492 

https://github.com/apache/kafka/pull/12505 
 (I need to respond to Dalibor’s 
comment on this one, but the overall PR could use some more eyes)
https://github.com/apache/kafka/pull/12524 

https://github.com/apache/kafka/pull/12527 


Lastly, I am keeping https://issues.apache.org/jira/browse/KAFKA-14133 
 and 
https://issues.apache.org/jira/browse/KAFKA-14132 
 up to date, so if anyone 
has spare bandwidth and would like to assign themselves some of the unassigned 
tests their contributions would be welcome :)

Best,
Christo

Re: Last sprint to finish line: Replace EasyMock/Powermock with Mockito

[Christo]

 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

[Guozhang Wang]

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 
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 
>
> 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
>   ) 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 
> wrote:
>
> > Hi folks
> >
> > We have been trying to replace EasyMock/Powermock with Mockito
> >  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 

Re: Last sprint to finish line: Replace EasyMock/Powermock with Mockito

[Christo]

 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 
 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
  ) 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  wrote:

> Hi folks
>
> We have been trying to replace EasyMock/Powermock with Mockito
>  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

[Matthew Benedict de Detrich]

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.dedetr...@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

[Divij Vaidya]

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
   ) 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  wrote:

> Hi folks
>
> We have been trying to replace EasyMock/Powermock with Mockito
>  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
>
>

Last sprint to finish line: Replace EasyMock/Powermock with Mockito

[Divij Vaidya]

Hi folks

We have been trying to replace EasyMock/Powermock with Mockito
 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