cadonna commented on code in PR #12739:
URL: https://github.com/apache/kafka/pull/12739#discussion_r1054286982
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/TimeOrderedCachingPersistentWindowStoreTest.java:
##########
@@ -104,11 +108,10 @@ public class TimeOrderedCachingPersistentWindowStoreTest {
private ThreadCache cache;
private InternalMockProcessorContext context;
private TimeFirstWindowKeySchema baseKeySchema;
- private WindowStore<Bytes, byte[]> underlyingStore;
+ private RocksDBTimeOrderedWindowStore underlyingStore;
Review Comment:
Why did you change the type?
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/TimeOrderedCachingPersistentWindowStoreTest.java:
##########
@@ -124,7 +127,8 @@ public static Collection<Object[]> data() {
public void setUp() {
baseKeySchema = new TimeFirstWindowKeySchema();
bytesStore = new RocksDBTimeOrderedWindowSegmentedBytesStore("test",
"metrics-scope", 100, SEGMENT_INTERVAL, hasIndex);
- underlyingStore = new RocksDBTimeOrderedWindowStore(bytesStore, false,
WINDOW_SIZE);
+ underlyingStore = new RocksDBTimeOrderedWindowStore(bytesStore,
+ false, WINDOW_SIZE);
Review Comment:
Also this change is not needed, right?
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/TimeOrderedCachingPersistentWindowStoreTest.java:
##########
@@ -144,45 +148,50 @@ public void closeStore() {
@SuppressWarnings("deprecation")
@Test
public void shouldDelegateDeprecatedInit() {
- final RocksDBTimeOrderedWindowStore inner =
EasyMock.mock(RocksDBTimeOrderedWindowStore.class);
- EasyMock.expect(inner.hasIndex()).andReturn(hasIndex);
- EasyMock.replay(inner);
- final TimeOrderedCachingWindowStore outer = new
TimeOrderedCachingWindowStore(inner, WINDOW_SIZE, SEGMENT_INTERVAL);
+ final RocksDBTimeOrderedWindowStore inner =
mock(RocksDBTimeOrderedWindowStore.class);
+ when(inner.hasIndex()).thenReturn(hasIndex);
+
+ final TimeOrderedCachingWindowStore outer =
+ new TimeOrderedCachingWindowStore(inner, WINDOW_SIZE,
+ SEGMENT_INTERVAL);
- EasyMock.reset(inner);
- EasyMock.expect(inner.name()).andStubReturn("store");
+ reset(inner);
+ when(inner.name()).thenReturn("store");
inner.init((org.apache.kafka.streams.processor.ProcessorContext)
context, outer);
- EasyMock.expectLastCall();
- EasyMock.replay(inner);
+
verify(inner).init((org.apache.kafka.streams.processor.ProcessorContext)
context, outer);
+
outer.init((org.apache.kafka.streams.processor.ProcessorContext)
context, outer);
- EasyMock.verify(inner);
+ verify(inner, times(2))
+ .init((org.apache.kafka.streams.processor.ProcessorContext)
context, outer);
}
@Test
public void shouldDelegateInit() {
Review Comment:
See my comments above.
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/TimeOrderedCachingPersistentWindowStoreTest.java:
##########
@@ -144,45 +148,50 @@ public void closeStore() {
@SuppressWarnings("deprecation")
@Test
public void shouldDelegateDeprecatedInit() {
- final RocksDBTimeOrderedWindowStore inner =
EasyMock.mock(RocksDBTimeOrderedWindowStore.class);
- EasyMock.expect(inner.hasIndex()).andReturn(hasIndex);
- EasyMock.replay(inner);
- final TimeOrderedCachingWindowStore outer = new
TimeOrderedCachingWindowStore(inner, WINDOW_SIZE, SEGMENT_INTERVAL);
+ final RocksDBTimeOrderedWindowStore inner =
mock(RocksDBTimeOrderedWindowStore.class);
+ when(inner.hasIndex()).thenReturn(hasIndex);
+
+ final TimeOrderedCachingWindowStore outer =
+ new TimeOrderedCachingWindowStore(inner, WINDOW_SIZE,
+ SEGMENT_INTERVAL);
Review Comment:
Please try to leave everything on the same line if the line does not exceed
by too much 120 characters.
```suggestion
final TimeOrderedCachingWindowStore outer = new
TimeOrderedCachingWindowStore(inner, WINDOW_SIZE, SEGMENT_INTERVAL);
```
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/TimeOrderedCachingPersistentWindowStoreTest.java:
##########
@@ -144,45 +148,50 @@ public void closeStore() {
@SuppressWarnings("deprecation")
@Test
public void shouldDelegateDeprecatedInit() {
- final RocksDBTimeOrderedWindowStore inner =
EasyMock.mock(RocksDBTimeOrderedWindowStore.class);
- EasyMock.expect(inner.hasIndex()).andReturn(hasIndex);
- EasyMock.replay(inner);
- final TimeOrderedCachingWindowStore outer = new
TimeOrderedCachingWindowStore(inner, WINDOW_SIZE, SEGMENT_INTERVAL);
+ final RocksDBTimeOrderedWindowStore inner =
mock(RocksDBTimeOrderedWindowStore.class);
+ when(inner.hasIndex()).thenReturn(hasIndex);
+
+ final TimeOrderedCachingWindowStore outer =
+ new TimeOrderedCachingWindowStore(inner, WINDOW_SIZE,
+ SEGMENT_INTERVAL);
- EasyMock.reset(inner);
- EasyMock.expect(inner.name()).andStubReturn("store");
+ reset(inner);
Review Comment:
This is not needed with Mockito.
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/TimeOrderedCachingPersistentWindowStoreTest.java:
##########
@@ -90,8 +88,15 @@
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertThrows;
import static org.junit.Assert.assertTrue;
-
-@RunWith(Parameterized.class)
Review Comment:
In the other migrations to Mockito we specified
`@RunWith(MockitoJUnitRunner.StrictStubs.class)`. So if that is not possible
here due to restrictions, I think we should use a rule as Divij proposed
instead of not using strict stubs at all.
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/TimeOrderedCachingPersistentWindowStoreTest.java:
##########
@@ -144,45 +148,50 @@ public void closeStore() {
@SuppressWarnings("deprecation")
@Test
public void shouldDelegateDeprecatedInit() {
- final RocksDBTimeOrderedWindowStore inner =
EasyMock.mock(RocksDBTimeOrderedWindowStore.class);
- EasyMock.expect(inner.hasIndex()).andReturn(hasIndex);
- EasyMock.replay(inner);
- final TimeOrderedCachingWindowStore outer = new
TimeOrderedCachingWindowStore(inner, WINDOW_SIZE, SEGMENT_INTERVAL);
+ final RocksDBTimeOrderedWindowStore inner =
mock(RocksDBTimeOrderedWindowStore.class);
+ when(inner.hasIndex()).thenReturn(hasIndex);
+
+ final TimeOrderedCachingWindowStore outer =
+ new TimeOrderedCachingWindowStore(inner, WINDOW_SIZE,
+ SEGMENT_INTERVAL);
- EasyMock.reset(inner);
- EasyMock.expect(inner.name()).andStubReturn("store");
+ reset(inner);
+ when(inner.name()).thenReturn("store");
inner.init((org.apache.kafka.streams.processor.ProcessorContext)
context, outer);
Review Comment:
So this test should look like this:
```java
@Test
public void shouldDelegateDeprecatedInit() {
final RocksDBTimeOrderedWindowStore inner =
mock(RocksDBTimeOrderedWindowStore.class);
when(inner.hasIndex()).thenReturn(hasIndex);
final TimeOrderedCachingWindowStore outer = new
TimeOrderedCachingWindowStore(inner, WINDOW_SIZE, SEGMENT_INTERVAL);
outer.init((org.apache.kafka.streams.processor.ProcessorContext)
context, outer);
verify(inner).init((org.apache.kafka.streams.processor.ProcessorContext)
context, outer);
}
```
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/TimeOrderedCachingPersistentWindowStoreTest.java:
##########
@@ -53,11 +53,9 @@
import
org.apache.kafka.streams.state.internals.PrefixedWindowKeySchemas.TimeFirstWindowKeySchema;
import org.apache.kafka.test.InternalMockProcessorContext;
import org.apache.kafka.test.TestUtils;
-import org.easymock.EasyMock;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
-
Review Comment:
Why did you remove those empty lines? Please put them back.
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/TimeOrderedCachingPersistentWindowStoreTest.java:
##########
@@ -144,45 +148,50 @@ public void closeStore() {
@SuppressWarnings("deprecation")
@Test
public void shouldDelegateDeprecatedInit() {
- final RocksDBTimeOrderedWindowStore inner =
EasyMock.mock(RocksDBTimeOrderedWindowStore.class);
- EasyMock.expect(inner.hasIndex()).andReturn(hasIndex);
- EasyMock.replay(inner);
- final TimeOrderedCachingWindowStore outer = new
TimeOrderedCachingWindowStore(inner, WINDOW_SIZE, SEGMENT_INTERVAL);
+ final RocksDBTimeOrderedWindowStore inner =
mock(RocksDBTimeOrderedWindowStore.class);
+ when(inner.hasIndex()).thenReturn(hasIndex);
+
+ final TimeOrderedCachingWindowStore outer =
+ new TimeOrderedCachingWindowStore(inner, WINDOW_SIZE,
+ SEGMENT_INTERVAL);
- EasyMock.reset(inner);
- EasyMock.expect(inner.name()).andStubReturn("store");
+ reset(inner);
+ when(inner.name()).thenReturn("store");
inner.init((org.apache.kafka.streams.processor.ProcessorContext)
context, outer);
Review Comment:
You need to remove this call since this call is only needed by EasyMock to
record the calls to verify. Mockito verifies calls with
`verify(inner).init((org.apache.kafka.streams.processor.ProcessorContext)
context, outer)`. In this and the next line you are basically calling a method
on the mock directly and verifying that you called the method on the mock. That
does actually not add anything to the test. In a test you should verify that a
call to a method of the class under test calls the method on the mock, not that
the test calls directly the method on the mock.
So you can remove both line.
After that you need also to remove the `times(2)` in `verify(inner,
times(2)).init((org.apache.kafka.streams.processor.ProcessorContext) context,
outer);` since that is only needed, because you call the method on the mock
directly.
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/TimeOrderedCachingPersistentWindowStoreTest.java:
##########
@@ -144,45 +148,50 @@ public void closeStore() {
@SuppressWarnings("deprecation")
@Test
public void shouldDelegateDeprecatedInit() {
- final RocksDBTimeOrderedWindowStore inner =
EasyMock.mock(RocksDBTimeOrderedWindowStore.class);
- EasyMock.expect(inner.hasIndex()).andReturn(hasIndex);
- EasyMock.replay(inner);
- final TimeOrderedCachingWindowStore outer = new
TimeOrderedCachingWindowStore(inner, WINDOW_SIZE, SEGMENT_INTERVAL);
+ final RocksDBTimeOrderedWindowStore inner =
mock(RocksDBTimeOrderedWindowStore.class);
+ when(inner.hasIndex()).thenReturn(hasIndex);
+
+ final TimeOrderedCachingWindowStore outer =
+ new TimeOrderedCachingWindowStore(inner, WINDOW_SIZE,
+ SEGMENT_INTERVAL);
- EasyMock.reset(inner);
- EasyMock.expect(inner.name()).andStubReturn("store");
+ reset(inner);
+ when(inner.name()).thenReturn("store");
inner.init((org.apache.kafka.streams.processor.ProcessorContext)
context, outer);
- EasyMock.expectLastCall();
- EasyMock.replay(inner);
+
verify(inner).init((org.apache.kafka.streams.processor.ProcessorContext)
context, outer);
+
outer.init((org.apache.kafka.streams.processor.ProcessorContext)
context, outer);
- EasyMock.verify(inner);
+ verify(inner, times(2))
+ .init((org.apache.kafka.streams.processor.ProcessorContext)
context, outer);
}
@Test
public void shouldDelegateInit() {
- final RocksDBTimeOrderedWindowStore inner =
EasyMock.mock(RocksDBTimeOrderedWindowStore.class);
- EasyMock.expect(inner.hasIndex()).andReturn(hasIndex);
- EasyMock.replay(inner);
- final TimeOrderedCachingWindowStore outer = new
TimeOrderedCachingWindowStore(inner, WINDOW_SIZE, SEGMENT_INTERVAL);
+ final RocksDBTimeOrderedWindowStore inner =
mock(RocksDBTimeOrderedWindowStore.class);
+ when(inner.hasIndex()).thenReturn(hasIndex);
+
+ final TimeOrderedCachingWindowStore outer =
+ spy(new TimeOrderedCachingWindowStore(inner, WINDOW_SIZE,
Review Comment:
Why do you need a spy here?
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/TimeOrderedCachingPersistentWindowStoreTest.java:
##########
@@ -1235,4 +1220,15 @@ private int addItemsToCache() {
return i;
}
+ private void verifyAndTearDownCloseTests() {
+ verify(underlyingStore).close();
+ verify(cache).flush(CACHE_NAMESPACE);
+ verify(cache).close(CACHE_NAMESPACE);
+
+ // resets the mocks created in #setUpCloseTests(). It is necessary to
+ // ensure that @After works correctly.
+ reset(cache);
+ reset(underlyingStore);
Review Comment:
You do not need this if you use add a `doNothing()` where you specify the
exceptions to be thrown. For example,
```java
doThrow(new RuntimeException("Simulating an error on
flush2")).when(cache).flush(CACHE_NAMESPACE);
```
becomes
```java
doThrow(new RuntimeException("Simulating an error on
flush2")).doNothing().when(cache).flush(CACHE_NAMESPACE);
```
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
