KAFKA-14134: Replace EasyMock with Mockito for WorkerConnectorTest

[yashmayya]

https://issues.apache.org/jira/browse/KAFKA-14134

From https://issues.apache.org/jira/browse/KAFKA-7438:

Development of EasyMock and PowerMock has stagnated while Mockito continues to be actively developed. With the new Java cadence, it's a problem to depend on libraries that do bytecode generation and are not actively maintained. In addition, Mockito is also easier to use.

[yashmayya]

@mimaison or @C0urante could you please take a look? The changes are fairly straightforward for this one.

[divijvaidya]

Thank you for the review. I added a minor comment, otherwise looks good.

[divijvaidya]

I am adding this comment here to help other reviewers in reviewing the changes in these lines.

It is ok to remove these void calls because Mockito automatically creates stubs for void method calls. As long as we are verifying that they are being invoked, we should be good.

[C0urante]

Thanks Yash. I haven't looked closely at every individual case yet, but I've left a few stylistic remarks. I'm hoping that if we can clean some of the logic here into succinct, reusable methods then it'll be easier to review and verify with certainty that we haven't mistranslated anything or accidentally lost guarantees. Every case I have examined looks good so far though 👍

[yashmayya]

Thanks for the reviews @divijvaidya and @C0urante! I've incorporated your feedback, could you please take another look?

[divijvaidya]

Looks good from my side!

[C0urante]

Looking pretty good! Just a few more comments.

[C0urante]

Almost every call to verifyCleanShutdown is preceded by a check to verify that listener::onShutdown was invoked. Could we add this logic to verifyCleanShutdown? Thinking of something like this:

private void verifyCleanShutdown() {
    verifyShutdown(true);
}

private void verifyShutdown(boolean clean) {
    verify(ctx).close();
    verify(offsetStorageReader).close();
    verify(offsetStore).stop();
    if (clean) {
        verify(listener).onShutdown(CONNECTOR);
    }
}

[C0urante]

We can also follow a similar approach with startup logic. This would involve renaming verifyCleanInitialize to verifyStartup and adding a clean parameter to the method:

private void verifyCleanStartup() {
    verifyStartup(true);
}

private void verifyStartup(boolean clean) {
    verify(connector).version();
    if (connector instanceof SourceConnector) {
        verify(offsetStore).start();
        verify(connector).initialize(any(SourceConnectorContext.class));
    } else {
        verify(connector).initialize(any(SinkConnectorContext.class));
    }
    if (clean) {
        verify(connector).start(CONFIG);
        verify(listener).onStartup(CONNECTOR);
    }
}

(clean isn't the most accurate term here since there are clean startups in the paused state that would involve passing false for that parameter; feel free to replace with something better)

[yashmayya]

The one for shutdown makes sense but I think with startup there are three different cases -

i) No call to connector.start or listener.onStartup (failure in initialization OR started in the paused state)
ii) Calls to both connector.start and listener.onStartup (successful start)
iii) Call to only connector.start (connector started in the paused state and then resumed OR failure on startup)

I think it might be more readable to keep these verify calls in the test methods as is rather than trying to force fit them into (in)appropriately named methods, WDYT?

[C0urante]

Yeah, that's fair. LGTM 👍

[C0urante]

LGTM, thanks Yash!