Skip to content

KAFKA-14133: Migrate StandbyTaskCreator mock in TaskManagerTest to Mockito#13711

Merged
divijvaidya merged 2 commits intoapache:trunkfrom
clolov:KAFKA-14133-PART8
Jun 15, 2023
Merged

KAFKA-14133: Migrate StandbyTaskCreator mock in TaskManagerTest to Mockito#13711
divijvaidya merged 2 commits intoapache:trunkfrom
clolov:KAFKA-14133-PART8

Conversation

@clolov
Copy link
Copy Markdown
Contributor

@clolov clolov commented May 12, 2023 •
edited
Loading

This pull requests migrates the StandbyTaskCreator mock in TaskManagerTest from EasyMock to Mockito.
The change is restricted to a single mock to minimize the scope and make it easier for review.

The reasoning as to why we would like to migrate a single mock rather than all mocks in the file at the same time has been discussed in #12607 (comment)

It takes the same approach as in:
#13529
#13621
#13681

This needs to be rebased on top of #13681 before merging

@clolov clolov mentioned this pull request May 12, 2023
Merged
@clolov clolov added streams tests Test fixes (including flaky tests) labels May 18, 2023
@cadonna
Copy link
Copy Markdown
Member

cadonna commented Jun 5, 2023

@clolov Could you please do the mentioned rebase?

@cadonna
Copy link
Copy Markdown
Member

cadonna commented Jun 5, 2023 •
edited
Loading

Could you also please add some more info to the PR description instead of only referencing other PRs? One or two sentences followed by the references is totally fine.

cadonna
cadonna reviewed Jun 5, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@cadonna cadonna left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @clolov !

Here my feedback.

Comment thread streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java Outdated
Comment on lines 359 to 360
Copy link
Copy Markdown
Member

@cadonna cadonna Jun 5, 2023

Choose a reason for hiding this comment

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

nit: Why did you introduce variable standbyTasks? The variable is not used anywhere else.

Comment thread streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java Outdated
Copy link
Copy Markdown
Member

@cadonna cadonna Jun 5, 2023

Choose a reason for hiding this comment

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

I do not think you need this verification since also with EasyMock the call is not verified (i.e., verify() on the mock is never called).

Copy link
Copy Markdown
Contributor Author

@clolov clolov Jun 5, 2023

Choose a reason for hiding this comment

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

To be honest, here I followed what was done in PR #13681
For example, the original in the mentioned PR was.

...
expect(activeTaskCreator.createTasks(consumer, Collections.emptyMap())).andReturn(emptySet());
...

(https://github.com/apache/kafka/pull/13681/files#diff-48bc1476f0437fd711093c7c80ce73eda10be0511705799b4248545c203d521dL294)

and with Mockito we changed it to

...
Mockito.verify(activeTaskCreator).createTasks(consumer, Collections.emptyMap());
...

(https://github.com/apache/kafka/pull/13681/files#diff-48bc1476f0437fd711093c7c80ce73eda10be0511705799b4248545c203d521dR306)

I followed the same logic for what I thought was a stronger verification, but I am happy to remove it if you think it is unnecessary. I realise the reasoning for doing the above in that PR might have made more sense because there was more than one argument.

Let me know!

Copy link
Copy Markdown
Contributor Author

@clolov clolov Jun 5, 2023

Choose a reason for hiding this comment

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

Oh, I see what you mean. You mean that in the EasyMock version there are no calls to

verify(standbyTaskCreator)

in the these specific tests.
Yeah, sure, I will change it!

Comment thread streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java Outdated
Copy link
Copy Markdown
Member

@cadonna cadonna Jun 5, 2023

Choose a reason for hiding this comment

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

Also here, I do not think you need to verify.

Comment thread streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java Outdated
Copy link
Copy Markdown
Member

@cadonna cadonna Jun 5, 2023

Choose a reason for hiding this comment

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

Also here no verification needed.

Comment thread streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java Outdated
Copy link
Copy Markdown
Member

@cadonna cadonna Jun 5, 2023

Choose a reason for hiding this comment

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

See above

Comment thread streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java Outdated
Copy link
Copy Markdown
Member

@cadonna cadonna Jun 5, 2023

Choose a reason for hiding this comment

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

No verification needed.

Comment thread streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java Outdated
Copy link
Copy Markdown
Member

@cadonna cadonna Jun 5, 2023

Choose a reason for hiding this comment

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

No verification needed.

Comment thread streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java Outdated
Copy link
Copy Markdown
Member

@cadonna cadonna Jun 5, 2023

Choose a reason for hiding this comment

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

No verification needed.

Comment thread streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java Outdated
Copy link
Copy Markdown
Member

@cadonna cadonna Jun 5, 2023

Choose a reason for hiding this comment

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

No verification needed.

Comment thread streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java Outdated
Copy link
Copy Markdown
Member

@cadonna cadonna Jun 5, 2023

Choose a reason for hiding this comment

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

No verification needed.

@clolov
Copy link
Copy Markdown
Contributor Author

clolov commented Jun 5, 2023

Yup, I will address all comments today, thank you for the review!

@clolov clolov force-pushed the KAFKA-14133-PART8 branch from db891f2 to d2d149c Compare June 6, 2023 10:48
@clolov
Copy link
Copy Markdown
Contributor Author

clolov commented Jun 6, 2023

Heya @cadonna, I hope I have addressed all of your comments:

  • Updated the PR's overview
  • Rebased
  • Comments on the code itself

Interestingly enough the test shouldInitializeNewStandbyTasks fails locally when I run all tests, but succeeds when it is ran in isolation. If the build passes I will blame it on something in my environment, but if it fails I would be glad for another pair of eyes as to what might be causing it.

@cadonna
Copy link
Copy Markdown
Member

cadonna commented Jun 12, 2023

The issue with shouldInitializeNewStandbyTasks() seems to be a missed cleanup due to a bug in EasyMock. If I rename the method to shouldInitialiseNewStandbyTasks() (notice the s instead of the z) it works for me locally. Probably, the order in which the test methods are executed matters.
I know, this sounds ridiculous and actually it is ridiculous. 😐
So rename the method. As soon as we will have migrated the whole test class the issue should be gone anyways.

@clolov clolov force-pushed the KAFKA-14133-PART8 branch from 67cf39c to 92fb1aa Compare June 13, 2023 09:48
@clolov
Copy link
Copy Markdown
Contributor Author

clolov commented Jun 13, 2023

Okay, the name has been changed and this has been rebased. I ran checkstyleTest and spotbugsTest and they are passing locally. If everything passes in the automated tests you should be able to merge it at your leisure!

cadonna
cadonna approved these changes Jun 13, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@cadonna cadonna left a comment

Choose a reason for hiding this comment

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

Thanks, @clolov !

LGTM!

@clolov
Copy link
Copy Markdown
Contributor Author

clolov commented Jun 15, 2023

Good morning! I believe that the failing checks are not connected with this change. Is it okay to merge this PR?

@divijvaidya
Copy link
Copy Markdown
Member

divijvaidya commented Jun 15, 2023

Failing tests are not related. Merging this code.

[Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.IdentityReplicationIntegrationTest.testReplicateSourceDefault()](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13711/5/testReport/junit/org.apache.kafka.connect.mirror.integration/IdentityReplicationIntegrationTest/Build___JDK_11_and_Scala_2_13___testReplicateSourceDefault__/)
[Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.IdentityReplicationIntegrationTest.testReplicateSourceDefault()](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13711/5/testReport/junit/org.apache.kafka.connect.mirror.integration/IdentityReplicationIntegrationTest/Build___JDK_11_and_Scala_2_13___testReplicateSourceDefault___2/)
[Build / JDK 17 and Scala 2.13 / org.apache.kafka.controller.QuorumControllerTest.testBalancePartitionLeaders()](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13711/5/testReport/junit/org.apache.kafka.controller/QuorumControllerTest/Build___JDK_17_and_Scala_2_13___testBalancePartitionLeaders__/)
[Build / JDK 8 and Scala 2.12 / org.apache.kafka.controller.QuorumControllerTest.testBalancePartitionLeaders()](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13711/5/testReport/junit/org.apache.kafka.controller/QuorumControllerTest/Build___JDK_8_and_Scala_2_12___testBalancePartitionLeaders__/)

@divijvaidya divijvaidya merged commit c5df47a into apache:trunk Jun 15, 2023
This was referenced Jun 19, 2023
Closed
Merged
@TaklaGerguis TaklaGerguis mentioned this pull request Jun 21, 2023
Closed
@clolov clolov deleted the KAFKA-14133-PART8 branch January 27, 2026 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cadonna cadonna cadonna approved these changes

Assignees

No one assigned

Labels

streams tests Test fixes (including flaky tests)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@clolov @cadonna @divijvaidya