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/12/21 12:29:12 UTC
[GitHub] [kafka] cadonna commented on a diff in pull request #12739: Replace EasyMock and PowerMock with Mockito | TimeOrderedCachingPersistentWindowStoreTest
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org