dplavcic commented on code in PR #12527:
URL: https://github.com/apache/kafka/pull/12527#discussion_r973351240
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/MeteredTimestampedWindowStoreTest.java:
##########
@@ -176,38 +183,34 @@ private void
doShouldPassChangelogTopicNameToStateStoreSerde(final String topic)
keySerde,
valueSerde
);
- store.init((StateStoreContext) context, store);
+ store.init((StateStoreContext) context, store);
store.fetch(KEY, TIMESTAMP);
store.put(KEY, VALUE_AND_TIMESTAMP, TIMESTAMP);
- verify(keySerializer, valueDeserializer, valueSerializer);
+ verify(innerStoreMock).fetch(any(Bytes.class), eq(TIMESTAMP));
Review Comment:
Since both values passed to the `fetch()` method are known (not mocks), we
can easily make this verify call more specific:
`verify(innerStoreMock).fetch(KEY_BYTES, TIMESTAMP);`
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/MeteredTimestampedWindowStoreTest.java:
##########
@@ -176,38 +183,34 @@ private void
doShouldPassChangelogTopicNameToStateStoreSerde(final String topic)
keySerde,
valueSerde
);
- store.init((StateStoreContext) context, store);
+ store.init((StateStoreContext) context, store);
store.fetch(KEY, TIMESTAMP);
store.put(KEY, VALUE_AND_TIMESTAMP, TIMESTAMP);
- verify(keySerializer, valueDeserializer, valueSerializer);
+ verify(innerStoreMock).fetch(any(Bytes.class), eq(TIMESTAMP));
+ verify(innerStoreMock).put(any(Bytes.class), any(byte[].class),
eq(TIMESTAMP));
Review Comment:
We can also make this `verify()` call more specific by updating it to: `
verify(innerStoreMock).put(KEY_BYTES, VALUE_AND_TIMESTAMP_BYTES, TIMESTAMP);
`
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/ChangeLoggingSessionBytesStoreTest.java:
##########
@@ -63,199 +55,128 @@ public class ChangeLoggingSessionBytesStoreTest {
@Before
public void setUp() {
store = new ChangeLoggingSessionBytesStore(inner);
+ store.init((StateStoreContext) context, store);
}
- private void init() {
- EasyMock.expect(context.taskId()).andReturn(taskId).anyTimes();
-
EasyMock.expect(context.recordCollector()).andReturn(collector).anyTimes();
-
EasyMock.expect(context.recordMetadata()).andReturn(Optional.empty()).anyTimes();
Review Comment:
I just double checked this, and all calls to `recordMetadata()` are not
needed anymore.
They were first introduced (in the production code) in the:
- Vicky Papavasileiou* 08.12.2021., 18:58 KAFKA-13506: Write and restore
position to/from changelog (#11513)
and than removed (in the production code), but they weren't removed from the
test class in the:
- Vicky Papavasileiou* 26.01.2022., 16:36 KAFKA-13524: Add IQv2 query
handling to the caching layer (#11682)
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/ChangeLoggingTimestampedWindowBytesStoreTest.java:
##########
@@ -63,129 +55,79 @@ public class ChangeLoggingTimestampedWindowBytesStoreTest {
@Before
public void setUp() {
store = new ChangeLoggingTimestampedWindowBytesStore(inner, false);
+ store.init((StateStoreContext) context, store);
}
- private void init() {
- EasyMock.expect(context.taskId()).andReturn(taskId).anyTimes();
-
EasyMock.expect(context.recordCollector()).andReturn(collector).anyTimes();
-
EasyMock.expect(context.recordMetadata()).andReturn(Optional.empty()).anyTimes();
- inner.init((StateStoreContext) context, store);
- EasyMock.expectLastCall();
- EasyMock.replay(inner, context);
-
- store.init((StateStoreContext) context, store);
+ @After
+ public void tearDown() {
+ verify(inner).init((StateStoreContext) context, store);
}
@SuppressWarnings("deprecation")
@Test
public void shouldDelegateDeprecatedInit() {
- inner.init((ProcessorContext) context, store);
- EasyMock.expectLastCall();
- EasyMock.replay(inner);
store.init((ProcessorContext) context, store);
- EasyMock.verify(inner);
+
+ verify(inner).init((ProcessorContext) context, store);
}
@Test
public void shouldDelegateInit() {
- inner.init((StateStoreContext) context, store);
- EasyMock.expectLastCall();
- EasyMock.replay(inner);
- store.init((StateStoreContext) context, store);
- EasyMock.verify(inner);
+ // testing the combination of setUp and tearDown
}
@Test
@SuppressWarnings("deprecation")
public void shouldLogPuts() {
-
EasyMock.expect(inner.getPosition()).andReturn(Position.emptyPosition()).anyTimes();
- inner.put(bytesKey, valueAndTimestamp, 0);
- EasyMock.expectLastCall();
-
- init();
-
final Bytes key = WindowKeySchema.toStoreKeyBinary(bytesKey, 0, 0);
+ when(inner.getPosition()).thenReturn(Position.emptyPosition());
- EasyMock.reset(context);
-
EasyMock.expect(context.recordMetadata()).andStubReturn(Optional.empty());
- context.logChange(store.name(), key, value, 42,
Position.emptyPosition());
-
- EasyMock.replay(context);
store.put(bytesKey, valueAndTimestamp, context.timestamp());
- EasyMock.verify(inner, context);
+ verify(inner).put(bytesKey, valueAndTimestamp, 0);
+ verify(context).logChange(store.name(), key, value, 42,
Position.emptyPosition());
}
@Test
public void shouldLogPutsWithPosition() {
- EasyMock.expect(inner.getPosition()).andReturn(POSITION).anyTimes();
- inner.put(bytesKey, valueAndTimestamp, 0);
- EasyMock.expectLastCall();
-
- init();
-
final Bytes key = WindowKeySchema.toStoreKeyBinary(bytesKey, 0, 0);
+ when(inner.getPosition()).thenReturn(POSITION);
- EasyMock.reset(context);
- final RecordMetadata recordContext = new ProcessorRecordContext(0L,
1L, 0, "", new RecordHeaders());
-
EasyMock.expect(context.recordMetadata()).andStubReturn(Optional.of(recordContext));
- final Position position = Position.fromMap(mkMap(mkEntry("",
mkMap(mkEntry(0, 1L)))));
- context.logChange(store.name(), key, value, 42, position);
-
- EasyMock.replay(context);
store.put(bytesKey, valueAndTimestamp, context.timestamp());
- EasyMock.verify(inner, context);
+ verify(inner).put(bytesKey, valueAndTimestamp, 0);
+ verify(context).logChange(store.name(), key, value, 42, POSITION);
}
@Test
public void shouldDelegateToUnderlyingStoreWhenFetching() {
- EasyMock
- .expect(inner.fetch(bytesKey, 0, 10))
- .andReturn(KeyValueIterators.emptyWindowStoreIterator());
-
- init();
-
store.fetch(bytesKey, ofEpochMilli(0), ofEpochMilli(10));
- EasyMock.verify(inner);
+
+ verify(inner).fetch(bytesKey, 0, 10);
}
@Test
public void shouldDelegateToUnderlyingStoreWhenFetchingRange() {
- EasyMock
- .expect(inner.fetch(bytesKey, bytesKey, 0, 1))
- .andReturn(KeyValueIterators.emptyIterator());
-
- init();
-
store.fetch(bytesKey, bytesKey, ofEpochMilli(0), ofEpochMilli(1));
- EasyMock.verify(inner);
+
+ verify(inner).fetch(bytesKey, bytesKey, 0, 1);
}
@Test
@SuppressWarnings("deprecation")
public void shouldRetainDuplicatesWhenSet() {
store = new ChangeLoggingTimestampedWindowBytesStore(inner, true);
-
EasyMock.expect(inner.getPosition()).andReturn(Position.emptyPosition()).anyTimes();
- inner.put(bytesKey, valueAndTimestamp, 0);
- EasyMock.expectLastCall().times(2);
-
- init();
-
+ store.init((StateStoreContext) context, store);
Review Comment:
We already have `store.init((StateStoreContext) context, store);` this in
the `@Before` part. Can you please double-check that this is not redundant?
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/ChangeLoggingSessionBytesStoreTest.java:
##########
@@ -63,199 +55,128 @@ public class ChangeLoggingSessionBytesStoreTest {
@Before
public void setUp() {
store = new ChangeLoggingSessionBytesStore(inner);
+ store.init((StateStoreContext) context, store);
}
- private void init() {
- EasyMock.expect(context.taskId()).andReturn(taskId).anyTimes();
Review Comment:
I just double checked this:
- manually, by putting breakpoints and executing each test
- automatically, by adding `when(context.taskId()).thenReturn(taskId)` in
which case mockito throws unnecessary stubbing exception
--
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]
