KAFKA-13414: Replace PowerMock/EasyMock with Mockito in connect.storage.KafkaOffsetBackingStoreTest #12418

clolov · 2022-07-18T15:59:36Z

Partially addressing https://issues.apache.org/jira/browse/KAFKA-13414. Other test files will be updated in other pull requests.

clolov · 2022-07-18T16:00:18Z

@mimaison I believe you might be best suited to review this pull request :)

C0urante

Thanks @clolov, this is looking pretty good! I have a few comments; a couple are fairly high-level so if you have any questions and/or want to clarify anything before working on the next round, let me know.

connect/runtime/src/test/java/org/apache/kafka/connect/storage/KafkaOffsetBackingStoreTest.java

C0urante · 2022-08-01T21:24:03Z

connect/runtime/src/test/java/org/apache/kafka/connect/storage/KafkaOffsetBackingStoreTest.java

+        doAnswer(invocation -> {
+            // Second get() should get the produced data and return the new values
            capturedConsumedCallback.getValue().onCompletion(null,
-                new ConsumerRecord<>(TOPIC, 0, 0, 0L, TimestampType.CREATE_TIME, 0, 0, TP0_KEY.array(), TP0_VALUE.array(),
-                    new RecordHeaders(), Optional.empty()));
+                    new ConsumerRecord<>(TOPIC, 0, 0, 0L, TimestampType.CREATE_TIME, 0, 0, TP0_KEY.array(), TP0_VALUE.array(),
+                            new RecordHeaders(), Optional.empty()));
            capturedConsumedCallback.getValue().onCompletion(null,
-                new ConsumerRecord<>(TOPIC, 1, 0, 0L, TimestampType.CREATE_TIME, 0, 0, TP1_KEY.array(), TP1_VALUE.array(),
-                    new RecordHeaders(), Optional.empty()));
-            secondGetReadToEndCallback.getValue().onCompletion(null, null);
+                    new ConsumerRecord<>(TOPIC, 1, 0, 0L, TimestampType.CREATE_TIME, 0, 0, TP1_KEY.array(), TP1_VALUE.array(),
+                            new RecordHeaders(), Optional.empty()));
+            storeLogCallbackArgumentCaptor.getValue().onCompletion(null, null);
            return null;
-        });
-
-        // Third get() should pick up data produced by someone else and return those values
-        final Capture<Callback<Void>> thirdGetReadToEndCallback = EasyMock.newCapture();
-        storeLog.readToEnd(EasyMock.capture(thirdGetReadToEndCallback));
-        PowerMock.expectLastCall().andAnswer(() -> {
+        }).doAnswer(invocation -> {
+            // Third get() should pick up data produced by someone else and return those values
            capturedConsumedCallback.getValue().onCompletion(null,
-                new ConsumerRecord<>(TOPIC, 0, 1, 0L, TimestampType.CREATE_TIME, 0, 0, TP0_KEY.array(), TP0_VALUE_NEW.array(),
-                    new RecordHeaders(), Optional.empty()));
+                    new ConsumerRecord<>(TOPIC, 0, 1, 0L, TimestampType.CREATE_TIME, 0, 0, TP0_KEY.array(), TP0_VALUE_NEW.array(),
+                            new RecordHeaders(), Optional.empty()));
            capturedConsumedCallback.getValue().onCompletion(null,
-                new ConsumerRecord<>(TOPIC, 1, 1, 0L, TimestampType.CREATE_TIME, 0, 0, TP1_KEY.array(), TP1_VALUE_NEW.array(),
-                    new RecordHeaders(), Optional.empty()));
-            thirdGetReadToEndCallback.getValue().onCompletion(null, null);
+                    new ConsumerRecord<>(TOPIC, 1, 1, 0L, TimestampType.CREATE_TIME, 0, 0, TP1_KEY.array(), TP1_VALUE_NEW.array(),
+                            new RecordHeaders(), Optional.empty()));
+            storeLogCallbackArgumentCaptor.getValue().onCompletion(null, null);
            return null;
-        });
+        }).when(storeLog).readToEnd(storeLogCallbackArgumentCaptor.capture());


We don't have to set up the expectations for all three calls to KafkaOffsetBackingStore::get at once. We can set up the expectation for each right before we invoke the method.

This has a few advantages:

Cleanliness (the chained doAnswer method invocations are a little unintuitive)

Readability (placing expectation-setting code closer to execution code makes it easier to see how the two are related)

In general, I think we should try to follow Mockito idioms more closely with these tests by establishing expectations as late as possible in tests, instead of having a monolithic expectation-setting section at the beginning of each test case.

I hope I have addressed this in the upcoming commit.

clolov · 2022-08-03T16:22:30Z

Thank you for the review @C0urante! I will have a look and aim to provide answers (+ new commits) in the next couple of days.

C0urante

Thanks @clolov, this is looking pretty good! Apologies for the delay; I should be more responsive now.

connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaOffsetBackingStore.java

connect/runtime/src/test/java/org/apache/kafka/connect/storage/KafkaOffsetBackingStoreTest.java

C0urante · 2022-11-07T15:11:35Z

@clolov are you planning on revisiting this one?

clolov · 2022-11-07T16:02:59Z

Hello, yes I am, it is the next one on my list :)

C0urante

Thanks @clolov! Looking great; once this is rebased and the remaining comments are addressed it'll probably be ready to merge.

connect/runtime/src/test/java/org/apache/kafka/connect/storage/KafkaOffsetBackingStoreTest.java

connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaOffsetBackingStore.java

connect/runtime/src/test/java/org/apache/kafka/connect/storage/KafkaOffsetBackingStoreTest.java

clolov · 2022-11-15T11:48:19Z

Thank you for the new comments @C0urante, I will aim to address them today!

C0urante

LGTM, thanks @clolov! Will merge pending CI build.

C0urante · 2022-11-15T21:30:36Z

The single test failure is unrelated; merging...

clolov · 2022-11-16T10:30:10Z

Thank you for the quick turnaround on the review @C0urante! I will aim to open a couple more pull requests in the upcoming days for Connect with respect to the Mockito migration and tag you there.

…ge.KafkaOffsetBackingStoreTest (apache#12418) Reviewers: Chris Egerton <chrise@aiven.io>

C0urante added the connect label Jul 26, 2022

C0urante self-requested a review July 30, 2022 21:34

C0urante reviewed Aug 2, 2022

View reviewed changes

C0urante reviewed Aug 22, 2022

View reviewed changes

clolov force-pushed the kafka-12220 branch from e506846 to 60fe5bc Compare November 7, 2022 17:43

C0urante reviewed Nov 11, 2022

View reviewed changes

clolov added 2 commits November 15, 2022 15:54

Use Mockito in KafkaOffsetBackingStoreTest

b0931bc

Address review comments

199bcfb

clolov force-pushed the kafka-12220 branch from 60fe5bc to 199bcfb Compare November 15, 2022 17:01

C0urante approved these changes Nov 15, 2022

View reviewed changes

C0urante merged commit 1894856 into apache:trunk Nov 15, 2022

guozhangwang pushed a commit to guozhangwang/kafka that referenced this pull request Jan 25, 2023

KAFKA-13414: Replace PowerMock/EasyMock with Mockito in connect.stora…

592bcd8

…ge.KafkaOffsetBackingStoreTest (apache#12418) Reviewers: Chris Egerton <chrise@aiven.io>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-13414: Replace PowerMock/EasyMock with Mockito in connect.storage.KafkaOffsetBackingStoreTest #12418

KAFKA-13414: Replace PowerMock/EasyMock with Mockito in connect.storage.KafkaOffsetBackingStoreTest #12418

clolov commented Jul 18, 2022

clolov commented Jul 18, 2022

C0urante left a comment

C0urante Aug 1, 2022

clolov Aug 4, 2022

clolov commented Aug 3, 2022

C0urante left a comment

C0urante commented Nov 7, 2022 •

edited

Loading

clolov commented Nov 7, 2022

C0urante left a comment

clolov commented Nov 15, 2022

C0urante left a comment

C0urante commented Nov 15, 2022

clolov commented Nov 16, 2022

KAFKA-13414: Replace PowerMock/EasyMock with Mockito in connect.storage.KafkaOffsetBackingStoreTest #12418

KAFKA-13414: Replace PowerMock/EasyMock with Mockito in connect.storage.KafkaOffsetBackingStoreTest #12418

Conversation

clolov commented Jul 18, 2022

clolov commented Jul 18, 2022

C0urante left a comment

Choose a reason for hiding this comment

C0urante Aug 1, 2022

Choose a reason for hiding this comment

clolov Aug 4, 2022

Choose a reason for hiding this comment

clolov commented Aug 3, 2022

C0urante left a comment

Choose a reason for hiding this comment

C0urante commented Nov 7, 2022 • edited Loading

clolov commented Nov 7, 2022

C0urante left a comment

Choose a reason for hiding this comment

clolov commented Nov 15, 2022

C0urante left a comment

Choose a reason for hiding this comment

C0urante commented Nov 15, 2022

clolov commented Nov 16, 2022

C0urante commented Nov 7, 2022 •

edited

Loading