KAFKA-14132: Replace Easymock & Powermock with Mockito in KafkaBasedLogTest #14153

bachmanity1 · 2023-08-04T16:07:53Z

Replaced Easymock & Powermock with Mockito in KafkaBasedLogTest.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

bachmanity1 · 2023-08-04T16:08:32Z

@C0urante @mjsax @cadonna could you please review this?

divijvaidya

We are tracking migration of this file at https://issues.apache.org/jira/browse/KAFKA-14132 and currently @mdedetrich has volunteered to work on KafkaBasedLogTest. Can you please add a comment on the JIRA and ask @mdedetrich if they have an open PR yet? If they don't, feel assign this test to yourself so that we don't end up duplicating effort across the community.

Thanks!

bachmanity1 · 2023-08-07T16:30:45Z

Hi @divijvaidya

Thanks for letting me know about ticket KAFKA-14132. It turns out that @mdedetrich has already begun working on this issue and has submitted a PR, but he said he doesn't have enough time to complete it and agreed to close it. Thus, I think we can handle this issue in this PR. When you have time could you please review this PR?

bachmanity1 · 2023-08-08T02:37:27Z

I don't know why checks result in This commit cannot be built. It looks like the built failure is not related to this PR or to the connect module, I would appreciate it if anybody could help me resolve it or explain me why it occurs.

divijvaidya · 2023-08-08T09:33:08Z

When you have time could you please review this PR?

I am sorry, I won't have time to review this any time soon. Will circle back in a couple of weeks perhaps. I am tagging a few folks who might be interested in reviewing this: @clolov @yashmayya @shekhar-rajak

yashmayya

Thanks for the PR @bachmanity1! I've left a few comments.

yashmayya · 2023-08-08T09:50:39Z

connect/runtime/src/test/java/org/apache/kafka/connect/util/KafkaBasedLogTest.java

    @Before
    public void setUp() {
-        store = PowerMock.createPartialMock(KafkaBasedLog.class, new String[]{"createConsumer", "createProducer"},
-                TOPIC, PRODUCER_PROPS, CONSUMER_PROPS, consumedCallback, time, initializer);
+        store = spy(new KafkaBasedLog<>(TOPIC, PRODUCER_PROPS, CONSUMER_PROPS, topicAdminSupplier, consumedCallback, time, initializer));


It looks like the only reason we need a partial mock here is to stub in the mock producer / consumer. Since the KafkaBasedLog::createProducer and KafkaBasedLog::createConsumer methods are now protected (looks like they were private when this test was originally written), could we just override those methods to return the mock clients and use a regular instance instead of using a spy / partial mock? I'd prefer it if we could avoid the use of spies / partial mocks as far as possible, since they aren't very OOP-y.

yashmayya · 2023-08-08T09:54:33Z

connect/runtime/src/test/java/org/apache/kafka/connect/util/KafkaBasedLogTest.java

+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+@RunWith(MockitoJUnitRunner.StrictStubs.class)


We should also remove this test from the list here so that it isn't skipped on Java 16 and newer -

kafka/build.gradle

Lines 413 to 423 in 8dec3e6

// Exclude PowerMock tests when running with Java 16 or newer until a version of PowerMock that supports the relevant versions is released

// The relevant issues are https://github.com/powermock/powermock/issues/1094 and https://github.com/powermock/powermock/issues/1099

if (JavaVersion.current().isCompatibleWith(JavaVersion.VERSION_16)) {

testsToExclude.addAll([

// connect tests

"**/DistributedHerderTest.*",

"**/KafkaConfigBackingStoreTest.*",

"**/KafkaBasedLogTest.*", "**/StandaloneHerderTest.*",

"**/WorkerSinkTaskTest.*", "**/WorkerSinkTaskThreadedTest.*"

])

}

yashmayya · 2023-08-08T10:01:19Z

connect/runtime/src/test/java/org/apache/kafka/connect/util/KafkaBasedLogTest.java

-    private static ByteBuffer buffer(String v) {
-        return ByteBuffer.wrap(v.getBytes());
+    private void verifyStartAndStop() {
+        verify(initializer).accept(any());


Now that we're moving to the newer non-deprecated KafkaBasedLog constructor in this test class, could we enhance this verification to also verify that the initializer is called with the admin client from the supplier -

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

Lines 232 to 240 in 8dec3e6

admin = topicAdminSupplier.get(); // may be null

if (admin == null && requireAdminForOffsets) {

throw new ConnectException(

"Must provide a TopicAdmin to KafkaBasedLog when consumer is configured with "

+ ConsumerConfig.ISOLATION_LEVEL_CONFIG + " set to "

+ IsolationLevel.READ_COMMITTED.name().toLowerCase(Locale.ROOT)

);

}

initializer.accept(admin);

yashmayya · 2023-08-08T10:03:16Z

connect/runtime/src/test/java/org/apache/kafka/connect/util/KafkaBasedLogTest.java

@@ -160,18 +156,12 @@ public void testStartStop() throws Exception {
        assertEquals(CONSUMER_ASSIGNMENT, consumer.assignment());

        store.stop();
-
-        assertFalse(Whitebox.<Thread>getInternalState(store, "thread").isAlive());


Aren't we losing this test coverage? Can we bump up the visibility of the thread instance variable to package-private (with a comment stating that it is being made visible for testing) and retain this coverage?

yashmayya · 2023-08-08T10:07:38Z

connect/runtime/src/test/java/org/apache/kafka/connect/util/KafkaBasedLogTest.java

-        Capture<org.apache.kafka.clients.producer.Callback> callback1 = EasyMock.newCapture();
-        EasyMock.expect(producer.send(EasyMock.eq(tp1Record), EasyMock.capture(callback1))).andReturn(tp1Future);
-
-        // Producer flushes when read to log end is called


Can we retain this clarifying comment (we can add it above the producer flush verification)?

yashmayya · 2023-08-08T10:09:57Z

connect/runtime/src/test/java/org/apache/kafka/connect/util/KafkaBasedLogTest.java

-        EasyMock.expectLastCall().times(1);
-
-        expectProducerAndConsumerCreate();
+        store = spy(new KafkaBasedLog<>(TOPIC, PRODUCER_PROPS, CONSUMER_PROPS, adminSupplier, consumedCallback, time, initializer));


Do we still need this separate setupWithAdmin method now that we're setting up the KafkaBasedLog store with a topic admin supplier even in the regular setUp method?

yes, I think we can merge setupWithAdmin into setUp.

yashmayya · 2023-08-08T10:11:31Z

connect/runtime/src/test/java/org/apache/kafka/connect/util/KafkaBasedLogTest.java

+        when(admin.retryEndOffsets(eq(tps), any(), anyLong())).thenReturn(endOffsets);
+        when(admin.endOffsets(eq(tps))).thenReturn(endOffsets);


We're losing out on the existing coverage here since we're no longer ensuring that these methods are called one time only. Can we add an explicit verification for this at the end of the test?

yashmayya · 2023-08-08T10:24:05Z

I don't know why checks result in This commit cannot be built. It looks like the built failure is not related to this PR or to the connect module, I would appreciate it if anybody could help me resolve it or explain me why it occurs.

I checked the build and the failures are from unrelated tests that are known to be flaky so you can just ignore them.

bachmanity1 · 2023-08-08T11:53:47Z

Hi @yashmayya, thank you for your review! I've addressed the questions raised by you. Can you have one more look?

yashmayya

Thanks @bachmanity1, this is looking really good. I just had a few more comments.

yashmayya · 2023-08-08T12:22:04Z

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

@@ -103,7 +103,7 @@ public class KafkaBasedLog<K, V> {
    private Optional<Producer<K, V>> producer;
    private TopicAdmin admin;

-    private Thread thread;
+    Thread thread;


Suggested change

Thread thread;

// Visible for testing

Thread thread;

nit

yashmayya · 2023-08-08T12:26:17Z

connect/runtime/src/test/java/org/apache/kafka/connect/util/KafkaBasedLogTest.java

+    private class MockedKafkaBasedLog extends KafkaBasedLog<String, String> {
+        public MockedKafkaBasedLog(String topic,
+                                   Map<String, Object> producerConfigs,
+                                   Map<String, Object> consumerConfigs,
+                                   Supplier<TopicAdmin> topicAdminSupplier,
+                                   Callback<ConsumerRecord<String, String>> consumedCallback,
+                                   Time time,
+                                   Consumer<TopicAdmin> initializer) {
+            super(topic, producerConfigs, consumerConfigs, topicAdminSupplier, consumedCallback, time, initializer);
+        }
+
+        @Override
+        protected KafkaProducer<String, String> createProducer() {
+            return producer;
+        }
+
+        @Override
+        protected MockConsumer<String, String> createConsumer() {
+            return consumer;
+        }
+    }


Can we use an anonymous inner class in the setUp method instead? I think that'll look a lot cleaner.

yashmayya · 2023-08-08T12:29:17Z

connect/runtime/src/test/java/org/apache/kafka/connect/util/KafkaBasedLogTest.java

    @Mock
    private KafkaProducer<String, String> producer;
-    private MockConsumer<String, String> consumer;
-    @Mock


I'm curious, why was this change required - 2c7d2e3?

admin field needs to be mocked only in methods where previously setupWithAdmin was used, if we mock it everywhere then some tests don't pass because they expect that admin is null.

Ah yeah, that makes sense, the other methods are expecting the consumer to be used to retrieve the end offsets instead of the admin.

yashmayya · 2023-08-08T12:29:53Z

connect/runtime/src/test/java/org/apache/kafka/connect/util/KafkaBasedLogTest.java

    }

    @Test
    public void testGetOffsetsConsumerErrorOnReadToEnd() throws Exception {
-        expectStart();
-
-        // Producer flushes when read to log end is called


Same comment as above (https://github.com/apache/kafka/pull/14153/files#r1286903819)

bachmanity1 · 2023-08-08T15:09:42Z

@yashmayya I've applied your suggestions.

yashmayya

Thanks @bachmanity1, LGTM!

divijvaidya · 2023-08-09T11:08:42Z

Thank you for reviewing @yashmayya. The CI is still flaky with large number of failures and I am restarting it.

bachmanity1 · 2023-08-10T12:38:12Z

It looks like test failures are not related to this PR.

Build / JDK 17 and Scala 2.13 / testBalancePartitionLeaders() – org.apache.kafka.controller.QuorumControllerTest
Build / JDK 11 and Scala 2.13 / testSecondaryRefreshAfterElapsedDelay() – org.apache.kafka.common.security.oauthbearer.internals.secured.RefreshingHttpsJwksTest
Build / JDK 11 and Scala 2.13 / testBalancePartitionLeaders() – org.apache.kafka.controller.QuorumControllerTest
Build / JDK 8 and Scala 2.12 / testSyncTopicConfigs() – org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationBaseTest
Build / JDK 8 and Scala 2.12 / testCreatePartitionsUseProvidedForwardingAdmin() – org.apache.kafka.connect.mirror.integration.MirrorConnectorsWithCustomForwardingAdminIntegrationTest
Build / JDK 8 and Scala 2.12 / testRackAwareRangeAssignor() – integration.kafka.server.FetchFromFollowerIntegrationTest
Build / JDK 8 and Scala 2.12 / testBalancePartitionLeaders() – org.apache.kafka.controller.QuorumControllerTest
Build / JDK 8 and Scala 2.12 / testHighAvailabilityTaskAssignorLargeNumConsumers – org.apache.kafka.streams.processor.internals.StreamsAssignmentScaleTest
Build / JDK 11 and Scala 2.13 / testOffsetTranslationBehindReplicationFlow() – org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationExactlyOnceTest
Build / JDK 8 and Scala 2.12 / testOffsetTranslationBehindReplicationFlow() – org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationExactlyOnceTest
Build / JDK 8 and Scala 2.12 / testOffsetTranslationBehindReplicationFlow() – org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationExactlyOnceTest

…ogTest (apache#14153) Reviewers: Yash Mayya <yash.mayya@gmail.com>, Divij Vaidya <diviv@amazon.com>

bachmanity1 added 4 commits August 5, 2023 01:05

remove easymock & powermock

4522967

almost done

675731e

only whitebox references left

0f194a0

just remove whitebox

f52125a

divijvaidya added the tests Test fixes (including flaky tests) label Aug 7, 2023

divijvaidya reviewed Aug 7, 2023

View reviewed changes

bachmanity1 mentioned this pull request Aug 7, 2023

KAFKA-14132; Replace EasyMock with Mockito in KafkaBasedLogTest #12781

Closed

bachmanity1 changed the title ~~KAFKA-7438: Replace Easymock & Powermock with Mockito in KafkaBasedLogTest~~ KAFKA-14132: Replace Easymock & Powermock with Mockito in KafkaBasedLogTest Aug 7, 2023

bachmanity1 added 2 commits August 8, 2023 01:06

fix build errors

9bb9661

reorder imports

c504dab

C0urante added the connect label Aug 7, 2023

yashmayya reviewed Aug 8, 2023

View reviewed changes

remove KafkaBasedLogTest from build.gradle

000891d

bachmanity1 added 4 commits August 8, 2023 19:57

make thread package private

d42a3e0

apply review suggestions

8a1ce9d

mock admin when needed only

2c7d2e3

make sure that TopicAdmin methods are called one time only

c91f102

yashmayya reviewed Aug 8, 2023

View reviewed changes

apply pr suggestions

65c33d4

yashmayya approved these changes Aug 9, 2023

View reviewed changes

yashmayya requested review from mimaison and C0urante August 9, 2023 10:58

divijvaidya approved these changes Aug 11, 2023

View reviewed changes

divijvaidya merged commit f137da0 into apache:trunk Aug 11, 2023
1 check failed

bachmanity1 deleted the replace-easymock-powermock branch August 11, 2023 08:54

jeqo pushed a commit to aiven/kafka that referenced this pull request Aug 15, 2023

KAFKA-14132: Replace Easymock & Powermock with Mockito in KafkaBasedL…

8c4d4d3

…ogTest (apache#14153) Reviewers: Yash Mayya <yash.mayya@gmail.com>, Divij Vaidya <diviv@amazon.com>

jeqo pushed a commit to jeqo/kafka that referenced this pull request Aug 15, 2023

KAFKA-14132: Replace Easymock & Powermock with Mockito in KafkaBasedL…

bbc2fa4

…ogTest (apache#14153) Reviewers: Yash Mayya <yash.mayya@gmail.com>, Divij Vaidya <diviv@amazon.com>

jeqo pushed a commit to jeqo/kafka that referenced this pull request Aug 15, 2023

KAFKA-14132: Replace Easymock & Powermock with Mockito in KafkaBasedL…

9ac5408

…ogTest (apache#14153) Reviewers: Yash Mayya <yash.mayya@gmail.com>, Divij Vaidya <diviv@amazon.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-14132: Replace Easymock & Powermock with Mockito in KafkaBasedLogTest #14153

KAFKA-14132: Replace Easymock & Powermock with Mockito in KafkaBasedLogTest #14153

bachmanity1 commented Aug 4, 2023

bachmanity1 commented Aug 4, 2023

divijvaidya left a comment

bachmanity1 commented Aug 7, 2023

bachmanity1 commented Aug 8, 2023 •

edited

divijvaidya commented Aug 8, 2023

yashmayya left a comment

yashmayya Aug 8, 2023

yashmayya Aug 8, 2023

yashmayya Aug 8, 2023

yashmayya Aug 8, 2023

yashmayya Aug 8, 2023

yashmayya Aug 8, 2023

bachmanity1 Aug 8, 2023

yashmayya Aug 8, 2023

yashmayya commented Aug 8, 2023

bachmanity1 commented Aug 8, 2023

yashmayya left a comment

yashmayya Aug 8, 2023

yashmayya Aug 8, 2023

yashmayya Aug 8, 2023

bachmanity1 Aug 8, 2023

yashmayya Aug 9, 2023

yashmayya Aug 8, 2023

bachmanity1 commented Aug 8, 2023

yashmayya left a comment

divijvaidya commented Aug 9, 2023

bachmanity1 commented Aug 10, 2023

	// Exclude PowerMock tests when running with Java 16 or newer until a version of PowerMock that supports the relevant versions is released
	// The relevant issues are https://github.com/powermock/powermock/issues/1094 and https://github.com/powermock/powermock/issues/1099
	if (JavaVersion.current().isCompatibleWith(JavaVersion.VERSION_16)) {
	testsToExclude.addAll([
	// connect tests
	"*/DistributedHerderTest.",
	"*/KafkaConfigBackingStoreTest.",
	"*/KafkaBasedLogTest.", "*/StandaloneHerderTest.",
	"*/WorkerSinkTaskTest.", "*/WorkerSinkTaskThreadedTest."
	])
	}

	admin = topicAdminSupplier.get(); // may be null
	if (admin == null && requireAdminForOffsets) {
	throw new ConnectException(
	"Must provide a TopicAdmin to KafkaBasedLog when consumer is configured with "
	+ ConsumerConfig.ISOLATION_LEVEL_CONFIG + " set to "
	+ IsolationLevel.READ_COMMITTED.name().toLowerCase(Locale.ROOT)
	);
	}
	initializer.accept(admin);

		when(admin.retryEndOffsets(eq(tps), any(), anyLong())).thenReturn(endOffsets);
		when(admin.endOffsets(eq(tps))).thenReturn(endOffsets);

KAFKA-14132: Replace Easymock & Powermock with Mockito in KafkaBasedLogTest #14153

KAFKA-14132: Replace Easymock & Powermock with Mockito in KafkaBasedLogTest #14153

Conversation

bachmanity1 commented Aug 4, 2023

Committer Checklist (excluded from commit message)

bachmanity1 commented Aug 4, 2023

divijvaidya left a comment

Choose a reason for hiding this comment

bachmanity1 commented Aug 7, 2023

bachmanity1 commented Aug 8, 2023 • edited

divijvaidya commented Aug 8, 2023

yashmayya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yashmayya commented Aug 8, 2023

bachmanity1 commented Aug 8, 2023

yashmayya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bachmanity1 commented Aug 8, 2023

yashmayya left a comment

Choose a reason for hiding this comment

divijvaidya commented Aug 9, 2023

bachmanity1 commented Aug 10, 2023

bachmanity1 commented Aug 8, 2023 •

edited