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/08/11 15:41:25 UTC
[GitHub] [kafka] clolov opened a new pull request, #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
clolov opened a new pull request, #12505:
URL: https://github.com/apache/kafka/pull/12505
Batch 2 of the tests detailed in https://issues.apache.org/jira/browse/KAFKA-14133 which use EasyMock and need to be moved to Mockito.
--
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
[GitHub] [kafka] cadonna commented on pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
cadonna commented on PR #12505:
URL: https://github.com/apache/kafka/pull/12505#issuecomment-1321749658
I restarted the builds to ensure everything is still fine with current trunk since they were run 18 days ago last time. If the builds are acceptable, I will merge the PR.
--
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
[GitHub] [kafka] cadonna commented on pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
cadonna commented on PR #12505:
URL: https://github.com/apache/kafka/pull/12505#issuecomment-1321960582
Build failures are not related:
```
Build / JDK 11 and Scala 2.13 / org.apache.kafka.common.network.SslVersionsTransportLayerTest.tlsServerProtocol = [TLSv1.2, TLSv1.3], tlsClientProtocol = [TLSv1.3]
Build / JDK 11 and Scala 2.13 / org.apache.kafka.common.record.MemoryRecordsBuilderTest.[6] magic=2, bufferOffset=0, compressionType=gzip
Build / JDK 11 and Scala 2.13 / org.apache.kafka.common.record.MemoryRecordsBuilderTest.[9] magic=2, bufferOffset=0, compressionType=snappy
Build / JDK 11 and Scala 2.13 / org.apache.kafka.common.record.MemoryRecordsBuilderTest.[10] magic=0, bufferOffset=0, compressionType=lz4
Build / JDK 11 and Scala 2.13 / org.apache.kafka.common.record.MemoryRecordsBuilderTest.[15] magic=1, bufferOffset=15, compressionType=none
Build / JDK 11 and Scala 2.13 / org.apache.kafka.common.record.MemoryRecordsBuilderTest.[18] magic=1, bufferOffset=15, compressionType=gzip
Build / JDK 11 and Scala 2.13 / org.apache.kafka.common.record.MemoryRecordsBuilderTest.[22] magic=2, bufferOffset=15, compressionType=snappy
Build / JDK 11 and Scala 2.13 / org.apache.kafka.common.record.MemoryRecordsBuilderTest.[24] magic=1, bufferOffset=15, compressionType=lz4
Build / JDK 11 and Scala 2.13 / org.apache.kafka.common.record.MemoryRecordsBuilderTest.[26] magic=2, bufferOffset=15, compressionType=zstd
Build / JDK 17 and Scala 2.13 / org.apache.kafka.connect.integration.ConnectorRestartApiIntegrationTest.testMultiWorkerRestartOnlyConnector
```
--
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
[GitHub] [kafka] clolov commented on pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
clolov commented on PR #12505:
URL: https://github.com/apache/kafka/pull/12505#issuecomment-1215575745
Hey @dplavcic, thank you for the comments. I will aim to provide an answer in the next couple of days!
--
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
[GitHub] [kafka] dplavcic commented on a diff in pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
dplavcic commented on code in PR #12505:
URL: https://github.com/apache/kafka/pull/12505#discussion_r944781108
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/KeyValueStoreBuilderTest.java:
##########
@@ -126,27 +121,15 @@ public void shouldThrowNullPointerIfInnerIsNull() {
Serdes.String(), new MockTime()));
}
- @Test
- public void shouldThrowNullPointerIfKeySerdeIsNull() {
- assertThrows(NullPointerException.class, () -> new KeyValueStoreBuilder<>(supplier, null, Serdes.String(), new MockTime()));
- }
-
- @Test
- public void shouldThrowNullPointerIfValueSerdeIsNull() {
- assertThrows(NullPointerException.class, () -> new KeyValueStoreBuilder<>(supplier, Serdes.String(), null, new MockTime()));
- }
Review Comment:
Please check previous comment for explanation.
```text
Expected: "value serde cannot be null"
but: was "name cannot be null"
```
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/KeyValueStoreBuilderTest.java:
##########
@@ -126,27 +121,15 @@ public void shouldThrowNullPointerIfInnerIsNull() {
Serdes.String(), new MockTime()));
}
- @Test
Review Comment:
Nice catch!
The thing is that `supplier.name()` returns `null` and there is an explicit null check on the `name` value in the `AbstractStoreBuilder` class.
If we update this test and assert/verify error message:
```java
@Test
public void shouldThrowNullPointerIfKeySerdeIsNull() {
final Exception e = assertThrows(NullPointerException.class, () -> new KeyValueStoreBuilder<>(supplier, null, Serdes.String(), new MockTime()));
assertThat(e.getMessage(), equalTo("serde cannot be null"));
}
```
We will see that it fails with following message:
```text
Expected: "serde cannot be null"
but: was "name cannot be null"
```
The question to someone more familiar with the project is:
- should we add `null` check in the `KeyValueStoreBuilder` constructor?
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/StreamThreadStateStoreProviderTest.java:
##########
@@ -449,22 +454,18 @@ private StreamTask createStreamsTask(final StreamsConfig streamsConfig,
new TopologyConfig(null, streamsConfig, new Properties()).getTaskConfig(),
streamsMetrics,
stateDirectory,
- EasyMock.createNiceMock(ThreadCache.class),
+ mock(ThreadCache.class),
new MockTime(),
stateManager,
recordCollector,
context, logContext);
}
private void mockThread(final boolean initialized) {
- EasyMock.expect(threadMock.isRunning()).andReturn(initialized);
- EasyMock.expect(threadMock.allTasks()).andStubReturn(tasks);
- EasyMock.expect(threadMock.activeTaskMap()).andStubReturn(tasks);
Review Comment:
I guess first three stubs are no longer needed or?
--
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
[GitHub] [kafka] clolov commented on a diff in pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
clolov commented on code in PR #12505:
URL: https://github.com/apache/kafka/pull/12505#discussion_r1011548562
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/CachingInMemoryKeyValueStoreTest.java:
##########
@@ -153,57 +154,48 @@ public void shouldAvoidFlushingDeletionsWithoutDirtyKeys() {
@Test
public void shouldCloseWrappedStoreAndCacheAfterErrorDuringCacheFlush() {
setUpCloseTests();
- EasyMock.reset(cache);
- cache.flush(CACHE_NAMESPACE);
- EasyMock.expectLastCall().andThrow(new RuntimeException("Simulating an error on flush"));
- EasyMock.replay(cache);
- EasyMock.reset(underlyingStore);
- underlyingStore.close();
- EasyMock.replay(underlyingStore);
+
+ doThrow(new RuntimeException("Simulating an error on flush")).when(cache).flush(CACHE_NAMESPACE);
assertThrows(RuntimeException.class, store::close);
- EasyMock.verify(cache, underlyingStore);
+
+ verify(cache).flush(eq(CACHE_NAMESPACE));
Review Comment:
Nope, we do not need the explicit verification, I have removed it in subsequent commits.
--
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
[GitHub] [kafka] clolov commented on a diff in pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
clolov commented on code in PR #12505:
URL: https://github.com/apache/kafka/pull/12505#discussion_r947629580
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/CachingPersistentWindowStoreTest.java:
##########
@@ -119,29 +126,24 @@ public void closeStore() {
cachingStore.close();
}
- @SuppressWarnings("deprecation")
+ @SuppressWarnings({"deprecation", "unchecked"})
@Test
public void shouldDelegateDeprecatedInit() {
- final WindowStore<Bytes, byte[]> inner = EasyMock.mock(WindowStore.class);
+ final WindowStore<Bytes, byte[]> inner = mock(WindowStore.class);
final CachingWindowStore outer = new CachingWindowStore(inner, WINDOW_SIZE, SEGMENT_INTERVAL);
- EasyMock.expect(inner.name()).andStubReturn("store");
- inner.init((ProcessorContext) context, outer);
- EasyMock.expectLastCall();
- EasyMock.replay(inner);
+ when(inner.name()).thenReturn("store");
outer.init((ProcessorContext) context, outer);
- EasyMock.verify(inner);
+ verify(inner).init(eq((ProcessorContext) context), eq(outer));
Review Comment:
Matchers are indeed not needed. Fixed in upcoming commits.
--
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
[GitHub] [kafka] cadonna commented on a diff in pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
cadonna commented on code in PR #12505:
URL: https://github.com/apache/kafka/pull/12505#discussion_r966707489
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/CachingPersistentSessionStoreTest.java:
##########
@@ -258,58 +266,45 @@ public void shouldPutBackwardFetchAllKeysFromCache() {
@Test
public void shouldCloseWrappedStoreAndCacheAfterErrorDuringCacheFlush() {
setUpCloseTests();
- EasyMock.reset(cache);
- cache.flush(CACHE_NAMESPACE);
- EasyMock.expectLastCall().andThrow(new RuntimeException("Simulating an error on flush"));
- EasyMock.replay(cache);
- EasyMock.reset(underlyingStore);
- underlyingStore.close();
- EasyMock.replay(underlyingStore);
+ doThrow(new RuntimeException("Simulating an error on flush")).doNothing().when(cache).flush(anyString());
assertThrows(RuntimeException.class, cachingStore::close);
- EasyMock.verify(cache, underlyingStore);
+ verify(cache).flush(eq(CACHE_NAMESPACE));
+ verify(cache).close(eq(CACHE_NAMESPACE));
+ verify(underlyingStore).close();
}
@Test
public void shouldCloseWrappedStoreAfterErrorDuringCacheClose() {
setUpCloseTests();
- EasyMock.reset(cache);
- cache.flush(CACHE_NAMESPACE);
- cache.close(CACHE_NAMESPACE);
- EasyMock.expectLastCall().andThrow(new RuntimeException("Simulating an error on close"));
- EasyMock.replay(cache);
- EasyMock.reset(underlyingStore);
- underlyingStore.close();
- EasyMock.replay(underlyingStore);
+
Review Comment:
nit: Please remove this line here and in other parts of this PR. Stubbing is part of the setup IMO.
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/CachingInMemoryKeyValueStoreTest.java:
##########
@@ -105,26 +112,20 @@ protected <K, V> KeyValueStore<K, V> createKeyValueStore(final StateStoreContext
@SuppressWarnings("deprecation")
@Test
public void shouldDelegateDeprecatedInit() {
- final KeyValueStore<Bytes, byte[]> inner = EasyMock.mock(InMemoryKeyValueStore.class);
+ final KeyValueStore<Bytes, byte[]> inner = mock(InMemoryKeyValueStore.class);
final CachingKeyValueStore outer = new CachingKeyValueStore(inner, false);
- EasyMock.expect(inner.name()).andStubReturn("store");
- inner.init((ProcessorContext) context, outer);
- EasyMock.expectLastCall();
- EasyMock.replay(inner);
+ when(inner.name()).thenReturn("store");
outer.init((ProcessorContext) context, outer);
- EasyMock.verify(inner);
+ verify(inner).init(eq((ProcessorContext) context), eq(outer));
}
@Test
public void shouldDelegateInit() {
- final KeyValueStore<Bytes, byte[]> inner = EasyMock.mock(InMemoryKeyValueStore.class);
+ final KeyValueStore<Bytes, byte[]> inner = mock(InMemoryKeyValueStore.class);
final CachingKeyValueStore outer = new CachingKeyValueStore(inner, false);
- EasyMock.expect(inner.name()).andStubReturn("store");
- inner.init((StateStoreContext) context, outer);
- EasyMock.expectLastCall();
- EasyMock.replay(inner);
+ when(inner.name()).thenReturn("store");
outer.init((StateStoreContext) context, outer);
- EasyMock.verify(inner);
+ verify(inner).init(eq((StateStoreContext) context), eq(outer));
Review Comment:
Out of curiosity: Why do you need `eq()` here?
This comment applies also to other parts of the PR.
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/CachingPersistentWindowStoreTest.java:
##########
@@ -119,29 +125,24 @@ public void closeStore() {
cachingStore.close();
}
- @SuppressWarnings("deprecation")
+ @SuppressWarnings({"deprecation", "unchecked"})
@Test
public void shouldDelegateDeprecatedInit() {
- final WindowStore<Bytes, byte[]> inner = EasyMock.mock(WindowStore.class);
+ final WindowStore<Bytes, byte[]> inner = mock(WindowStore.class);
Review Comment:
You could move the mock specification to the object fields and use `@Mock` to avoid suppression. Same is true for other places in this PR where suppression is applied.
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/StreamsRebalanceListenerTest.java:
##########
@@ -22,164 +22,143 @@
import org.apache.kafka.streams.errors.TaskAssignmentException;
import org.apache.kafka.streams.processor.internals.StreamThread.State;
import org.apache.kafka.streams.processor.internals.assignment.AssignorError;
-import org.easymock.EasyMock;
import org.junit.Before;
import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
import org.slf4j.LoggerFactory;
import java.util.Collection;
import java.util.Collections;
import java.util.concurrent.atomic.AtomicInteger;
-import static org.easymock.EasyMock.expect;
-import static org.easymock.EasyMock.expectLastCall;
-import static org.easymock.EasyMock.mock;
-import static org.easymock.EasyMock.replay;
-import static org.easymock.EasyMock.verify;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertThrows;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+@RunWith(MockitoJUnitRunner.StrictStubs.class)
public class StreamsRebalanceListenerTest {
- private final TaskManager taskManager = mock(TaskManager.class);
- private final StreamThread streamThread = mock(StreamThread.class);
+ @Mock
+ private TaskManager taskManager;
+ @Mock
+ private StreamThread streamThread;
private final AtomicInteger assignmentErrorCode = new AtomicInteger();
private final MockTime time = new MockTime();
- private final StreamsRebalanceListener streamsRebalanceListener = new StreamsRebalanceListener(
- time,
- taskManager,
- streamThread,
- LoggerFactory.getLogger(StreamsRebalanceListenerTest.class),
- assignmentErrorCode
- );
+ private StreamsRebalanceListener streamsRebalanceListener;
@Before
- public void before() {
- expect(streamThread.state()).andStubReturn(null);
- expect(taskManager.activeTaskIds()).andStubReturn(null);
- expect(taskManager.standbyTaskIds()).andStubReturn(null);
+ public void setup() {
+ streamsRebalanceListener = new StreamsRebalanceListener(time,
+ taskManager,
+ streamThread,
+ LoggerFactory.getLogger(StreamsRebalanceListenerTest.class),
+ assignmentErrorCode
+ );
Review Comment:
Is there a specific reason to move the creation of the listener to the setup method? I think it is fine to leave the creation at the field.
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/CachingInMemoryKeyValueStoreTest.java:
##########
@@ -153,57 +154,48 @@ public void shouldAvoidFlushingDeletionsWithoutDirtyKeys() {
@Test
public void shouldCloseWrappedStoreAndCacheAfterErrorDuringCacheFlush() {
setUpCloseTests();
- EasyMock.reset(cache);
- cache.flush(CACHE_NAMESPACE);
- EasyMock.expectLastCall().andThrow(new RuntimeException("Simulating an error on flush"));
- EasyMock.replay(cache);
- EasyMock.reset(underlyingStore);
- underlyingStore.close();
- EasyMock.replay(underlyingStore);
+
+ doThrow(new RuntimeException("Simulating an error on flush")).when(cache).flush(CACHE_NAMESPACE);
assertThrows(RuntimeException.class, store::close);
- EasyMock.verify(cache, underlyingStore);
+
+ verify(cache).flush(eq(CACHE_NAMESPACE));
+ verify(cache).close(eq(CACHE_NAMESPACE));
+ verify(underlyingStore).close();
}
@Test
public void shouldCloseWrappedStoreAfterErrorDuringCacheClose() {
setUpCloseTests();
- EasyMock.reset(cache);
- cache.flush(CACHE_NAMESPACE);
- cache.close(CACHE_NAMESPACE);
- EasyMock.expectLastCall().andThrow(new RuntimeException("Simulating an error on close"));
- EasyMock.replay(cache);
- EasyMock.reset(underlyingStore);
- underlyingStore.close();
- EasyMock.replay(underlyingStore);
+
+ doThrow(new RuntimeException("Simulating an error on close")).when(cache).close(CACHE_NAMESPACE);
assertThrows(RuntimeException.class, store::close);
- EasyMock.verify(cache, underlyingStore);
+
+ verify(cache).flush(eq(CACHE_NAMESPACE));
+ verify(cache).close(eq(CACHE_NAMESPACE));
Review Comment:
Same question as above about verifying a stubbing.
I saw the same situation also in other parts of this PR.
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/CachingInMemoryKeyValueStoreTest.java:
##########
@@ -153,57 +154,48 @@ public void shouldAvoidFlushingDeletionsWithoutDirtyKeys() {
@Test
public void shouldCloseWrappedStoreAndCacheAfterErrorDuringCacheFlush() {
setUpCloseTests();
- EasyMock.reset(cache);
- cache.flush(CACHE_NAMESPACE);
- EasyMock.expectLastCall().andThrow(new RuntimeException("Simulating an error on flush"));
- EasyMock.replay(cache);
- EasyMock.reset(underlyingStore);
- underlyingStore.close();
- EasyMock.replay(underlyingStore);
+
+ doThrow(new RuntimeException("Simulating an error on flush")).when(cache).flush(CACHE_NAMESPACE);
assertThrows(RuntimeException.class, store::close);
- EasyMock.verify(cache, underlyingStore);
+
+ verify(cache).flush(eq(CACHE_NAMESPACE));
Review Comment:
Do we need to verify this since it is already stubbed and we use `StrictStubbed`?
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/CachingInMemoryKeyValueStoreTest.java:
##########
@@ -153,57 +154,48 @@ public void shouldAvoidFlushingDeletionsWithoutDirtyKeys() {
@Test
public void shouldCloseWrappedStoreAndCacheAfterErrorDuringCacheFlush() {
setUpCloseTests();
- EasyMock.reset(cache);
- cache.flush(CACHE_NAMESPACE);
- EasyMock.expectLastCall().andThrow(new RuntimeException("Simulating an error on flush"));
- EasyMock.replay(cache);
- EasyMock.reset(underlyingStore);
- underlyingStore.close();
- EasyMock.replay(underlyingStore);
+
+ doThrow(new RuntimeException("Simulating an error on flush")).when(cache).flush(CACHE_NAMESPACE);
assertThrows(RuntimeException.class, store::close);
- EasyMock.verify(cache, underlyingStore);
+
+ verify(cache).flush(eq(CACHE_NAMESPACE));
+ verify(cache).close(eq(CACHE_NAMESPACE));
Review Comment:
Again, why do we need `eq()` here?
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/CachingPersistentWindowStoreTest.java:
##########
@@ -1039,58 +1041,42 @@ public void shouldNotThrowInvalidBackwardRangeExceptionWithNegativeFromKey() {
@Test
public void shouldCloseCacheAndWrappedStoreAfterErrorDuringCacheFlush() {
setUpCloseTests();
- EasyMock.reset(cache);
- cache.flush(CACHE_NAMESPACE);
- EasyMock.expectLastCall().andThrow(new RuntimeException("Simulating an error on flush"));
- cache.close(CACHE_NAMESPACE);
- EasyMock.replay(cache);
- EasyMock.reset(underlyingStore);
- underlyingStore.close();
- EasyMock.replay(underlyingStore);
+ doThrow(new RuntimeException("Simulating an error on flush")).doNothing().when(cache).flush(CACHE_NAMESPACE);
assertThrows(RuntimeException.class, cachingStore::close);
- EasyMock.verify(cache, underlyingStore);
+ verify(cache).flush(CACHE_NAMESPACE);
+ verify(cache).close(CACHE_NAMESPACE);
+ verify(underlyingStore).close();
Review Comment:
I think it makes sense to use `InOrder` here since we want to flush a cache/store before closing it.
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/KeyValueStoreBuilderTest.java:
##########
@@ -126,27 +121,15 @@ public void shouldThrowNullPointerIfInnerIsNull() {
Serdes.String(), new MockTime()));
}
- @Test
Review Comment:
That is unfortunate! Yes, we should add null checks!
Lesson learned:
If you verify an exception always verify the exception message as well. 🙂
--
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
[GitHub] [kafka] clolov commented on pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
clolov commented on PR #12505:
URL: https://github.com/apache/kafka/pull/12505#issuecomment-1299981863
Hello @cadonna. Apologies, I am back on having bandwidth to move this (and the rest of the pull requests) to a conclusion. I will post an update later today.
--
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
[GitHub] [kafka] clolov commented on a diff in pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
clolov commented on code in PR #12505:
URL: https://github.com/apache/kafka/pull/12505#discussion_r943639431
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/KeyValueStoreBuilderTest.java:
##########
@@ -126,27 +121,15 @@ public void shouldThrowNullPointerIfInnerIsNull() {
Serdes.String(), new MockTime()));
}
- @Test
Review Comment:
As far as I can tell, the behaviour exercised in these tests is never enforced in the code so I have no idea how they have been passing up to now. I deleted them, but if I am shown to be wrong I will return them.
--
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
[GitHub] [kafka] clolov commented on pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
clolov commented on PR #12505:
URL: https://github.com/apache/kafka/pull/12505#issuecomment-1212166713
Tagging @mdedetrich @cadonna @ijuma for visibility and reviews :)
--
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
[GitHub] [kafka] dplavcic commented on a diff in pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
dplavcic commented on code in PR #12505:
URL: https://github.com/apache/kafka/pull/12505#discussion_r949392447
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/KeyValueStoreBuilderTest.java:
##########
@@ -126,27 +121,15 @@ public void shouldThrowNullPointerIfInnerIsNull() {
Serdes.String(), new MockTime()));
}
- @Test
Review Comment:
Absolutely. I will create a new Jira ticket for this.
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/CachingPersistentWindowStoreTest.java:
##########
@@ -1039,58 +1041,42 @@ public void shouldNotThrowInvalidBackwardRangeExceptionWithNegativeFromKey() {
@Test
public void shouldCloseCacheAndWrappedStoreAfterErrorDuringCacheFlush() {
setUpCloseTests();
- EasyMock.reset(cache);
- cache.flush(CACHE_NAMESPACE);
- EasyMock.expectLastCall().andThrow(new RuntimeException("Simulating an error on flush"));
- cache.close(CACHE_NAMESPACE);
- EasyMock.replay(cache);
- EasyMock.reset(underlyingStore);
- underlyingStore.close();
- EasyMock.replay(underlyingStore);
+ doThrow(new RuntimeException("Simulating an error on flush")).doNothing().when(cache).flush(CACHE_NAMESPACE);
assertThrows(RuntimeException.class, cachingStore::close);
- EasyMock.verify(cache, underlyingStore);
+ verify(cache).flush(CACHE_NAMESPACE);
+ verify(cache).close(CACHE_NAMESPACE);
+ verify(underlyingStore).close();
Review Comment:
If we check `cachingStore::close` method implementation it looks like this:
```java
@Override
public synchronized void close() {
final LinkedList<RuntimeException> suppressed = executeAll(
() -> context.cache().flush(cacheName),
() -> context.cache().close(cacheName),
wrapped()::close
);
if (!suppressed.isEmpty()) {
throwSuppressed("Caught an exception while closing caching window store for store " + name(),
suppressed);
}
}
```
Than, we have`executeAll()` method implementation that will execute multiple runnables in order:
```java
static LinkedList<RuntimeException> executeAll(final Runnable... actions) {
final LinkedList<RuntimeException> suppressed = new LinkedList<>();
for (final Runnable action : actions) {
try {
action.run();
} catch (final RuntimeException exception) {
suppressed.add(exception);
}
}
return suppressed;
}
```
Based on that, to me, it seems that the original production code author purposely called `flush` before `close` and I would explicitly verify that with Mockito's `inOrder`.
If you think the `flush()` and `close()` method invocation order doesn't matter please feel free to live it as is.
--
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
[GitHub] [kafka] clolov commented on a diff in pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
clolov commented on code in PR #12505:
URL: https://github.com/apache/kafka/pull/12505#discussion_r1011531468
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/CachingInMemoryKeyValueStoreTest.java:
##########
@@ -105,26 +112,20 @@ protected <K, V> KeyValueStore<K, V> createKeyValueStore(final StateStoreContext
@SuppressWarnings("deprecation")
@Test
public void shouldDelegateDeprecatedInit() {
- final KeyValueStore<Bytes, byte[]> inner = EasyMock.mock(InMemoryKeyValueStore.class);
+ final KeyValueStore<Bytes, byte[]> inner = mock(InMemoryKeyValueStore.class);
final CachingKeyValueStore outer = new CachingKeyValueStore(inner, false);
- EasyMock.expect(inner.name()).andStubReturn("store");
- inner.init((ProcessorContext) context, outer);
- EasyMock.expectLastCall();
- EasyMock.replay(inner);
+ when(inner.name()).thenReturn("store");
outer.init((ProcessorContext) context, outer);
- EasyMock.verify(inner);
+ verify(inner).init(eq((ProcessorContext) context), eq(outer));
}
@Test
public void shouldDelegateInit() {
- final KeyValueStore<Bytes, byte[]> inner = EasyMock.mock(InMemoryKeyValueStore.class);
+ final KeyValueStore<Bytes, byte[]> inner = mock(InMemoryKeyValueStore.class);
final CachingKeyValueStore outer = new CachingKeyValueStore(inner, false);
- EasyMock.expect(inner.name()).andStubReturn("store");
- inner.init((StateStoreContext) context, outer);
- EasyMock.expectLastCall();
- EasyMock.replay(inner);
+ when(inner.name()).thenReturn("store");
outer.init((StateStoreContext) context, outer);
- EasyMock.verify(inner);
+ verify(inner).init(eq((StateStoreContext) context), eq(outer));
Review Comment:
We do not need it here. I will make another pass through the pull request to remove them.
--
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
[GitHub] [kafka] clolov commented on a diff in pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
clolov commented on code in PR #12505:
URL: https://github.com/apache/kafka/pull/12505#discussion_r1011752132
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/StreamsRebalanceListenerTest.java:
##########
@@ -22,164 +22,143 @@
import org.apache.kafka.streams.errors.TaskAssignmentException;
import org.apache.kafka.streams.processor.internals.StreamThread.State;
import org.apache.kafka.streams.processor.internals.assignment.AssignorError;
-import org.easymock.EasyMock;
import org.junit.Before;
import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
import org.slf4j.LoggerFactory;
import java.util.Collection;
import java.util.Collections;
import java.util.concurrent.atomic.AtomicInteger;
-import static org.easymock.EasyMock.expect;
-import static org.easymock.EasyMock.expectLastCall;
-import static org.easymock.EasyMock.mock;
-import static org.easymock.EasyMock.replay;
-import static org.easymock.EasyMock.verify;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertThrows;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+@RunWith(MockitoJUnitRunner.StrictStubs.class)
public class StreamsRebalanceListenerTest {
- private final TaskManager taskManager = mock(TaskManager.class);
- private final StreamThread streamThread = mock(StreamThread.class);
+ @Mock
+ private TaskManager taskManager;
+ @Mock
+ private StreamThread streamThread;
private final AtomicInteger assignmentErrorCode = new AtomicInteger();
private final MockTime time = new MockTime();
- private final StreamsRebalanceListener streamsRebalanceListener = new StreamsRebalanceListener(
- time,
- taskManager,
- streamThread,
- LoggerFactory.getLogger(StreamsRebalanceListenerTest.class),
- assignmentErrorCode
- );
+ private StreamsRebalanceListener streamsRebalanceListener;
@Before
- public void before() {
- expect(streamThread.state()).andStubReturn(null);
- expect(taskManager.activeTaskIds()).andStubReturn(null);
- expect(taskManager.standbyTaskIds()).andStubReturn(null);
+ public void setup() {
+ streamsRebalanceListener = new StreamsRebalanceListener(time,
+ taskManager,
+ streamThread,
+ LoggerFactory.getLogger(StreamsRebalanceListenerTest.class),
+ assignmentErrorCode
+ );
Review Comment:
We give mocks as arguments to the listener. The mocks are not initialised at that point in time if we use the `@Mock` annotation which leads to all the tests failing with `NullPointerException`s. If we put the assignment in `@Before` then the mocks are initialised. Alternatively I can offer changing from
```
@Mock
private TaskManager taskManager;
@Mock
private StreamThread streamThread;
```
to
```
private TaskManager taskManager = mock(TaskManager.class);
private StreamThread streamThread = mock(StreamThread.class);
```
which also seems to work.
--
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
[GitHub] [kafka] clolov commented on a diff in pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
clolov commented on code in PR #12505:
URL: https://github.com/apache/kafka/pull/12505#discussion_r1011538831
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/RocksDBStoreTest.java:
##########
@@ -194,28 +197,20 @@ private InternalMockProcessorContext getProcessorContext(final RecordingLevel re
public void shouldAddValueProvidersWithoutStatisticsToInjectedMetricsRecorderWhenRecordingLevelInfo() {
rocksDBStore = getRocksDBStoreWithRocksDBMetricsRecorder();
context = getProcessorContext(RecordingLevel.INFO);
- reset(metricsRecorder);
- metricsRecorder.addValueProviders(eq(DB_NAME), notNull(), notNull(), isNull());
- replay(metricsRecorder);
rocksDBStore.openDB(context.appConfigs(), context.stateDir());
- verify(metricsRecorder);
- reset(metricsRecorder);
+ verify(metricsRecorder).addValueProviders(eq(DB_NAME), notNull(), notNull(), isNull());
Review Comment:
I cannot remove `eq()` here because we need to either fully use matchers or not use them at all.
--
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
[GitHub] [kafka] clolov commented on a diff in pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
clolov commented on code in PR #12505:
URL: https://github.com/apache/kafka/pull/12505#discussion_r947632500
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/CachingPersistentWindowStoreTest.java:
##########
@@ -1039,58 +1041,42 @@ public void shouldNotThrowInvalidBackwardRangeExceptionWithNegativeFromKey() {
@Test
public void shouldCloseCacheAndWrappedStoreAfterErrorDuringCacheFlush() {
setUpCloseTests();
- EasyMock.reset(cache);
- cache.flush(CACHE_NAMESPACE);
- EasyMock.expectLastCall().andThrow(new RuntimeException("Simulating an error on flush"));
- cache.close(CACHE_NAMESPACE);
- EasyMock.replay(cache);
- EasyMock.reset(underlyingStore);
- underlyingStore.close();
- EasyMock.replay(underlyingStore);
+ doThrow(new RuntimeException("Simulating an error on flush")).doNothing().when(cache).flush(CACHE_NAMESPACE);
assertThrows(RuntimeException.class, cachingStore::close);
- EasyMock.verify(cache, underlyingStore);
+ verify(cache).flush(CACHE_NAMESPACE);
+ verify(cache).close(CACHE_NAMESPACE);
+ verify(underlyingStore).close();
Review Comment:
I am ambivalent. My reasoning would be to use InOrder to confirm that records are consumed in order, much like what Bruno pointed out in https://github.com/apache/kafka/pull/12492#discussion_r944369076 rather than in this case. If you would like stronger assertions I can add them. What is your preference?
--
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
[GitHub] [kafka] clolov commented on a diff in pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
clolov commented on code in PR #12505:
URL: https://github.com/apache/kafka/pull/12505#discussion_r947652273
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/ChangeLoggingKeyValueBytesStoreTest.java:
##########
@@ -280,12 +276,9 @@ private StreamsConfig streamsConfigMock() {
final Map<String, Object> myValues = new HashMap<>();
myValues.put(InternalConfig.IQ_CONSISTENCY_OFFSET_VECTOR_ENABLED, true);
- expect(streamsConfig.originals()).andStubReturn(myValues);
- expect(streamsConfig.values()).andStubReturn(Collections.emptyMap());
- expect(streamsConfig.getString(StreamsConfig.APPLICATION_ID_CONFIG)).andStubReturn("add-id");
- expect(streamsConfig.defaultValueSerde()).andStubReturn(Serdes.ByteArray());
- expect(streamsConfig.defaultKeySerde()).andStubReturn(Serdes.ByteArray());
Review Comment:
I followed the same reasoning as in the other comment for removed stubs - Mockito reported these as extraneous and deleting them from EasyMock tests caused tests to still pass.
--
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
[GitHub] [kafka] mdedetrich commented on a diff in pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on code in PR #12505:
URL: https://github.com/apache/kafka/pull/12505#discussion_r947761921
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/ChangeLoggingKeyValueBytesStoreTest.java:
##########
@@ -280,12 +276,9 @@ private StreamsConfig streamsConfigMock() {
final Map<String, Object> myValues = new HashMap<>();
myValues.put(InternalConfig.IQ_CONSISTENCY_OFFSET_VECTOR_ENABLED, true);
- expect(streamsConfig.originals()).andStubReturn(myValues);
- expect(streamsConfig.values()).andStubReturn(Collections.emptyMap());
- expect(streamsConfig.getString(StreamsConfig.APPLICATION_ID_CONFIG)).andStubReturn("add-id");
- expect(streamsConfig.defaultValueSerde()).andStubReturn(Serdes.ByteArray());
- expect(streamsConfig.defaultKeySerde()).andStubReturn(Serdes.ByteArray());
Review Comment:
I can also confirm I had the same problem when migrating `ProcesserStateManagerTest` in https://github.com/apache/kafka/pull/12524, i.e. there was quite a few tests which had extra/unnecessary stubs/verifications which `MockitoJUnitRunner.StrictStubs` picked up.
--
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
[GitHub] [kafka] clolov commented on a diff in pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
clolov commented on code in PR #12505:
URL: https://github.com/apache/kafka/pull/12505#discussion_r947643109
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/KeyValueStoreBuilderTest.java:
##########
@@ -126,27 +121,15 @@ public void shouldThrowNullPointerIfInnerIsNull() {
Serdes.String(), new MockTime()));
}
- @Test
Review Comment:
Tagging a range of people in the hope to get an answer to this: @dguy (they seem to have created this file), @chia7712 (they seem to have added these tests), @cadonna (since this is a streams-related test file)
--
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
[GitHub] [kafka] clolov commented on a diff in pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
clolov commented on code in PR #12505:
URL: https://github.com/apache/kafka/pull/12505#discussion_r943639431
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/KeyValueStoreBuilderTest.java:
##########
@@ -126,27 +121,15 @@ public void shouldThrowNullPointerIfInnerIsNull() {
Serdes.String(), new MockTime()));
}
- @Test
Review Comment:
As far as I can tell from the code, these tests were testing something which isn't done in the code so I deleted them. If I am wrong I will return them.
--
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
[GitHub] [kafka] mdedetrich commented on pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on PR #12505:
URL: https://github.com/apache/kafka/pull/12505#issuecomment-1212822560
Thanks, will review today!
--
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
[GitHub] [kafka] clolov commented on a diff in pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
clolov commented on code in PR #12505:
URL: https://github.com/apache/kafka/pull/12505#discussion_r1011557825
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/CachingInMemoryKeyValueStoreTest.java:
##########
@@ -153,57 +154,48 @@ public void shouldAvoidFlushingDeletionsWithoutDirtyKeys() {
@Test
public void shouldCloseWrappedStoreAndCacheAfterErrorDuringCacheFlush() {
setUpCloseTests();
- EasyMock.reset(cache);
- cache.flush(CACHE_NAMESPACE);
- EasyMock.expectLastCall().andThrow(new RuntimeException("Simulating an error on flush"));
- EasyMock.replay(cache);
- EasyMock.reset(underlyingStore);
- underlyingStore.close();
- EasyMock.replay(underlyingStore);
+
+ doThrow(new RuntimeException("Simulating an error on flush")).when(cache).flush(CACHE_NAMESPACE);
assertThrows(RuntimeException.class, store::close);
- EasyMock.verify(cache, underlyingStore);
+
+ verify(cache).flush(eq(CACHE_NAMESPACE));
+ verify(cache).close(eq(CACHE_NAMESPACE));
+ verify(underlyingStore).close();
}
@Test
public void shouldCloseWrappedStoreAfterErrorDuringCacheClose() {
setUpCloseTests();
- EasyMock.reset(cache);
- cache.flush(CACHE_NAMESPACE);
- cache.close(CACHE_NAMESPACE);
- EasyMock.expectLastCall().andThrow(new RuntimeException("Simulating an error on close"));
- EasyMock.replay(cache);
- EasyMock.reset(underlyingStore);
- underlyingStore.close();
- EasyMock.replay(underlyingStore);
+
+ doThrow(new RuntimeException("Simulating an error on close")).when(cache).close(CACHE_NAMESPACE);
assertThrows(RuntimeException.class, store::close);
- EasyMock.verify(cache, underlyingStore);
+
+ verify(cache).flush(eq(CACHE_NAMESPACE));
+ verify(cache).close(eq(CACHE_NAMESPACE));
Review Comment:
Hopefully addressed in subsequent commits.
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/CachingInMemoryKeyValueStoreTest.java:
##########
@@ -153,57 +154,48 @@ public void shouldAvoidFlushingDeletionsWithoutDirtyKeys() {
@Test
public void shouldCloseWrappedStoreAndCacheAfterErrorDuringCacheFlush() {
setUpCloseTests();
- EasyMock.reset(cache);
- cache.flush(CACHE_NAMESPACE);
- EasyMock.expectLastCall().andThrow(new RuntimeException("Simulating an error on flush"));
- EasyMock.replay(cache);
- EasyMock.reset(underlyingStore);
- underlyingStore.close();
- EasyMock.replay(underlyingStore);
+
+ doThrow(new RuntimeException("Simulating an error on flush")).when(cache).flush(CACHE_NAMESPACE);
assertThrows(RuntimeException.class, store::close);
- EasyMock.verify(cache, underlyingStore);
+
+ verify(cache).flush(eq(CACHE_NAMESPACE));
+ verify(cache).close(eq(CACHE_NAMESPACE));
Review Comment:
Hopefully addressed in subsequent commits.
--
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
[GitHub] [kafka] clolov commented on a diff in pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
clolov commented on code in PR #12505:
URL: https://github.com/apache/kafka/pull/12505#discussion_r1011797179
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/CachingPersistentWindowStoreTest.java:
##########
@@ -1039,58 +1041,42 @@ public void shouldNotThrowInvalidBackwardRangeExceptionWithNegativeFromKey() {
@Test
public void shouldCloseCacheAndWrappedStoreAfterErrorDuringCacheFlush() {
setUpCloseTests();
- EasyMock.reset(cache);
- cache.flush(CACHE_NAMESPACE);
- EasyMock.expectLastCall().andThrow(new RuntimeException("Simulating an error on flush"));
- cache.close(CACHE_NAMESPACE);
- EasyMock.replay(cache);
- EasyMock.reset(underlyingStore);
- underlyingStore.close();
- EasyMock.replay(underlyingStore);
+ doThrow(new RuntimeException("Simulating an error on flush")).doNothing().when(cache).flush(CACHE_NAMESPACE);
assertThrows(RuntimeException.class, cachingStore::close);
- EasyMock.verify(cache, underlyingStore);
+ verify(cache).flush(CACHE_NAMESPACE);
+ verify(cache).close(CACHE_NAMESPACE);
+ verify(underlyingStore).close();
Review Comment:
Hopefully I have addressed this in subsequent commits.
--
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
[GitHub] [kafka] cadonna merged pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
cadonna merged PR #12505:
URL: https://github.com/apache/kafka/pull/12505
--
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
[GitHub] [kafka] clolov commented on pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
clolov commented on PR #12505:
URL: https://github.com/apache/kafka/pull/12505#issuecomment-1323460811
Yay 🎉 , thank you for your help in reviewing, approving and merging this @cadonna!
--
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
[GitHub] [kafka] cadonna commented on pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
cadonna commented on PR #12505:
URL: https://github.com/apache/kafka/pull/12505#issuecomment-1286760706
@clolov What is the status of this PR?
--
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
[GitHub] [kafka] clolov commented on a diff in pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
clolov commented on code in PR #12505:
URL: https://github.com/apache/kafka/pull/12505#discussion_r1011564112
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/CachingPersistentSessionStoreTest.java:
##########
@@ -258,58 +266,45 @@ public void shouldPutBackwardFetchAllKeysFromCache() {
@Test
public void shouldCloseWrappedStoreAndCacheAfterErrorDuringCacheFlush() {
setUpCloseTests();
- EasyMock.reset(cache);
- cache.flush(CACHE_NAMESPACE);
- EasyMock.expectLastCall().andThrow(new RuntimeException("Simulating an error on flush"));
- EasyMock.replay(cache);
- EasyMock.reset(underlyingStore);
- underlyingStore.close();
- EasyMock.replay(underlyingStore);
+ doThrow(new RuntimeException("Simulating an error on flush")).doNothing().when(cache).flush(anyString());
assertThrows(RuntimeException.class, cachingStore::close);
- EasyMock.verify(cache, underlyingStore);
+ verify(cache).flush(eq(CACHE_NAMESPACE));
+ verify(cache).close(eq(CACHE_NAMESPACE));
+ verify(underlyingStore).close();
}
@Test
public void shouldCloseWrappedStoreAfterErrorDuringCacheClose() {
setUpCloseTests();
- EasyMock.reset(cache);
- cache.flush(CACHE_NAMESPACE);
- cache.close(CACHE_NAMESPACE);
- EasyMock.expectLastCall().andThrow(new RuntimeException("Simulating an error on close"));
- EasyMock.replay(cache);
- EasyMock.reset(underlyingStore);
- underlyingStore.close();
- EasyMock.replay(underlyingStore);
+
Review Comment:
If you are referring to `setUpCloseTests` - it isn't used in all tests in the class, so I suspect the original author abstracted it in a method for ease-of-use. I don't think it will aid readability if we try to move it in the `@Before` method.
--
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
[GitHub] [kafka] clolov commented on a diff in pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
clolov commented on code in PR #12505:
URL: https://github.com/apache/kafka/pull/12505#discussion_r1011770027
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/CachingPersistentWindowStoreTest.java:
##########
@@ -119,29 +125,24 @@ public void closeStore() {
cachingStore.close();
}
- @SuppressWarnings("deprecation")
+ @SuppressWarnings({"deprecation", "unchecked"})
@Test
public void shouldDelegateDeprecatedInit() {
- final WindowStore<Bytes, byte[]> inner = EasyMock.mock(WindowStore.class);
+ final WindowStore<Bytes, byte[]> inner = mock(WindowStore.class);
Review Comment:
In these situations we sometimes use a mock and sometimes the real thing. For example, in most test cases we use the real thing
```
@Before
public void setUp() {
underlyingStore = new RocksDBWindowStore(bytesStore, false, WINDOW_SIZE);
...
cachingStore = new CachingWindowStore(underlyingStore, WINDOW_SIZE, SEGMENT_INTERVAL);
}
```
but for the test you picked we want a mock and defining it inline seems a bit cleaner.
--
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
[GitHub] [kafka] clolov commented on pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
clolov commented on PR #12505:
URL: https://github.com/apache/kafka/pull/12505#issuecomment-1241764136
Heya @cadonna and thank you very much for the review! I will address your comments over the next few days.
--
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
[GitHub] [kafka] dplavcic commented on a diff in pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
dplavcic commented on code in PR #12505:
URL: https://github.com/apache/kafka/pull/12505#discussion_r944784869
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/ChangeLoggingKeyValueBytesStoreTest.java:
##########
@@ -280,12 +276,9 @@ private StreamsConfig streamsConfigMock() {
final Map<String, Object> myValues = new HashMap<>();
myValues.put(InternalConfig.IQ_CONSISTENCY_OFFSET_VECTOR_ENABLED, true);
- expect(streamsConfig.originals()).andStubReturn(myValues);
- expect(streamsConfig.values()).andStubReturn(Collections.emptyMap());
- expect(streamsConfig.getString(StreamsConfig.APPLICATION_ID_CONFIG)).andStubReturn("add-id");
- expect(streamsConfig.defaultValueSerde()).andStubReturn(Serdes.ByteArray());
- expect(streamsConfig.defaultKeySerde()).andStubReturn(Serdes.ByteArray());
Review Comment:
Are these ever invoked? Should we stub and/or verify these calls? 🤔
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/CachingPersistentWindowStoreTest.java:
##########
@@ -119,29 +126,24 @@ public void closeStore() {
cachingStore.close();
}
- @SuppressWarnings("deprecation")
+ @SuppressWarnings({"deprecation", "unchecked"})
@Test
public void shouldDelegateDeprecatedInit() {
- final WindowStore<Bytes, byte[]> inner = EasyMock.mock(WindowStore.class);
+ final WindowStore<Bytes, byte[]> inner = mock(WindowStore.class);
final CachingWindowStore outer = new CachingWindowStore(inner, WINDOW_SIZE, SEGMENT_INTERVAL);
- EasyMock.expect(inner.name()).andStubReturn("store");
- inner.init((ProcessorContext) context, outer);
- EasyMock.expectLastCall();
- EasyMock.replay(inner);
+ when(inner.name()).thenReturn("store");
outer.init((ProcessorContext) context, outer);
- EasyMock.verify(inner);
+ verify(inner).init(eq((ProcessorContext) context), eq(outer));
Review Comment:
Can you please double check if `eq()` matchers are needed here?
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/CachingPersistentWindowStoreTest.java:
##########
@@ -1039,58 +1041,42 @@ public void shouldNotThrowInvalidBackwardRangeExceptionWithNegativeFromKey() {
@Test
public void shouldCloseCacheAndWrappedStoreAfterErrorDuringCacheFlush() {
setUpCloseTests();
- EasyMock.reset(cache);
- cache.flush(CACHE_NAMESPACE);
- EasyMock.expectLastCall().andThrow(new RuntimeException("Simulating an error on flush"));
- cache.close(CACHE_NAMESPACE);
- EasyMock.replay(cache);
- EasyMock.reset(underlyingStore);
- underlyingStore.close();
- EasyMock.replay(underlyingStore);
+ doThrow(new RuntimeException("Simulating an error on flush")).doNothing().when(cache).flush(CACHE_NAMESPACE);
assertThrows(RuntimeException.class, cachingStore::close);
- EasyMock.verify(cache, underlyingStore);
+ verify(cache).flush(CACHE_NAMESPACE);
+ verify(cache).close(CACHE_NAMESPACE);
+ verify(underlyingStore).close();
Review Comment:
Do you think there are any additional benefits if we use Mockito's `InOrder` interface to verify these method calls? (same question applies to other changed `verify` calls in this class)
<img width="597" alt="image" src="https://user-images.githubusercontent.com/9079844/184432082-93586a57-0bef-46a2-b39b-1d58062123e3.png">
Link to docs:
- https://javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/InOrder.html
--
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
[GitHub] [kafka] clolov commented on pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
clolov commented on PR #12505:
URL: https://github.com/apache/kafka/pull/12505#issuecomment-1212874952
I am aware checkstyle is complaining about some indentation which I will fix later throughout the day.
--
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
[GitHub] [kafka] clolov commented on a diff in pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
clolov commented on code in PR #12505:
URL: https://github.com/apache/kafka/pull/12505#discussion_r947650090
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/StreamThreadStateStoreProviderTest.java:
##########
@@ -449,22 +454,18 @@ private StreamTask createStreamsTask(final StreamsConfig streamsConfig,
new TopologyConfig(null, streamsConfig, new Properties()).getTaskConfig(),
streamsMetrics,
stateDirectory,
- EasyMock.createNiceMock(ThreadCache.class),
+ mock(ThreadCache.class),
new MockTime(),
stateManager,
recordCollector,
context, logContext);
}
private void mockThread(final boolean initialized) {
- EasyMock.expect(threadMock.isRunning()).andReturn(initialized);
- EasyMock.expect(threadMock.allTasks()).andStubReturn(tasks);
- EasyMock.expect(threadMock.activeTaskMap()).andStubReturn(tasks);
Review Comment:
Yes, apologies, I will add comments when I delete stubs like this in future pull requests. Mockito reported these as unused and when I commented them out in the original EasyMock tests the tests still passed.
--
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
[GitHub] [kafka] mdedetrich commented on a diff in pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on code in PR #12505:
URL: https://github.com/apache/kafka/pull/12505#discussion_r947762500
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/CachingPersistentWindowStoreTest.java:
##########
@@ -119,29 +126,24 @@ public void closeStore() {
cachingStore.close();
}
- @SuppressWarnings("deprecation")
+ @SuppressWarnings({"deprecation", "unchecked"})
@Test
public void shouldDelegateDeprecatedInit() {
- final WindowStore<Bytes, byte[]> inner = EasyMock.mock(WindowStore.class);
+ final WindowStore<Bytes, byte[]> inner = mock(WindowStore.class);
final CachingWindowStore outer = new CachingWindowStore(inner, WINDOW_SIZE, SEGMENT_INTERVAL);
- EasyMock.expect(inner.name()).andStubReturn("store");
- inner.init((ProcessorContext) context, outer);
- EasyMock.expectLastCall();
- EasyMock.replay(inner);
+ when(inner.name()).thenReturn("store");
outer.init((ProcessorContext) context, outer);
- EasyMock.verify(inner);
+ verify(inner).init(eq((ProcessorContext) context), eq(outer));
Review Comment:
Also experienced same with https://github.com/apache/kafka/pull/12524, `eq` was either not necessary or even caused tests to break
--
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
[GitHub] [kafka] mdedetrich commented on pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on PR #12505:
URL: https://github.com/apache/kafka/pull/12505#issuecomment-1217827983
@clolov Sorry for not reviewing earlier, I ended up doing some work myself for the EasyMock to Mockito migration so I have better context/background info. Will review the PR later today.
--
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
[GitHub] [kafka] mdedetrich commented on a diff in pull request #12505: KAFKA-14133: Replace EasyMock with Mockito in streams tests
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on code in PR #12505:
URL: https://github.com/apache/kafka/pull/12505#discussion_r947944463
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/KeyValueStoreBuilderTest.java:
##########
@@ -126,27 +121,15 @@ public void shouldThrowNullPointerIfInnerIsNull() {
Serdes.String(), new MockTime()));
}
- @Test
Review Comment:
> should we add null check in the KeyValueStoreBuilder constructor?
Does it make sense to fix this in another ticket/PR? For me at least it makes sense to not change any behaviour that isn't test related so that if there are any future problems its clear that it wasn't due to us changing tests.
--
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