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/09/07 08:19:29 UTC
[GitHub] [kafka] cadonna commented on a diff in pull request #12492: KAFKA-14133: Replace EasyMock with Mockito in streams tests
cadonna commented on code in PR #12492:
URL: https://github.com/apache/kafka/pull/12492#discussion_r964500209
##########
streams/src/test/java/org/apache/kafka/streams/kstream/internals/KStreamFlatTransformTest.java:
##########
@@ -68,71 +76,59 @@ public void shouldTransformInputRecordToMultipleOutputRecords() {
KeyValue.pair(2, 20),
KeyValue.pair(3, 30),
KeyValue.pair(4, 40));
+
processor.init(context);
- EasyMock.reset(transformer);
- EasyMock.expect(transformer.transform(inputKey, inputValue)).andReturn(outputRecords);
- for (final KeyValue<Integer, Integer> outputRecord : outputRecords) {
- context.forward(new Record<>(outputRecord.key, outputRecord.value, 0L));
- }
- replayAll();
+ when(transformer.transform(inputKey, inputValue)).thenReturn(outputRecords);
processor.process(new Record<>(inputKey, inputValue, 0L));
- verifyAll();
+ for (final KeyValue<Integer, Integer> outputRecord : outputRecords) {
+ inOrder.verify(context).forward(new Record<>(outputRecord.key, outputRecord.value, 0L));
+ }
}
@Test
public void shouldAllowEmptyListAsResultOfTransform() {
processor.init(context);
- EasyMock.reset(transformer);
- EasyMock.expect(transformer.transform(inputKey, inputValue))
- .andReturn(Collections.emptyList());
- replayAll();
+ when(transformer.transform(inputKey, inputValue)).thenReturn(Collections.emptyList());
processor.process(new Record<>(inputKey, inputValue, 0L));
- verifyAll();
+ inOrder.verify(context, never()).forward(ArgumentMatchers.<Record<Integer, Integer>>any());
}
@Test
public void shouldAllowNullAsResultOfTransform() {
processor.init(context);
- EasyMock.reset(transformer);
- EasyMock.expect(transformer.transform(inputKey, inputValue))
- .andReturn(null);
- replayAll();
+ when(transformer.transform(inputKey, inputValue)).thenReturn(null);
processor.process(new Record<>(inputKey, inputValue, 0L));
- verifyAll();
+ inOrder.verify(context, never()).forward(ArgumentMatchers.<Record<Integer, Integer>>any());
}
@Test
public void shouldCloseFlatTransformProcessor() {
- transformer.close();
- replayAll();
-
processor.close();
- verifyAll();
+ verify(transformer).close();
}
@Test
public void shouldGetFlatTransformProcessor() {
+ @SuppressWarnings("unchecked")
final TransformerSupplier<Number, Number, Iterable<KeyValue<Integer, Integer>>> transformerSupplier =
mock(TransformerSupplier.class);
Review Comment:
nit: You could define the mock as a instance field and annotate it with `@Mock` to avoid suppressing the warning. That is a bit unfortunate in Mockito that there is no other way to set type parameters for mocks.
##########
streams/src/test/java/org/apache/kafka/streams/kstream/internals/KStreamFlatTransformTest.java:
##########
@@ -68,71 +76,59 @@ public void shouldTransformInputRecordToMultipleOutputRecords() {
KeyValue.pair(2, 20),
KeyValue.pair(3, 30),
KeyValue.pair(4, 40));
+
processor.init(context);
- EasyMock.reset(transformer);
- EasyMock.expect(transformer.transform(inputKey, inputValue)).andReturn(outputRecords);
- for (final KeyValue<Integer, Integer> outputRecord : outputRecords) {
- context.forward(new Record<>(outputRecord.key, outputRecord.value, 0L));
- }
- replayAll();
+ when(transformer.transform(inputKey, inputValue)).thenReturn(outputRecords);
processor.process(new Record<>(inputKey, inputValue, 0L));
- verifyAll();
+ for (final KeyValue<Integer, Integer> outputRecord : outputRecords) {
+ inOrder.verify(context).forward(new Record<>(outputRecord.key, outputRecord.value, 0L));
+ }
}
@Test
public void shouldAllowEmptyListAsResultOfTransform() {
processor.init(context);
- EasyMock.reset(transformer);
- EasyMock.expect(transformer.transform(inputKey, inputValue))
- .andReturn(Collections.emptyList());
- replayAll();
+ when(transformer.transform(inputKey, inputValue)).thenReturn(Collections.emptyList());
processor.process(new Record<>(inputKey, inputValue, 0L));
- verifyAll();
+ inOrder.verify(context, never()).forward(ArgumentMatchers.<Record<Integer, Integer>>any());
Review Comment:
nit: Do you actually need in order here since it is just one verification?
##########
streams/src/test/java/org/apache/kafka/streams/kstream/internals/KStreamFlatTransformValuesTest.java:
##########
@@ -68,69 +76,59 @@ public void shouldTransformInputRecordToMultipleOutputValues() {
"Hello",
"Blue",
"Planet");
+
processor.init(context);
- EasyMock.reset(valueTransformer);
- EasyMock.expect(valueTransformer.transform(inputKey, inputValue)).andReturn(outputValues);
- for (final String outputValue : outputValues) {
- context.forward(new Record<>(inputKey, outputValue, 0L));
- }
- replayAll();
+ when(valueTransformer.transform(inputKey, inputValue)).thenReturn(outputValues);
processor.process(new Record<>(inputKey, inputValue, 0L));
- verifyAll();
+ for (final String outputValue : outputValues) {
+ inOrder.verify(context).forward(new Record<>(inputKey, outputValue, 0L));
+ }
}
@Test
public void shouldEmitNoRecordIfTransformReturnsEmptyList() {
processor.init(context);
- EasyMock.reset(valueTransformer);
- EasyMock.expect(valueTransformer.transform(inputKey, inputValue)).andReturn(Collections.emptyList());
- replayAll();
+ when(valueTransformer.transform(inputKey, inputValue)).thenReturn(Collections.emptyList());
processor.process(new Record<>(inputKey, inputValue, 0L));
- verifyAll();
+ inOrder.verify(context, never()).forward(ArgumentMatchers.<Record<Integer, String>>any());
Review Comment:
nit: see my comment above about in order with just one verification. This comment applies also to other parts of this PR.
##########
streams/src/test/java/org/apache/kafka/streams/kstream/internals/KStreamFlatTransformTest.java:
##########
@@ -68,71 +76,59 @@ public void shouldTransformInputRecordToMultipleOutputRecords() {
KeyValue.pair(2, 20),
KeyValue.pair(3, 30),
KeyValue.pair(4, 40));
+
Review Comment:
nit: I would remove this line. I usually try to have a block for the setup, a block for the call under test, and a block for the verifications. Does not always work, but often.
This comment applies also to other places in this PR.
##########
streams/src/test/java/org/apache/kafka/streams/kstream/internals/KStreamFlatTransformTest.java:
##########
@@ -68,71 +76,59 @@ public void shouldTransformInputRecordToMultipleOutputRecords() {
KeyValue.pair(2, 20),
KeyValue.pair(3, 30),
KeyValue.pair(4, 40));
+
processor.init(context);
- EasyMock.reset(transformer);
- EasyMock.expect(transformer.transform(inputKey, inputValue)).andReturn(outputRecords);
- for (final KeyValue<Integer, Integer> outputRecord : outputRecords) {
- context.forward(new Record<>(outputRecord.key, outputRecord.value, 0L));
- }
- replayAll();
+ when(transformer.transform(inputKey, inputValue)).thenReturn(outputRecords);
processor.process(new Record<>(inputKey, inputValue, 0L));
- verifyAll();
+ for (final KeyValue<Integer, Integer> outputRecord : outputRecords) {
+ inOrder.verify(context).forward(new Record<>(outputRecord.key, outputRecord.value, 0L));
+ }
}
@Test
public void shouldAllowEmptyListAsResultOfTransform() {
processor.init(context);
- EasyMock.reset(transformer);
- EasyMock.expect(transformer.transform(inputKey, inputValue))
- .andReturn(Collections.emptyList());
- replayAll();
+ when(transformer.transform(inputKey, inputValue)).thenReturn(Collections.emptyList());
processor.process(new Record<>(inputKey, inputValue, 0L));
- verifyAll();
+ inOrder.verify(context, never()).forward(ArgumentMatchers.<Record<Integer, Integer>>any());
}
@Test
public void shouldAllowNullAsResultOfTransform() {
processor.init(context);
- EasyMock.reset(transformer);
- EasyMock.expect(transformer.transform(inputKey, inputValue))
- .andReturn(null);
- replayAll();
+ when(transformer.transform(inputKey, inputValue)).thenReturn(null);
processor.process(new Record<>(inputKey, inputValue, 0L));
- verifyAll();
+ inOrder.verify(context, never()).forward(ArgumentMatchers.<Record<Integer, Integer>>any());
Review Comment:
See my comment above.
##########
streams/src/test/java/org/apache/kafka/streams/kstream/internals/TransformerSupplierAdapterTest.java:
##########
@@ -24,117 +24,104 @@
import org.apache.kafka.streams.kstream.TransformerSupplier;
import org.apache.kafka.streams.processor.ProcessorContext;
import org.apache.kafka.streams.state.StoreBuilder;
-import org.easymock.EasyMock;
-import org.easymock.EasyMockSupport;
-import org.junit.Before;
import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
import static org.hamcrest.core.IsEqual.equalTo;
import static org.hamcrest.core.IsSame.sameInstance;
import static org.hamcrest.core.IsNot.not;
import static org.hamcrest.MatcherAssert.assertThat;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
-public class TransformerSupplierAdapterTest extends EasyMockSupport {
+@RunWith(MockitoJUnitRunner.StrictStubs.class)
+public class TransformerSupplierAdapterTest {
+ @Mock
private ProcessorContext context;
+ @Mock
private Transformer<String, String, KeyValue<Integer, Integer>> transformer;
+ @Mock
private TransformerSupplier<String, String, KeyValue<Integer, Integer>> transformerSupplier;
+ @Mock
private Set<StoreBuilder<?>> stores;
final String key = "Hello";
final String value = "World";
- @Before
- public void before() {
- context = mock(ProcessorContext.class);
- transformer = mock(Transformer.class);
- transformerSupplier = mock(TransformerSupplier.class);
- stores = mock(Set.class);
- }
-
@Test
public void shouldCallInitOfAdapteeTransformer() {
- EasyMock.expect(transformerSupplier.get()).andReturn(transformer);
- transformer.init(context);
- replayAll();
+ when(transformerSupplier.get()).thenReturn(transformer);
final TransformerSupplierAdapter<String, String, Integer, Integer> adapter =
new TransformerSupplierAdapter<>(transformerSupplier);
final Transformer<String, String, Iterable<KeyValue<Integer, Integer>>> adaptedTransformer = adapter.get();
adaptedTransformer.init(context);
- verifyAll();
+ verify(transformer).init(context);
}
@Test
public void shouldCallCloseOfAdapteeTransformer() {
- EasyMock.expect(transformerSupplier.get()).andReturn(transformer);
- transformer.close();
- replayAll();
+ when(transformerSupplier.get()).thenReturn(transformer);
final TransformerSupplierAdapter<String, String, Integer, Integer> adapter =
new TransformerSupplierAdapter<>(transformerSupplier);
final Transformer<String, String, Iterable<KeyValue<Integer, Integer>>> adaptedTransformer = adapter.get();
adaptedTransformer.close();
- verifyAll();
+ verify(transformer).close();
}
@Test
public void shouldCallStoresOfAdapteeTransformerSupplier() {
- EasyMock.expect(transformerSupplier.stores()).andReturn(stores);
- replayAll();
+ when(transformerSupplier.stores()).thenReturn(stores);
final TransformerSupplierAdapter<String, String, Integer, Integer> adapter =
new TransformerSupplierAdapter<>(transformerSupplier);
adapter.stores();
- verifyAll();
}
@Test
public void shouldCallTransformOfAdapteeTransformerAndReturnSingletonIterable() {
- EasyMock.expect(transformerSupplier.get()).andReturn(transformer);
- EasyMock.expect(transformer.transform(key, value)).andReturn(KeyValue.pair(0, 1));
- replayAll();
+ when(transformerSupplier.get()).thenReturn(transformer);
+ when(transformer.transform(key, value)).thenReturn(KeyValue.pair(0, 1));
final TransformerSupplierAdapter<String, String, Integer, Integer> adapter =
new TransformerSupplierAdapter<>(transformerSupplier);
final Transformer<String, String, Iterable<KeyValue<Integer, Integer>>> adaptedTransformer = adapter.get();
final Iterator<KeyValue<Integer, Integer>> iterator = adaptedTransformer.transform(key, value).iterator();
- verifyAll();
assertThat(iterator.hasNext(), equalTo(true));
iterator.next();
assertThat(iterator.hasNext(), equalTo(false));
}
@Test
public void shouldCallTransformOfAdapteeTransformerAndReturnEmptyIterable() {
- EasyMock.expect(transformerSupplier.get()).andReturn(transformer);
- EasyMock.expect(transformer.transform(key, value)).andReturn(null);
- replayAll();
+ when(transformerSupplier.get()).thenReturn(transformer);
+ when(transformer.transform(key, value)).thenReturn(null);
final TransformerSupplierAdapter<String, String, Integer, Integer> adapter =
new TransformerSupplierAdapter<>(transformerSupplier);
final Transformer<String, String, Iterable<KeyValue<Integer, Integer>>> adaptedTransformer = adapter.get();
final Iterator<KeyValue<Integer, Integer>> iterator = adaptedTransformer.transform(key, value).iterator();
- verifyAll();
assertThat(iterator.hasNext(), equalTo(false));
}
@Test
public void shouldAlwaysGetNewAdapterTransformer() {
+ @SuppressWarnings("unchecked")
final Transformer<String, String, KeyValue<Integer, Integer>> transformer1 = mock(Transformer.class);
+ @SuppressWarnings("unchecked")
final Transformer<String, String, KeyValue<Integer, Integer>> transformer2 = mock(Transformer.class);
+ @SuppressWarnings("unchecked")
final Transformer<String, String, KeyValue<Integer, Integer>> transformer3 = mock(Transformer.class);
Review Comment:
nit: see my comment above about avoiding suppressing warnings. Maybe you can just rename the global `transformer` to `transformer1` and then move `transformer2` and `transformer3` to the object fields.
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/ClientUtilsTest.java:
##########
@@ -108,75 +110,73 @@ public class ClientUtilsTest {
@Test
public void fetchCommittedOffsetsShouldRethrowKafkaExceptionAsStreamsException() {
- final Consumer<byte[], byte[]> consumer = EasyMock.createMock(Consumer.class);
- expect(consumer.committed(PARTITIONS)).andThrow(new KafkaException());
- replay(consumer);
+ @SuppressWarnings("unchecked")
+ final Consumer<byte[], byte[]> consumer = mock(Consumer.class);
Review Comment:
nit: I think here it makes sense to move the `Consumer` mock to the object fields to avoid the suppress.
--
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