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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
