kafka
kafka copied to clipboard
apache
KAFKA-14006: Parameterize WorkerConnectorTest suite
Tweaks this test class to add complete coverage for both source and sink connectors. This helped unearth a NullPointerException in the shutdown logic for sink connectors introduced by a not-yet-released change for KIP-618.
Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
@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 org.easymock.Mockorg.easymock.Mock annotation, instead of org.powermock.api.easymock.annotation.Mock, which is used by at least some of the tests that have to be skipped for Java 16+.
@mimaison @showuon if you have a moment, would you mind taking a look? Thanks!
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