You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/11/11 18:38:40 UTC
[GitHub] [kafka] C0urante commented on a diff in pull request #12418: KAFKA-13414: Replace PowerMock/EasyMock with Mockito in connect.storage.KafkaOffsetBackingStoreTest
C0urante commented on code in PR #12418:
URL: https://github.com/apache/kafka/pull/12418#discussion_r1020456794
##########
connect/runtime/src/test/java/org/apache/kafka/connect/storage/KafkaOffsetBackingStoreTest.java:
##########
@@ -30,17 +30,16 @@
import org.apache.kafka.connect.util.Callback;
import org.apache.kafka.connect.util.KafkaBasedLog;
import org.apache.kafka.connect.util.TopicAdmin;
-import org.easymock.Capture;
-import org.easymock.EasyMock;
+import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
-import org.powermock.api.easymock.PowerMock;
-import org.powermock.api.easymock.annotation.Mock;
-import org.powermock.core.classloader.annotations.PowerMockIgnore;
-import org.powermock.core.classloader.annotations.PrepareForTest;
-import org.powermock.modules.junit4.PowerMockRunner;
-import org.powermock.reflect.Whitebox;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Captor;
+import org.mockito.Mock;
+import org.mockito.MockedStatic;
+import org.mockito.Spy;
Review Comment:
The build is failing right now because this import and a few others are unused; can you clean these up?
You can run `./gradlew :connect:runtime:build -x test` if you'd like to make sure that all of the non-test parts of the build pass.
##########
connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaOffsetBackingStore.java:
##########
@@ -214,7 +214,7 @@ public void configure(final WorkerConfig config) {
this.offsetLog = createKafkaBasedLog(topic, producerProps, consumerProps, consumedCallback, topicDescription, adminSupplier);
}
- private KafkaBasedLog<byte[], byte[]> createKafkaBasedLog(String topic, Map<String, Object> producerProps,
+ KafkaBasedLog<byte[], byte[]> createKafkaBasedLog(String topic, Map<String, Object> producerProps,
Review Comment:
```suggestion
// Visible for testing
KafkaBasedLog<byte[], byte[]> createKafkaBasedLog(String topic, Map<String, Object> producerProps,
```
##########
connect/runtime/src/test/java/org/apache/kafka/connect/storage/KafkaOffsetBackingStoreTest.java:
##########
@@ -385,114 +377,64 @@ public void testSetFailure() throws Exception {
store.stop();
- PowerMock.verifyAll();
+ verify(storeLog).stop();
Review Comment:
Can we replace the `try/catch` block above with `assertThrows`?
```java
ExecutionException e = assertThrows(ExecutionException.class, () -> setFuture.get(10000, TimeUnit.MILLISECONDS));
assertNotNull(e.getCause());
assertTrue(e.getCause() instanceof KafkaException);
```
##########
connect/runtime/src/test/java/org/apache/kafka/connect/storage/KafkaOffsetBackingStoreTest.java:
##########
@@ -105,33 +112,58 @@ public class KafkaOffsetBackingStoreTest {
@Mock
KafkaBasedLog<byte[], byte[]> storeLog;
+ @Mock
+ Supplier<TopicAdmin> topicAdminSupplier;
private KafkaOffsetBackingStore store;
- private Capture<String> capturedTopic = EasyMock.newCapture();
- private Capture<Map<String, Object>> capturedProducerProps = EasyMock.newCapture();
- private Capture<Map<String, Object>> capturedConsumerProps = EasyMock.newCapture();
- private Capture<Supplier<TopicAdmin>> capturedAdminSupplier = EasyMock.newCapture();
- private Capture<NewTopic> capturedNewTopic = EasyMock.newCapture();
- private Capture<Callback<ConsumerRecord<byte[], byte[]>>> capturedConsumedCallback = EasyMock.newCapture();
+ private MockedStatic<WorkerConfig> workerConfigMockedStatic;
+
+ @Captor
+ private ArgumentCaptor<String> capturedTopic;
+ @Captor
+ private ArgumentCaptor<Map<String, Object>> capturedProducerProps;
+ @Captor
+ private ArgumentCaptor<Map<String, Object>> capturedConsumerProps;
+ @Captor
+ private ArgumentCaptor<Supplier<TopicAdmin>> capturedAdminSupplier;
+ @Captor
+ private ArgumentCaptor<NewTopic> capturedNewTopic;
+ @Captor
+ private ArgumentCaptor<Callback<ConsumerRecord<byte[], byte[]>>> capturedConsumedCallback;
+ @Captor
+ private ArgumentCaptor<Callback<Void>> storeLogCallbackArgumentCaptor;
+ @Captor
+ private ArgumentCaptor<Callback<Void>> secondGetReadToEndCallback;
Review Comment:
Probably makes sense to inline this one since it's only used in one test.
##########
connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaOffsetBackingStore.java:
##########
@@ -134,7 +134,7 @@ public void configure(final WorkerConfig config) {
}
protected KafkaBasedLog<byte[], byte[]> offsetLog;
- private final HashMap<ByteBuffer, ByteBuffer> data = new HashMap<>();
+ final HashMap<ByteBuffer, ByteBuffer> data = new HashMap<>();
Review Comment:
```suggestion
// Visible for testing
final HashMap<ByteBuffer, ByteBuffer> data = new HashMap<>();
```
##########
connect/runtime/src/test/java/org/apache/kafka/connect/storage/KafkaOffsetBackingStoreTest.java:
##########
@@ -105,33 +112,58 @@ public class KafkaOffsetBackingStoreTest {
@Mock
KafkaBasedLog<byte[], byte[]> storeLog;
+ @Mock
+ Supplier<TopicAdmin> topicAdminSupplier;
private KafkaOffsetBackingStore store;
- private Capture<String> capturedTopic = EasyMock.newCapture();
- private Capture<Map<String, Object>> capturedProducerProps = EasyMock.newCapture();
- private Capture<Map<String, Object>> capturedConsumerProps = EasyMock.newCapture();
- private Capture<Supplier<TopicAdmin>> capturedAdminSupplier = EasyMock.newCapture();
- private Capture<NewTopic> capturedNewTopic = EasyMock.newCapture();
- private Capture<Callback<ConsumerRecord<byte[], byte[]>>> capturedConsumedCallback = EasyMock.newCapture();
+ private MockedStatic<WorkerConfig> workerConfigMockedStatic;
+
+ @Captor
+ private ArgumentCaptor<String> capturedTopic;
+ @Captor
+ private ArgumentCaptor<Map<String, Object>> capturedProducerProps;
+ @Captor
+ private ArgumentCaptor<Map<String, Object>> capturedConsumerProps;
+ @Captor
+ private ArgumentCaptor<Supplier<TopicAdmin>> capturedAdminSupplier;
+ @Captor
+ private ArgumentCaptor<NewTopic> capturedNewTopic;
+ @Captor
+ private ArgumentCaptor<Callback<ConsumerRecord<byte[], byte[]>>> capturedConsumedCallback;
+ @Captor
+ private ArgumentCaptor<Callback<Void>> storeLogCallbackArgumentCaptor;
+ @Captor
+ private ArgumentCaptor<Callback<Void>> secondGetReadToEndCallback;
Review Comment:
Actually, would it be possible to replace uses of it altogether with the `storeLogCallbackArgumentCaptor` field?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org