Skip to content
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.

Sign up for GitHub

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

Jump to bottom

KAFKA-14059 Replace PowerMock with Mockito in WorkerSourceTaskTest #13383

Merged
merged 15 commits into from Jul 10, 2023
Merged

KAFKA-14059 Replace PowerMock with Mockito in WorkerSourceTaskTest #13383

merged 15 commits into from Jul 10, 2023

Conversation

hgeraldino
Copy link
Contributor

@hgeraldino hgeraldino commented Mar 13, 2023 •
edited

Follows more or less the same principles as #13191

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@C0urante C0urante added connect tests Test fixes (including flaky tests) labels Mar 13, 2023
C0urante
C0urante reviewed Mar 21, 2023
View reviewed changes
Copy link
Contributor

@C0urante C0urante left a comment

Choose a reason for hiding this comment

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

Thanks @hgeraldino! I've made it about halfway through in this pass; will try to revisit this week or the next and look at the rest.

Also, similar to #13191 (comment), can we add verifyNoMoreInteractions(statusListener); to the test's tearDown method?

connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated Show resolved Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated Show resolved Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated Show resolved Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated Show resolved Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated Show resolved Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Show resolved Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated Show resolved Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated Show resolved Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Show resolved Hide resolved
@hgeraldino
Copy link
Contributor Author

Thanks @hgeraldino! I've made it about halfway through in this pass; will try to revisit this week or the next and look at the rest.

Also, similar to #13191 (comment), can we add verifyNoMoreInteractions(statusListener); to the test's tearDown method?

Good catch, will add it.

@hgeraldino hgeraldino force-pushed the workersourcetask branch 2 times, most recently from 6197644 to 1e46ec4 Compare April 24, 2023 03:39
@hgeraldino
Copy link
Contributor Author

Thanks for your feedback @C0urante.

I finally had time this past week and tweaked the test to address your comments and fix coverage.

C0urante
C0urante reviewed Jun 2, 2023
View reviewed changes
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated Show resolved Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated Show resolved Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated Show resolved Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated Show resolved Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated Show resolved Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated Show resolved Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated Show resolved Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated Show resolved Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated Show resolved Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Show resolved Hide resolved
C0urante
C0urante reviewed Jun 2, 2023
View reviewed changes
Copy link
Contributor

@C0urante C0urante left a comment

Choose a reason for hiding this comment

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

Thanks @hgeraldino! Apologies for the wait, releases, Kafka Summit London, and a few other things have made it difficult to find for review over the last few weeks.

I've finished a complete pass over the PR; once everything is addressed this should be good to merge.

connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated Show resolved Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated Show resolved Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated Show resolved Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated Show resolved Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated Show resolved Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated Show resolved Hide resolved
@hgeraldino hgeraldino force-pushed the workersourcetask branch 2 times, most recently from 4037471 to 357f582 Compare June 4, 2023 21:45
@hgeraldino hgeraldino requested a review from C0urante June 4, 2023 21:46
@hgeraldino
Copy link
Contributor Author

Thanks @C0urante for your thoroughly review! I think I've addressed all your comments.

I've also picked up the work to have the WorkerSinkTaskTest migrated next (this one is a bit trickier), hope to have a PR soon

C0urante
C0urante reviewed Jun 5, 2023
View reviewed changes
Copy link
Contributor

@C0urante C0urante left a comment

Choose a reason for hiding this comment

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

Thanks @hgeraldino, really close!

connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated Show resolved Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated Show resolved Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated Show resolved Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated Show resolved Hide resolved
hgeraldino and others added 10 commits June 11, 2023 10:15
@hgeraldino
KAFKA-14059 Replace EasyMock and PowerMock with Mockito in WorkerSour…
07f96fe 
…ceTaskTest
@hgeraldino
KAFKA-14059 Replace EasyMock and PowerMock with Mockito in WorkerSou…
97e208a 
…rceTaskTest
@hgeraldino
Few tweaks to make JDK8 builds happy
34f4c7e
@hgeraldino @C0urante
Update connect/runtime/src/test/java/org/apache/kafka/connect/runtime…
3b6bb8b 
…/WorkerSourceTaskTest.java

Co-authored-by: Chris Egerton <fearthecellos@gmail.com>
@hgeraldino @C0urante
Update connect/runtime/src/test/java/org/apache/kafka/connect/runtime…
9996f6d 
…/WorkerSourceTaskTest.java

Co-authored-by: Chris Egerton <fearthecellos@gmail.com>
@hgeraldino @C0urante
Update connect/runtime/src/test/java/org/apache/kafka/connect/runtime…
9e55cd9 
…/WorkerSourceTaskTest.java

Co-authored-by: Chris Egerton <fearthecellos@gmail.com>
@hgeraldino
PR feedback
611d76a
@hgeraldino @C0urante
Update connect/runtime/src/test/java/org/apache/kafka/connect/runtime…
389d3f3 
…/WorkerSourceTaskTest.java

Co-authored-by: Chris Egerton <fearthecellos@gmail.com>
@hgeraldino @C0urante
Update connect/runtime/src/test/java/org/apache/kafka/connect/runtime…
f7aa082 
…/WorkerSourceTaskTest.java

Co-authored-by: Chris Egerton <fearthecellos@gmail.com>
@hgeraldino
PR feedback
0b4c2fa
@hgeraldino hgeraldino requested a review from C0urante July 1, 2023 16:07
C0urante
C0urante reviewed Jul 6, 2023
View reviewed changes
Copy link
Contributor

@C0urante C0urante left a comment

Choose a reason for hiding this comment

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

Thanks @hgeraldino! Some final suggestions and then this will be good to go. I think it's important that these messages are accurate as they help illustrate the purpose of these tests and may save time for developers who have to read/rewrite them in the future.

connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated Show resolved Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated Show resolved Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated Show resolved Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated Show resolved Hide resolved
@hgeraldino @C0urante
Update connect/runtime/src/test/java/org/apache/kafka/connect/runtime…
6562053 
…/WorkerSourceTaskTest.java

Co-authored-by: Chris Egerton <fearthecellos@gmail.com>
hgeraldino and others added 3 commits July 6, 2023 13:23
@hgeraldino @C0urante
Update connect/runtime/src/test/java/org/apache/kafka/connect/runtime…
04fe47f 
…/WorkerSourceTaskTest.java

Co-authored-by: Chris Egerton <fearthecellos@gmail.com>
@hgeraldino @C0urante
Update connect/runtime/src/test/java/org/apache/kafka/connect/runtime…
abe246e 
…/WorkerSourceTaskTest.java

Co-authored-by: Chris Egerton <fearthecellos@gmail.com>
@hgeraldino @C0urante
Update connect/runtime/src/test/java/org/apache/kafka/connect/runtime…
8d71de4 
…/WorkerSourceTaskTest.java

Co-authored-by: Chris Egerton <fearthecellos@gmail.com>
@hgeraldino
Copy link
Contributor Author

Committed the suggestions, thanks again for reviewing @C0urante!

BTW, I also opened #13951 to have WorkerSinkTaskThreadedTest migrated. There's only one more in my bucket (WorkerSinkTaskTest) which has proven to be quite challenging

C0urante
C0urante reviewed Jul 10, 2023
View reviewed changes
Copy link
Contributor

@C0urante C0urante left a comment

Choose a reason for hiding this comment

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

Thanks @hgeraldino, LGTM!

@C0urante C0urante merged commit 6368d14 into apache:trunk Jul 10, 2023
1 check failed
Cerchie pushed a commit to Cerchie/kafka that referenced this pull request Jul 25, 2023
@hgeraldino
KAFKA-14059 Replace PowerMock with Mockito in WorkerSourceTaskTest (a…
39b2794 
…pache#13383)

Reviewers: Chris Egerton <chrise@aiven.io>
@hgeraldino hgeraldino deleted the workersourcetask branch October 5, 2023 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@C0urante C0urante C0urante approved these changes

Assignees
No one assigned
Labels
connect tests Test fixes (including flaky tests)
Projects
None yet
Milestone
No milestone
2 participants
@hgeraldino @C0urante