cadonna commented on code in PR #14716:
URL: https://github.com/apache/kafka/pull/14716#discussion_r1552378325
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/StreamTaskTest.java:
##########
@@ -309,49 +300,49 @@ public void
shouldThrowLockExceptionIfFailedToLockStateDirectory() throws IOExce
@Test
public void shouldNotAttemptToLockIfNoStores() {
- stateDirectory = EasyMock.createNiceMock(StateDirectory.class);
- EasyMock.replay(stateDirectory);
+ stateDirectory = mock(StateDirectory.class);
task = createStatelessTask(createConfig("100"));
task.initializeIfNeeded();
// should fail if lock is called
- EasyMock.verify(stateDirectory);
+ verify(stateDirectory, never()).lock(any());
}
@Test
public void
shouldAttemptToDeleteStateDirectoryWhenCloseDirtyAndEosEnabled() throws
IOException {
- final IMocksControl ctrl = EasyMock.createStrictControl();
- final ProcessorStateManager stateManager =
ctrl.createMock(ProcessorStateManager.class);
-
EasyMock.expect(stateManager.taskType()).andStubReturn(TaskType.ACTIVE);
- stateDirectory = ctrl.createMock(StateDirectory.class);
+ final ProcessorStateManager stateManager =
mock(ProcessorStateManager.class);
+ when(stateManager.taskType()).thenReturn(TaskType.ACTIVE);
+ stateDirectory = mock(StateDirectory.class);
- stateManager.registerGlobalStateStores(emptyList());
- EasyMock.expectLastCall();
+ doNothing().when(stateManager).registerGlobalStateStores(emptyList());
Review Comment:
Do we really need this stub?
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/StreamTaskTest.java:
##########
@@ -309,49 +300,49 @@ public void
shouldThrowLockExceptionIfFailedToLockStateDirectory() throws IOExce
@Test
public void shouldNotAttemptToLockIfNoStores() {
- stateDirectory = EasyMock.createNiceMock(StateDirectory.class);
- EasyMock.replay(stateDirectory);
+ stateDirectory = mock(StateDirectory.class);
task = createStatelessTask(createConfig("100"));
task.initializeIfNeeded();
// should fail if lock is called
- EasyMock.verify(stateDirectory);
+ verify(stateDirectory, never()).lock(any());
}
@Test
public void
shouldAttemptToDeleteStateDirectoryWhenCloseDirtyAndEosEnabled() throws
IOException {
- final IMocksControl ctrl = EasyMock.createStrictControl();
- final ProcessorStateManager stateManager =
ctrl.createMock(ProcessorStateManager.class);
-
EasyMock.expect(stateManager.taskType()).andStubReturn(TaskType.ACTIVE);
- stateDirectory = ctrl.createMock(StateDirectory.class);
+ final ProcessorStateManager stateManager =
mock(ProcessorStateManager.class);
+ when(stateManager.taskType()).thenReturn(TaskType.ACTIVE);
+ stateDirectory = mock(StateDirectory.class);
- stateManager.registerGlobalStateStores(emptyList());
- EasyMock.expectLastCall();
+ doNothing().when(stateManager).registerGlobalStateStores(emptyList());
- EasyMock.expect(stateManager.taskId()).andReturn(taskId);
+ when(stateManager.taskId()).thenReturn(taskId);
- EasyMock.expect(stateDirectory.lock(taskId)).andReturn(true);
+ when(stateDirectory.lock(taskId)).thenReturn(true);
- stateManager.close();
- EasyMock.expectLastCall();
+ doNothing().when(stateManager).close();
Review Comment:
Do we really need this stub?
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/StreamTaskTest.java:
##########
@@ -309,49 +300,49 @@ public void
shouldThrowLockExceptionIfFailedToLockStateDirectory() throws IOExce
@Test
public void shouldNotAttemptToLockIfNoStores() {
- stateDirectory = EasyMock.createNiceMock(StateDirectory.class);
- EasyMock.replay(stateDirectory);
+ stateDirectory = mock(StateDirectory.class);
task = createStatelessTask(createConfig("100"));
task.initializeIfNeeded();
// should fail if lock is called
- EasyMock.verify(stateDirectory);
+ verify(stateDirectory, never()).lock(any());
}
@Test
public void
shouldAttemptToDeleteStateDirectoryWhenCloseDirtyAndEosEnabled() throws
IOException {
- final IMocksControl ctrl = EasyMock.createStrictControl();
- final ProcessorStateManager stateManager =
ctrl.createMock(ProcessorStateManager.class);
-
EasyMock.expect(stateManager.taskType()).andStubReturn(TaskType.ACTIVE);
- stateDirectory = ctrl.createMock(StateDirectory.class);
+ final ProcessorStateManager stateManager =
mock(ProcessorStateManager.class);
+ when(stateManager.taskType()).thenReturn(TaskType.ACTIVE);
+ stateDirectory = mock(StateDirectory.class);
- stateManager.registerGlobalStateStores(emptyList());
- EasyMock.expectLastCall();
+ doNothing().when(stateManager).registerGlobalStateStores(emptyList());
- EasyMock.expect(stateManager.taskId()).andReturn(taskId);
+ when(stateManager.taskId()).thenReturn(taskId);
- EasyMock.expect(stateDirectory.lock(taskId)).andReturn(true);
+ when(stateDirectory.lock(taskId)).thenReturn(true);
- stateManager.close();
- EasyMock.expectLastCall();
+ doNothing().when(stateManager).close();
// The `baseDir` will be accessed when attempting to delete the state
store.
-
EasyMock.expect(stateManager.baseDir()).andReturn(TestUtils.tempDirectory("state_store"));
+
when(stateManager.baseDir()).thenReturn(TestUtils.tempDirectory("state_store"));
- stateDirectory.unlock(taskId);
- EasyMock.expectLastCall();
+ doNothing().when(stateDirectory).unlock(taskId);
Review Comment:
Do we really need this stub?
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/StreamTaskTest.java:
##########
@@ -1903,42 +1844,36 @@ public void shouldThrowIfPostCommittingOnIllegalState()
{
@Test
public void shouldSkipCheckpointingSuspendedCreatedTask() {
- stateManager.checkpoint();
- EasyMock.expectLastCall().andThrow(new AssertionError("Should not have
tried to checkpoint"));
-
EasyMock.expect(recordCollector.offsets()).andReturn(emptyMap()).anyTimes();
- EasyMock.replay(stateManager, recordCollector);
+ when(recordCollector.offsets()).thenReturn(emptyMap());
Review Comment:
Is this stub needed since it returns an empty map?
Please also check other occurrences of such stubs.
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/StreamTaskTest.java:
##########
@@ -365,11 +356,10 @@ public void
shouldResetOffsetsToLastCommittedForSpecifiedPartitions() {
consumer.seek(partition1, 10L);
consumer.seek(partition2, 15L);
+ @SuppressWarnings("unchecked")
final java.util.function.Consumer<Set<TopicPartition>> resetter =
- EasyMock.mock(java.util.function.Consumer.class);
- resetter.accept(Collections.emptySet());
- EasyMock.expectLastCall();
- EasyMock.replay(resetter);
+ mock(java.util.function.Consumer.class);
+ doNothing().when(resetter).accept(Collections.emptySet());
Review Comment:
Again, I do not think we need this stub with Mockito.
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/StreamTaskTest.java:
##########
@@ -1568,10 +1548,8 @@ public void
shouldNotShareHeadersBetweenPunctuateIterations() {
@Test
public void shouldWrapKafkaExceptionWithStreamsExceptionWhenProcess() {
-
EasyMock.expect(stateManager.changelogPartitions()).andReturn(Collections.emptySet()).anyTimes();
-
EasyMock.expect(stateManager.changelogOffsets()).andReturn(emptyMap()).anyTimes();
-
EasyMock.expect(recordCollector.offsets()).andReturn(emptyMap()).anyTimes();
- EasyMock.replay(stateManager, recordCollector);
+ when(stateManager.changelogOffsets()).thenReturn(emptyMap());
+ when(recordCollector.offsets()).thenReturn(emptyMap());
Review Comment:
Do we even need those? What is the default return value for a map? I guess
an empty map. Could you please verify?
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/StreamTaskTest.java:
##########
@@ -309,49 +300,49 @@ public void
shouldThrowLockExceptionIfFailedToLockStateDirectory() throws IOExce
@Test
public void shouldNotAttemptToLockIfNoStores() {
- stateDirectory = EasyMock.createNiceMock(StateDirectory.class);
- EasyMock.replay(stateDirectory);
+ stateDirectory = mock(StateDirectory.class);
task = createStatelessTask(createConfig("100"));
task.initializeIfNeeded();
// should fail if lock is called
- EasyMock.verify(stateDirectory);
+ verify(stateDirectory, never()).lock(any());
}
@Test
public void
shouldAttemptToDeleteStateDirectoryWhenCloseDirtyAndEosEnabled() throws
IOException {
- final IMocksControl ctrl = EasyMock.createStrictControl();
- final ProcessorStateManager stateManager =
ctrl.createMock(ProcessorStateManager.class);
-
EasyMock.expect(stateManager.taskType()).andStubReturn(TaskType.ACTIVE);
- stateDirectory = ctrl.createMock(StateDirectory.class);
+ final ProcessorStateManager stateManager =
mock(ProcessorStateManager.class);
+ when(stateManager.taskType()).thenReturn(TaskType.ACTIVE);
+ stateDirectory = mock(StateDirectory.class);
- stateManager.registerGlobalStateStores(emptyList());
- EasyMock.expectLastCall();
+ doNothing().when(stateManager).registerGlobalStateStores(emptyList());
- EasyMock.expect(stateManager.taskId()).andReturn(taskId);
+ when(stateManager.taskId()).thenReturn(taskId);
Review Comment:
Do we really need to redefine this stub?
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/StreamTaskTest.java:
##########
@@ -309,49 +300,49 @@ public void
shouldThrowLockExceptionIfFailedToLockStateDirectory() throws IOExce
@Test
public void shouldNotAttemptToLockIfNoStores() {
- stateDirectory = EasyMock.createNiceMock(StateDirectory.class);
- EasyMock.replay(stateDirectory);
+ stateDirectory = mock(StateDirectory.class);
task = createStatelessTask(createConfig("100"));
task.initializeIfNeeded();
// should fail if lock is called
- EasyMock.verify(stateDirectory);
+ verify(stateDirectory, never()).lock(any());
}
@Test
public void
shouldAttemptToDeleteStateDirectoryWhenCloseDirtyAndEosEnabled() throws
IOException {
- final IMocksControl ctrl = EasyMock.createStrictControl();
- final ProcessorStateManager stateManager =
ctrl.createMock(ProcessorStateManager.class);
-
EasyMock.expect(stateManager.taskType()).andStubReturn(TaskType.ACTIVE);
- stateDirectory = ctrl.createMock(StateDirectory.class);
+ final ProcessorStateManager stateManager =
mock(ProcessorStateManager.class);
+ when(stateManager.taskType()).thenReturn(TaskType.ACTIVE);
Review Comment:
Do we really need to redefine this mock?
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/StreamTaskTest.java:
##########
@@ -309,49 +300,49 @@ public void
shouldThrowLockExceptionIfFailedToLockStateDirectory() throws IOExce
@Test
public void shouldNotAttemptToLockIfNoStores() {
- stateDirectory = EasyMock.createNiceMock(StateDirectory.class);
- EasyMock.replay(stateDirectory);
+ stateDirectory = mock(StateDirectory.class);
task = createStatelessTask(createConfig("100"));
task.initializeIfNeeded();
// should fail if lock is called
- EasyMock.verify(stateDirectory);
+ verify(stateDirectory, never()).lock(any());
}
@Test
public void
shouldAttemptToDeleteStateDirectoryWhenCloseDirtyAndEosEnabled() throws
IOException {
- final IMocksControl ctrl = EasyMock.createStrictControl();
- final ProcessorStateManager stateManager =
ctrl.createMock(ProcessorStateManager.class);
-
EasyMock.expect(stateManager.taskType()).andStubReturn(TaskType.ACTIVE);
- stateDirectory = ctrl.createMock(StateDirectory.class);
+ final ProcessorStateManager stateManager =
mock(ProcessorStateManager.class);
+ when(stateManager.taskType()).thenReturn(TaskType.ACTIVE);
+ stateDirectory = mock(StateDirectory.class);
- stateManager.registerGlobalStateStores(emptyList());
- EasyMock.expectLastCall();
+ doNothing().when(stateManager).registerGlobalStateStores(emptyList());
- EasyMock.expect(stateManager.taskId()).andReturn(taskId);
+ when(stateManager.taskId()).thenReturn(taskId);
- EasyMock.expect(stateDirectory.lock(taskId)).andReturn(true);
+ when(stateDirectory.lock(taskId)).thenReturn(true);
- stateManager.close();
- EasyMock.expectLastCall();
+ doNothing().when(stateManager).close();
// The `baseDir` will be accessed when attempting to delete the state
store.
-
EasyMock.expect(stateManager.baseDir()).andReturn(TestUtils.tempDirectory("state_store"));
+
when(stateManager.baseDir()).thenReturn(TestUtils.tempDirectory("state_store"));
- stateDirectory.unlock(taskId);
- EasyMock.expectLastCall();
+ doNothing().when(stateDirectory).unlock(taskId);
- ctrl.checkOrder(true);
- ctrl.replay();
+ final InOrder inOrder = inOrder(stateManager, stateDirectory);
task = createStatefulTask(createConfig(StreamsConfig.EXACTLY_ONCE_V2,
"100"), true, stateManager);
task.suspend();
task.closeDirty();
task = null;
- ctrl.verify();
+ inOrder.verify(stateManager).taskType();
+ inOrder.verify(stateManager).registerGlobalStateStores(emptyList());
+ inOrder.verify(stateManager).taskId();
+ inOrder.verify(stateDirectory).lock(taskId);
+ inOrder.verify(stateManager).close();
+ inOrder.verify(stateManager).baseDir();
+ inOrder.verify(stateDirectory).unlock(taskId);
Review Comment:
I think these verifications cover the `doNothing()...` stubs from above and
we do not really need those stubs. Could you verify?
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/StreamTaskTest.java:
##########
@@ -1798,8 +1746,6 @@ public void
shouldCloseStateManagerEvenDuringFailureOnUncleanTaskClose() {
assertThrows(RuntimeException.class, () -> task.suspend());
task.closeDirty();
-
- EasyMock.verify(stateManager);
Review Comment:
Verifications are missing in the new code. For example,
`stateManager.close()`.
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/StreamTaskTest.java:
##########
@@ -514,8 +496,6 @@ public void
shouldTransitToRestoringThenRunningAfterCreation() throws IOExceptio
assertEquals(RUNNING, task.state());
assertTrue(source1.initialized);
assertTrue(source2.initialized);
-
- EasyMock.verify(stateDirectory);
Review Comment:
I think some verifications are missing here, aren't they? For example,
`stateManager.registerStore()`.
--
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]
