Skip to content

KAFKA-14133: Migrate ActiveTaskCreator mock in TaskManagerTest to Mockito#13681

Merged
cadonna merged 5 commits intoapache:trunkfrom
mehbey:KAFKA-14133
Jun 1, 2023
Merged

KAFKA-14133: Migrate ActiveTaskCreator mock in TaskManagerTest to Mockito#13681
cadonna merged 5 commits intoapache:trunkfrom
mehbey:KAFKA-14133

Conversation

@mehbey
Copy link
Copy Markdown
Contributor

@mehbey mehbey commented May 8, 2023

This pull requests migrates the ActiveTaskCreator 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.
Please see two examples that follows the same pattern below:
#13529
#13621

Summary of testing strategy (including rationale)

Verified that all tests are passing

Copy link
Copy Markdown
Member

@divijvaidya divijvaidya left a comment

Choose a reason for hiding this comment

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

Thank you for your first contribution to the community @mehbey. Welcome!

I think that this PR may be incomplete since the objective of JIRA-14133 (i.e. to migrate from easymock to mockito) is not accomplished in this PR. We have a large number of easymock usage in this PR. Is that intentional? Nevermind, I missed reading the description earlier where you mentioned that it is intentional to reduce the scope of changes.

As a reviewer, I would have personally have preferred an approach where we are migrating a few tests at a time completely instead of a PR per mock, but this approach works as well.

@cadonna cadonna added streams tests Test fixes (including flaky tests) labels May 8, 2023
…kito V2. 1) Addressed @divijvaidya's comment. 2) Resolved merge conflict with the latest changes from trunk
@mehbey
Copy link
Copy Markdown
Contributor Author

mehbey commented May 8, 2023

Addressed Divij's comment and re-based with the latest changes.

@mehbey
Copy link
Copy Markdown
Contributor Author

mehbey commented May 9, 2023

Verified that failing Testing are not related to this change

[2023-05-08T22:22:09.340Z] 1: Task failed with an exception.

[2023-05-08T22:22:09.340Z] -----------

[2023-05-08T22:22:09.340Z] * What went wrong:

[2023-05-08T22:22:09.340Z] Execution failed for task ':streams:upgrade-system-tests-0102:integrationTest'.

[2023-05-08T22:22:09.340Z] > Process 'Gradle Test Executor 151' finished with non-zero exit value 1

[2023-05-08T22:22:09.340Z]   This problem might be caused by incorrect test process configuration.

[2023-05-08T22:22:09.340Z]   Please refer to the test execution section in the User Manual at https://docs.gradle.org/8.1.1/userguide/java_testing.html#sec:test_execution

[2023-05-08T22:22:09.340Z] 

[2023-05-08T22:22:09.340Z] * Try:

[2023-05-08T22:22:09.340Z] > Run with --stacktrace option to get the stack trace.

[2023-05-08T22:22:09.340Z] > Run with --info or --debug option to get more log output.

[2023-05-08T22:22:09.340Z] > Run with --scan to get full insights.

[2023-05-08T22:22:09.340Z] ==============================================================================

[2023-05-08T22:22:09.340Z] 

[2023-05-08T22:22:09.340Z] 2: Task failed with an exception.

[2023-05-08T22:22:09.340Z] -----------

[2023-05-08T22:22:09.340Z] * What went wrong:

[2023-05-08T22:22:09.340Z] Execution failed for task ':connect:mirror:integrationTest'.

[2023-05-08T22:22:09.340Z] > Process 'Gradle Test Executor 130' finished with non-zero exit value 137

[2023-05-08T22:22:09.340Z]   This problem might be caused by incorrect test process configuration.

[2023-05-08T22:22:09.340Z]   Please refer to the test execution section in the User Manual at https://docs.gradle.org/8.1.1/userguide/java_testing.html#sec:test_execution

[2023-05-08T22:22:09.340Z] 

[2023-05-08T22:22:09.340Z] * Try:

[2023-05-08T22:22:09.340Z] > Run with --stacktrace option to get the stack trace.

[2023-05-08T22:22:09.340Z] > Run with --info or --debug option to get more log output.

[2023-05-08T22:22:09.340Z] > Run with --scan to get full insights.

Copy link
Copy Markdown
Contributor

@clolov clolov left a comment

Choose a reason for hiding this comment

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

Thank you very much! There are a couple of stylistic changes which I have aimed to provide an alternative for. Let me know your thoughts if you disagree.

@mehbey
Copy link
Copy Markdown
Contributor Author

mehbey commented May 12, 2023

Thank you for the review @clolov - I will address the comments shortly

@mehbey
Copy link
Copy Markdown
Contributor Author

mehbey commented May 12, 2023

addressed all comments from @clolov.

@cadonna - when you get a chance would you be able to review?

Copy link
Copy Markdown
Contributor

@machi1990 machi1990 left a comment

Choose a reason for hiding this comment

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

I noticed a small unrelated typo as I was reviewing the PR (unrelated to this PR) but I think it can be fixed along this PR if you are okay with this. The typo is in

// check that if there's no records proccssible, we would stop early

It should read processable and not proccssible. Thaks

@mehbey
Copy link
Copy Markdown
Contributor Author

mehbey commented May 15, 2023

@machi1990, thank you for the review. I have addressed your comment.

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.

@mehbey Thanks for your PR!

Sorry that it took me so long to review it!

Please fix the indentations that @clolov pointed out, the needless multiple calls to .thenReturn(), and the duplicate line (https://github.com/apache/kafka/pull/13681/files#r1191127707).

Once that is done, I think the PR is ready to merge.

@mehbey
Copy link
Copy Markdown
Contributor Author

mehbey commented May 31, 2023

@cadonna Thank you for the review. Addressed all outstanding comments. Please take another look when you get some time. Thank you.

@cadonna
Copy link
Copy Markdown
Member

cadonna commented Jun 1, 2023

Build failures are unrelated:

Build / JDK 17 and Scala 2.13 / kafka.admin.TopicCommandIntegrationTest.testDescribeAndListTopicsWithoutInternalTopics(String).quorum=kraft
Build / JDK 17 and Scala 2.13 / org.apache.kafka.trogdor.coordinator.CoordinatorTest.testTaskRequestWithOldStartMsGetsUpdated()
Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsWithCustomForwardingAdminIntegrationTest.testReplicateSourceDefault()
Build / JDK 11 and Scala 2.13 / integration.kafka.server.FetchFromFollowerIntegrationTest.testRackAwareRangeAssignor()
Build / JDK 11 and Scala 2.13 / org.apache.kafka.controller.QuorumControllerTest.testBalancePartitionLeaders()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

streams tests Test fixes (including flaky tests)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants