kafka
kafka copied to clipboard
apache
Replace EasyMock/PowerMock with Mockito in DistributedHerderTest
Adding mockitoJunitJupiter test dependency to :connect:runtime project
Converting class level mocks from EasyMock -> Mockito
Changing first test (testJoinAssignment) to use Mockito constructs, along with its assertion helper methods
Made a bit more progress on this. Converting testRebalance took me a very long time (due to the different paradigm that Mockito uses versus Powermock). The lack of a Mockito equivalent of PowerMock.verifyAll() takes a while to work through (have to manually figure out the verifications). The next test - testIncrementalCooperativeRebalanceForNewMember - went much faster at least.
Also, had to put in a hacky workaround for mockito/mockito#2601 for now.
Something I've learned, that I feel is not adequately covered by this wiki:
EasyMock expectations seem to stack up, as you build them. Or perhaps more like a FIFO queue. Expectations are queued up, then as invocations happen, the front of the queue is removed to satisfy the invocation, and the next mock is moved to the front.
On the other hand, Mockito only seems to have a single current mock behavior for a particular set of argument matchers. This means that new expectations can't be set until after the actual code exercises the first one (ex: the calls to expectRebalance in our tests). Because EasyMock is broken into phases, it allows for the "stacking" of those expectations in one phase, followed by execution, followed by verification. But with Mockito, it's necessary to follow a more staggered approach (set expectation, exercise code, set new expectation, exercise again, verify at end).
Just an observation for anyone else (or myself) in the future that is hopefully useful.
@jeff303 do you still plan to work on this? Happy to provide a round of review for what's here already if you'd like some feedback.
@divijvaidya This is another conversion PR that needs help if you have cycles. It would also simplify #12295.
Hey @ijuma! Divij is taking some time off, but I can have a look in the meantime and try to get it over the line.
Hi @clolov, just an info, to avoid duplicate work..
I've started working on this one few days ago. There is a Jira ticket: Replace EasyMock and PowerMock with Mockito for DistributedHerderTest assigned to me from 02/Aug/22 23:45 .
Hi @dplavcic! Any update? The refactoring to the DistributedHerderTest is blocking a few other PRs; if you don't have the time to tackle it, let us know and I'm sure someone else would be happy to take over.
Hi @C0urante, I'm really sorry but I didn't have time to complete this on planned schedule (last week). It would be awesome if someone else can complete this (and unblock other PRs).
@dplavcic No worries, it happens :)
@yashmayya @mdedetrich @clolov @divijvaidya any interest in taking this one on? Fairly high-priority as it's blocking other PRs.
@C0urante I won't be able to pick this up until 16th Oct. If no one has picked this up my then, I would happily pick it up.
A problem that is common amongst the tests that I am migrating (WorkerSinkTaskTest and WorkerSinkTaskThreadedTest) and this PR is the FIFO queue stubbing not available in Mockito as @jeff303 mentioned earlier. The approach that I have taken up to resolve that is to use OngoingStubbing<T> and chaining the expectations on that. We can also do that for void method using:
doNothing().doThrow(exception).doNothing().when(mockedObject).methodThatReturnsVoid()
Does anyone else have a better solution to this? In my scenario, I want to queue expectations for consumer.poll and consumer.assignment() methods.
@divijvaidya I've also used the OngoingStubbing class to set up multiple expectations for a mock at once. I'm not sure if it's applicable to your case, but I found that it wasn't too painful to create a utility method that accepts and returns an OngoingStubbing in cases where the logic was too complicated to repeat each time. See here and here for an example of that approach.
It's also worth noting that, due to the interleaving of setting up mock expectations with actually executing test code, often times it's not actually necessary to establish multiple expectations on a single mock when using Mockito. There are exceptions for this, of course, but otherwise, it's usually better to set a single expectation, run some test code, set a new expectation, etc. instead of setting up everything all at once (which is necessary with EasyMock/PowerMock).
@mdedetrich Great, thanks! I've assigned KAFKA-13187 to you to let others know that the ticket is being worked on. If you need to drop this for some reason, can you unassign the ticket and ping me either here or on the ticket?
Realistically, I am not going to have time to finish this one up, despite having spent a considerable amount of time months back. Hopefully some others can build upon what I discovered to finish it up.
@mdedetrich are you still planning to work on this one? If not, I'd be happy to take this on and try to drive it to closure soon.
@yashmayya I am currently on a company offside, will get back to you on this at the end of the week
@yashmayya I don't really have time to finish this any time soon so feel free to take over
@mdedetrich no problem, I've assigned https://issues.apache.org/jira/browse/KAFKA-13187 to myself and I'll start working on it next week.