KAFKA-14006: Parameterize WorkerConnectorTest suite #12307
Conversation
There was a problem hiding this comment.
Hello! I believe there is a general push to replace PowerMock and EasyMock with Mockito here https://issues.apache.org/jira/browse/KAFKA-7438. Since I have been involved with the JUnit 4 to JUnit 5 migration I have also started looking out for PowerMock, EasyMock -> Mockito opportunities (or at the very least to not make the migration effort harder). Would you be willing to, instead of changing from EasyMock to PowerMock to try and change things to Mockito directly so we can alleviate some future effort?
| if (connectorType == ConnectorType.SOURCE) { | ||
| offsetStorageReader.close(); | ||
| expectLastCall(); | ||
|
|
||
| offsetStore.stop(); | ||
| expectLastCall(); | ||
| offsetStore.stop(); | ||
| expectLastCall(); | ||
| } |
There was a problem hiding this comment.
I see these lines repeat in a few tests. Would it be better to move them to a separate method and invoke that method here?
There was a problem hiding this comment.
I don't see this as significantly different than a lot of the other repeated sections in the test, such as the expectations for clean startup of a non-paused connector. While it might be nice to fold some of these sections into reusable methods, the changes here don't make this repetition any worse than it already is, and I'm worried that decomposing them might complicate the Mockito transition that @clolov has mentioned.
There was a problem hiding this comment.
hmm I did see the other repeated sections in the tests but since they are out of scope for the context of this PR, didn't bring them up. I guess we can live with these newer repetitions.
|
@clolov Hi! I'm aware of the effort to transition to Mockito; I don't think this change makes that transition significantly harder. There are already examples of parameterized tests in the Connect code base that use Mockito (such as the WorkerTest suite) that can be followed when we make that transition for this test suite, and all of the other logic in the changes here is trivial to translate between the two mocking libraries. I'd rather keep the changes here focused and in-scope for the ticket that they aim to address, so I would prefer not to add switching over to a new mocking library to this PR. |
|
Okay, your reasoning makes sense to me. Would you not need to add the class to the list in https://github.com/apache/kafka/blob/trunk/build.gradle#L397-L416? |
|
@clolov I don't think it's necessary to add these tests to that list since they're still passing locally with Java versions 16-18 and on Jenkins with Java 17. It's a little risky to add tests to that list, too, since it causes them to be skipped with Java versions 16+. FWIW, it seems like the reason that tests still work is that they use the |
|
Got it, okay, looks good to me, thanks for the detailed explanation :) |
|
LGTM! |
|
Rebuilt locally after rebasing on trunk and discovered a few small issues caused by newly-introduced tests; pushed a fix for those. Will merge pending CI build |
|
Test failures appear unrelated; merging... |
Jira
Tweaks this test class to add complete coverage for both source and sink connectors. This helped unearth a
NullPointerExceptionin the shutdown logic for sink connectors introduced by a not-yet-released change for KIP-618.Committer Checklist (excluded from commit message)