KAFKA-14060: Replace EasyMock and PowerMock with Mockito in AbstractWorkerSourceTaskTest#13191
Conversation
edcc7c9 to
e47c1a6
Compare
e29f304 to
534f8c7
Compare
clolov
left a comment
There was a problem hiding this comment.
Thank you for the contribution! The changes make sense to me and I don't think I have any comments, but I will leave @gharris1727 or @C0urante to give it another pass.
C0urante
left a comment
There was a problem hiding this comment.
Thanks @hgeraldino!
I'm a little confused by the amount of churn here--it seems like a lot of utility methods related to topic creation, record transformation, etc. have been removed and their contents inlined directly into test cases. If this isn't necessary for the migration, can we try to retain that approach in order to reduce duplication?
It's also worth noting that there are unused stubbings in some of these tests, which should be failing the build but are not at the moment due to KAFKA-14682. You can find these unused stubbings by running ./gradlew :connect:runtime:test --tests AbstractWorkerSourceTaskTest in your command line, or possibly by running the entire AbstractWorkerSourceTaskTest test suite in IntelliJ (which is how I discovered them). These unused stubbings should be removed before we merge the PR.
That's fair. Personally I prefer self-contained test methods hat can be read top-bottom (without having to jump around), even if it violates the DRY principle. But I'm ok on keeping the existing structure & style, so I put back the (revised) helper methods |
@C0urante, I have a question about this, why didn't the GitHub builds fail with this when running the tests (or am I misinterpreting the output)? I wouldn't expect for reviewers to have to locally build every pull request to carry out such checks. |
|
@clolov I've provided some information on that front in the description for the ticket I linked. TL;DR: I believe it's because we use |
|
Oh, sorry, I don't know where I was looking when I read your response. Apologies for the back-and-forth. I will look into the ticket. |
C0urante
left a comment
There was a problem hiding this comment.
Thanks @hgeraldino, I've given this a full pass this time around. Appreciate the preservation of the utility methods where appropriate, it's made review much easier.
|
Thanks for the thorough review @C0urante! I'll get to it right away. |
…/AbstractWorkerSourceTaskTest.java Co-authored-by: Chris Egerton <fearthecellos@gmail.com>
…/AbstractWorkerSourceTaskTest.java Co-authored-by: Chris Egerton <fearthecellos@gmail.com>
C0urante
left a comment
There was a problem hiding this comment.
Thanks @hgeraldino, this is almost there. One nit left inline and one overarching comment: we're losing test coverage in a few places with these tests. Anywhere in the old tests where we invoked expectTopicCreation, we should have a corresponding invocation in the new tests of verifyTopicCreation, and likewise for expectTaskGetTopic (old) and verifyTaskGetTopic (new).
Once these are addressed this should be good to go!
|
Added verifications for the getTopic and topicCreation expectations in the few places where it was missing. |
C0urante
left a comment
There was a problem hiding this comment.
LGTM, thanks @hgeraldino!
This PR is part of the larger initiative of migrating tests from EasyMock/PowerMock to Mockito](https://issues.apache.org/jira/browse/KAFKA-7438), and is a "blocker" of https://issues.apache.org/jira/browse/KAFKA-14659 (not really a blocker, but it'd be nice to write tests for that bug fix after this test class is migrated)
Committer Checklist (excluded from commit message)