KAFKA-14132; Replace EasyMock with Mockito in KafkaBasedLogTest

[mdedetrich]

Migrates the tests from EasyMock to Mockito. Notes about non trivial changes below

• Originally the test was using PowerMock.expectPrivate to mock protected fields. There is no equivalent for mockito so instead we just subclassed KafkaBasedLog with MockedKafkaBasedLog and override the fields to be returned.
• The test was using Whitebox.<Thread>getInternalState(store, "thread") to access an internal private thread field. Similarly mockito has no such method so instead I resorted to using FieldUtils from Apache commons whos implementation was abstracted away into a getStorePrivateThread() method. I had to also add an exclusion rule to checkstyle/import-control.xml for this new import. Thread is now package private so no reflection is used.
• Some tests had a throw Exception even though they never threw an exception so this was removed.

[mdedetrich]

Minor tidbit, in the old test initializer was actually the wrong type (its a Runnable when it should be a Supplier) so if it was ever accessed during a test it would have thrown a ClassCastException (or something along those lines). Furthermore the createPartialMock was actually missing an argument (constructor has 7 arguments, there are only 6 here).

This is why the version in this PR as () -> null as a topicAdminSupplier and null for the initializer. These fields are never actually called (or if they are they are setup with another method, i.e. setupWithAdmin which inserts the correct fields)

[gharris1727]

I think the @Mock private Runnable initializer is the incorrect signature here, and should be changed to be a Consumer mock. Otherwise the initializer value isn't behaving like a mock for the mocked kafka based log.

[mdedetrich]

I think the @mock private Runnable initializer is the incorrect signature here, and should be changed to be a Consumer mock. Otherwise the initializer value isn't behaving like a mock for the mocked kafka based log.

So it appears that the @Mock private Runnable initializer is being used correctly in the setupWithAdmin method and if you look at setupWithAdmin you can confirm this because we aren't using a mock, instead we actually using the raw constructor on an extended class, i.e.

    private void setupWithAdmin() {
        Supplier<TopicAdmin> adminSupplier = () -> admin;
        java.util.function.Consumer<TopicAdmin> initializer = admin -> { };
        store = new MockedKafkaBasedLog(TOPIC, PRODUCER_PROPS, CONSUMER_PROPS, adminSupplier, consumedCallback, time, initializer);
    }

I believe the oversight was just in the setUp of the mock.

[mdedetrich]

So I have resolved this in the Fix expectStart and expectStop along with initializer commit.

[mdedetrich]

@C0urante Pinging for visibility

[mdedetrich]

This isn't actually used.

[C0urante]

Thanks for the cleanup 🎉

[gharris1727]

Does the initializer.run() call trivially satisfy the following verify() call?

[mdedetrich]

Also answered in #12781 (comment)

[gharris1727]

Does the producer.close() call trivially satisfy the following verify() call?

[gharris1727]

Also it looks like this method is not really translated from easymock yet, as it is separate from the store.stop() calls throughout the test. I think that instead of calling store.stop(), the test should call expectStop, and then the expectStop calls store.stop and asserts that the shutdown procedure happens.

[mdedetrich]

Does the producer.close() call trivially satisfy the following verify() call?

So to be honest I don't know whats going on here. The literal translation of

producer.close();
PowerMock.expectLastCall();

to Mockito is

producer.close();
verify(producer, times(1)).close();

To answer your question this does seem fishy because of course we call producer.close() we expect it to be called unless some exception is thrown (which would fail the test otherwise anyways).

Also it looks like this method is not really translated from easymock yet, as it is separate from the store.stop() calls throughout the test. I think that instead of calling store.stop(), the test should call expectStop, and then the expectStop calls store.stop and asserts that the shutdown procedure happens.

Are you alluding to the fact that the equivalent PowerMock.verifyAll(); is missing at the end of the test? If thats so then yes this is indeed the case and this is one of the problems with PowerMock/EasyMock is that its not that explicit, i.e. PowerMock.verifyAll() will verify methods that are directly called (which almost all of the time is pointless) where as ideally you are meant to use verify on methods that are called indirectly called by tests to ensure that they are being executed.

So I guess to answer your test more directly, I can see what you are saying but I would like to know what exactly a "shutdown procedure" actually means in terms of mocking, i.e. what methods (aside from store.stop() which we directly call) should we be verifying? If there aren't any indirect methods that need to be verified with .verify then unless I am missing something nothing really needs to be done here. As long as .stop is successfully executed without throwing an exception then that is the best we can do?

[mdedetrich]

Okay so you can largely ignore my previous comment, I missed the replayAll which was being called elsewhere which means we are actually expecting producer.close() to be called indirectly. I did this quickly and realized that I also need to change the initial argument for the mock constructor in setUp (see #12781 (comment))

[gharris1727]

I think the @Mock private Runnable initializer is the incorrect signature here, and should be changed to be a Consumer mock. Otherwise the initializer value isn't behaving like a mock for the mocked kafka based log.

[mdedetrich]

@C0urante @gharris1727

So now that I understand what the expectStart and expectStop was meant to do, I have both fixed this issue as well as the initializer being of the wrong type problem in the Fix expectStart and expectStop along with initializer commit.

[mdedetrich]

As pointed out by @gharris1727 , the type of initializer had to be changed because it was in fact incorrect. I have no idea how this happened to work beforehand with EasyMock, but this appears to work alongside with changing expectStart to verify(initializer).accept(any()); (since we have a Supplier and not a Runnable)

[C0urante]

The existing tests use a different (now-deprecated) constructor that accepts a Runnable initializer:

kafka/connect/runtime/src/main/java/org/apache/kafka/connect/util/KafkaBasedLog.java

Lines 125 to 132 in dc18dd9

	public KafkaBasedLog(String topic,
	Map<String, Object> producerConfigs,
	Map<String, Object> consumerConfigs,
	Callback<ConsumerRecord<K, V>> consumedCallback,
	Time time,
	Runnable initializer) {
	this(topic, producerConfigs, consumerConfigs, () -> null, consumedCallback, time, initializer != null ? admin -> initializer.run() : null);
	}

It should be fine to use the new constructor since it's trivially verifiable that the logic in this PR replicates the logic in the older constructor and we don't have to worry about a loss of coverage.

[C0urante]

Thanks @mdedetrich, looks pretty good!

RE the unused throws Exception clauses in tests--I generally prefer to leave those in since there's virtually no cost to having that clause on test methods (it's not like anyone is calling them except for JUnit) and there's the benefit of not having to worry about adding it in later on when some operation we perform like waiting on a CountDownLatch throws, e.g., InterruptedException. It's not a huge deal and not worth blocking a PR on, though, so don't feel obligated to add them back.

[C0urante]

The existing tests use a different (now-deprecated) constructor that accepts a Runnable initializer:

kafka/connect/runtime/src/main/java/org/apache/kafka/connect/util/KafkaBasedLog.java

Lines 125 to 132 in dc18dd9

	public KafkaBasedLog(String topic,
	Map<String, Object> producerConfigs,
	Map<String, Object> consumerConfigs,
	Callback<ConsumerRecord<K, V>> consumedCallback,
	Time time,
	Runnable initializer) {
	this(topic, producerConfigs, consumerConfigs, () -> null, consumedCallback, time, initializer != null ? admin -> initializer.run() : null);
	}

It should be fine to use the new constructor since it's trivially verifiable that the logic in this PR replicates the logic in the older constructor and we don't have to worry about a loss of coverage.

[C0urante]

Don't we lose coverage here? The existing test verifies that the log invokes its createProducer and createConsumer methods exactly once on startup. We should keep those guarantees (especially since those methods have become pluggable over time).

One way to do this is to add tracking logic to the MockedKafkaBasedLog class for how many times its createProducer and createConsumer methods are invoked, and then verify that the number of invocations matches an expected value in the expectStart method.

[C0urante]

Don't we also lose coverage here? The current tests ensure that the producer is closed; we should retain those guarantees.

[C0urante]

This no longer establishes expectations, but verifies them. Should probably rename to verifyStart.

[C0urante]

Thanks for the cleanup 🎉

[C0urante]

I think we can just fold this logic into the method here since it's used consistently throughout each test case that invokes this method:

        assertFalse(store.thread.isAlive());
        assertTrue(consumer.closed());

[C0urante]

This isn't necessary anymore; we don't have to establish no-op expectations for void methods.

It does technically provide some guarantees that producer::flush has been invoked by the end of the test since we'd see an UnnecessaryStubbingException otherwise, but it'd be better to be more explicit about that by adding verify(producer).flush() sometime after the call to store::readToEnd. We probably don't want to put it directly after that call since it's made inside the mock consumer, but it should be fine to add it after the line with readEndFutureCallback::get.

[C0urante]

Same comment; we should replace this with an explicit verify step.

[C0urante]

I think we technically lose coverage here in that we no longer verify that these methods are invoked once and only once. But that should be fine; those guarantees aren't crucial.

[github-actions]

This PR is being marked as stale since it has not had any activity in 90 days. If you would like to keep this PR alive, please ask a committer for review. If the PR has merge conflicts, please update it with the latest from trunk (or appropriate release branch)

If this PR is no longer valid or desired, please feel free to close it. If no activity occurrs in the next 30 days, it will be automatically closed.

[bachmanity1]

Hi @mdedetrich,
I believe I may have accidentally created a duplicate of the work addressed here in #14153. If you plan to complete this PR, would you kindly let me know so I can close the duplicate? Otherwise, could you please close this PR and we can focus our efforts on the new PR?

[mdedetrich]

@bachmanity1 I don't have time to finish this off right now so I will close the PR, thanks for taking the time on this.