-
Notifications
You must be signed in to change notification settings - Fork 14k
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-13187: Replace EasyMock / PowerMock with Mockito in DistributedHerderTest #14102
KAFKA-13187: Replace EasyMock / PowerMock with Mockito in DistributedHerderTest #14102
Conversation
@C0urante @mimaison would you be able to take a look at this one? This effort was originally started 1 ½ years ago here and has had multiple ownership changes. I started working on this quite a while back myself but had to park it midway numerous times because it was taking way too long but it's finally been completed (or well, at least the tests all consistently pass and I'm hoping we've not lost coverage) 🙂 |
Hm I just noticed that the
This is the same issue (mockito/mockito#2601) that was discovered in #11792 but supposed to be fixed in a newer version of Mockito (which we're already using). I'll take a closer look at why this is re-surfacing only on the JDK 11 build (and not on the other JDK builds; these tests were consistently passing for me locally with JDK 8). Edit: I was able to reproduce this locally. The tests pass on JDK 8 but fail with the above issue on JDK 11 and JDK 17 as well (the CI run was skipping this test on JDK 17 because this bit in the build config wasn't removed - Lines 413 to 418 in 3ba718e
|
…varargs to work around Mockito issue; remove DistributedHerderTest from JDK >= 16 exclude list
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 Yash, this is a truly Herculean effort and I really appreciate the work it took. Everything looks great, I've left a few small comments but I don't expect this to take more than one or two rounds before we can merge.
...untime/src/test/java/org/apache/kafka/connect/runtime/distributed/DistributedHerderTest.java
Outdated
Show resolved
Hide resolved
...untime/src/test/java/org/apache/kafka/connect/runtime/distributed/DistributedHerderTest.java
Show resolved
Hide resolved
...untime/src/test/java/org/apache/kafka/connect/runtime/distributed/DistributedHerderTest.java
Outdated
Show resolved
Hide resolved
...untime/src/test/java/org/apache/kafka/connect/runtime/distributed/DistributedHerderTest.java
Show resolved
Hide resolved
...untime/src/test/java/org/apache/kafka/connect/runtime/distributed/DistributedHerderTest.java
Outdated
Show resolved
Hide resolved
...untime/src/test/java/org/apache/kafka/connect/runtime/distributed/DistributedHerderTest.java
Outdated
Show resolved
Hide resolved
...untime/src/test/java/org/apache/kafka/connect/runtime/distributed/DistributedHerderTest.java
Show resolved
Hide resolved
...untime/src/test/java/org/apache/kafka/connect/runtime/distributed/DistributedHerderTest.java
Outdated
Show resolved
Hide resolved
...untime/src/test/java/org/apache/kafka/connect/runtime/distributed/DistributedHerderTest.java
Outdated
Show resolved
Hide resolved
...untime/src/test/java/org/apache/kafka/connect/runtime/distributed/DistributedHerderTest.java
Show resolved
Hide resolved
…o startConnector, connectorTaskConfigs, startSourceTask
…tion and testCreateConnectorAlreadyExists
…ped (once) in testRestartConnector
…nnector to ensure connector isn't started with null config
… new restart API tests
…once) in testRestartTask
…igning test cases
…connector in testExternalZombieFencingRequestDelayedCompletion
…configurationRetries
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 equally Herculean review Chris, your attention to detail is second to none 😄
I believe I've addressed all the review comments.
...untime/src/test/java/org/apache/kafka/connect/runtime/distributed/DistributedHerderTest.java
Show resolved
Hide resolved
...untime/src/test/java/org/apache/kafka/connect/runtime/distributed/DistributedHerderTest.java
Outdated
Show resolved
Hide resolved
...untime/src/test/java/org/apache/kafka/connect/runtime/distributed/DistributedHerderTest.java
Show resolved
Hide resolved
...untime/src/test/java/org/apache/kafka/connect/runtime/distributed/DistributedHerderTest.java
Outdated
Show resolved
Hide resolved
...untime/src/test/java/org/apache/kafka/connect/runtime/distributed/DistributedHerderTest.java
Outdated
Show resolved
Hide resolved
...untime/src/test/java/org/apache/kafka/connect/runtime/distributed/DistributedHerderTest.java
Outdated
Show resolved
Hide resolved
...untime/src/test/java/org/apache/kafka/connect/runtime/distributed/DistributedHerderTest.java
Outdated
Show resolved
Hide resolved
...untime/src/test/java/org/apache/kafka/connect/runtime/distributed/DistributedHerderTest.java
Outdated
Show resolved
Hide resolved
...untime/src/test/java/org/apache/kafka/connect/runtime/distributed/DistributedHerderTest.java
Show resolved
Hide resolved
...untime/src/test/java/org/apache/kafka/connect/runtime/distributed/DistributedHerderTest.java
Show resolved
Hide resolved
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, thanks again Yash!
I should note that because this test makes heavy use of Mockito's strict stubbing feature, I've opened #14186 to enable reporting of unused stubbings in our CI builds. I've verified locally that there are no unused stubbings in this PR ( Test failures in CI also appear unrelated. Merging... |
…HerderTest (apache#14102) Reviewers: Chris Egerton <chrise@aiven.io>
…HerderTest (apache#14102) Reviewers: Chris Egerton <chrise@aiven.io>
…HerderTest (apache#14102) Reviewers: Chris Egerton <chrise@aiven.io>
Mocking with explicit constructor fails with varargs parameter mockito/mockito#2601 is resolved in the current version of Mockito that we're using (-> the issue still exists on newer JDKs, a different workaround hack is used (by modifying the testing-only constructor for4.11.0
) so the workaround hack is no longer needed.DistributedHerder
)Mockito
stubbings are applicable any number of times by default (unlike inEasyMock
where the default is a single invocation being stubbed).EasyMock
way of stacking up expectations and then verifying them automatically took care of verifying the number of invocations for stubbed methods (assuminganyTimes()
wasn't used in the expect calls).WorkerGroupMember::wakeup
in large tests that involve multiple calls toDistributedHerder::tick
), there are some cases where it is crucial to the test's coverage and this patch attempts to retain that coverage wherever it seemed important by using the times() verification mode explicitly.DistributedHerder
class itself -herderExecutor
field was bumped up for this specific test -kafka/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/distributed/DistributedHerderTest.java
Line 3906 in ff390ab
uponShutdown
parameter in order to work around Mocking with explicit constructor fails with varargs parameter mockito/mockito#2601 here -kafka/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedHerder.java
Line 277 in 938fee2
Committer Checklist (excluded from commit message)