-
Notifications
You must be signed in to change notification settings - Fork 14.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KAFKA-16878: Remove powermock and easymock from code base #16236
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@FrankYang0529 could you please rebase code to trigger QA? |
8b7703f
to
9fecf47
Compare
Wow, we're finally doing it! |
There was a problem hiding this 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!
We still have a handful of comments mentioning EasyMock/PowerMock, it would be good to remove them as well.
@@ -522,13 +522,6 @@ subprojects { | |||
// The suites are for running sets of tests in IDEs. | |||
// Gradle will run each test class, so we exclude the suites to avoid redundantly running the tests twice. | |||
def testsToExclude = ['**/*Suite.class'] | |||
// Exclude PowerMock tests when running with Java 16 or newer until a version of PowerMock that supports the relevant versions is released |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's another comment about PowerMock at line 822, can we remove it as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are also comments about EasyMock in 4 places:
- 2 in
DescribeTopicPartitionsRequestHandlerTest
- 2 in
KafkaApisTest
Let's clean those up as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mimaison, thanks for the review. I remove all remaining comments.
That is a lengthy journey 😄 |
9fecf47
to
f01ff39
Compare
@FrankYang0529 thanks for this patch. I will merge it after getting approve from @mimaison |
oh, the QA is not completed. I re-trigger it ... |
f01ff39
to
633bce6
Compare
I rebase trunk branch to include #16248. |
@FrankYang0529 FYI it turned out there's another blocking test so we may need to wait #16249 gets merged not only 16248 |
Thank you for the reminder. We will wait for it 👍 |
#16249 is merged. FYI. |
@@ -826,7 +819,6 @@ subprojects { | |||
toolVersion = versions.jacoco | |||
} | |||
|
|||
// NOTE: Jacoco Gradle plugin does not support "offline instrumentation" this means that classes mocked by PowerMock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't just remove the first line, the following line does not mean anything now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for that. I remove related comments.
Signed-off-by: PoAn Yang <[email protected]>
633bce6
to
135dd6d
Compare
Rebased it. Thanks. |
failed tests pass on my local. |
Well done team, this was a Herculean effort! |
Reviewers: TaiJuWu <[email protected]>, Chia-Ping Tsai <[email protected]>
This is the follow-up of KAFKA-16223. It should include following changes:
Committer Checklist (excluded from commit message)