cadonna commented on code in PR #12524:
URL: https://github.com/apache/kafka/pull/12524#discussion_r968474491
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/ProcessorStateManagerTest.java:
##########
@@ -308,29 +300,21 @@ public void shouldRestoreTimestampedStoreWithConverter() {
@Test
public void shouldUnregisterChangelogsDuringClose() {
final ProcessorStateManager stateMgr =
getStateManager(Task.TaskType.ACTIVE);
- reset(storeMetadata);
- final StateStore store = EasyMock.createMock(StateStore.class);
-
expect(storeMetadata.changelogPartition()).andStubReturn(persistentStorePartition);
- expect(storeMetadata.store()).andStubReturn(store);
- expect(store.name()).andStubReturn(persistentStoreName);
-
- context.uninitialize();
- store.init((StateStoreContext) context, store);
- replay(storeMetadata, context, store);
+ final StateStore store = mock(StateStore.class);
+ when(store.name()).thenReturn(persistentStoreName);
stateMgr.registerStateStores(singletonList(store), context);
- verify(context, store);
+ verify(context).uninitialize();
+ verify(store).init((StateStoreContext) context, store);
stateMgr.registerStore(store, noopStateRestoreCallback, null);
assertTrue(changelogReader.isPartitionRegistered(persistentStorePartition));
reset(store);
- expect(store.name()).andStubReturn(persistentStoreName);
- store.close();
- replay(store);
+ when(store.name()).thenReturn(persistentStoreName);
Review Comment:
Is the `reset()` and the stubbing of `store.name()` still needed? The
stubbing is the same as on line 304. Since we are not doing a new replay, I am
not sure that we still need the `reset()` with Mockito. You think you can moves
all `verify()` to the end of the method.
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/StandbyTaskTest.java:
##########
@@ -195,12 +196,11 @@ public void shouldTransitToRunningAfterInitialization() {
assertEquals(RUNNING, task.state());
- EasyMock.verify(stateManager);
+ verify(stateManager, atLeastOnce()).registerStateStores(any(), any());
Review Comment:
```suggestion
verify(stateManager).registerStateStores(any(), any());
```
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/StandbyTaskTest.java:
##########
@@ -212,31 +212,29 @@ public void shouldThrowIfCommittingOnIllegalState() {
@Test
public void shouldAlwaysCheckpointStateIfEnforced() {
stateManager.flush();
- EasyMock.expectLastCall().once();
+ verify(stateManager, times(1)).flush();
Review Comment:
Again, you make a call on the mock and verify that you made the call. In
EasyMock you need to make the calls on the mock to specify what calls you
expect. The call to `replay()` ends the specification phase. When you
`verify()`, you verify that the call under test indeed called to methods you
specified.
This is not the case in Mockito. In Mockito the mocks record the calls and
you verify what was called. Making calls on the mock directly in the test
method does not make sense. The methods of the mocks should be called within
the call under test.
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/StandbyTaskTest.java:
##########
@@ -212,31 +212,29 @@ public void shouldThrowIfCommittingOnIllegalState() {
@Test
public void shouldAlwaysCheckpointStateIfEnforced() {
stateManager.flush();
- EasyMock.expectLastCall().once();
+ verify(stateManager, times(1)).flush();
stateManager.checkpoint();
- EasyMock.expectLastCall().once();
-
EasyMock.expect(stateManager.changelogOffsets()).andReturn(Collections.emptyMap()).anyTimes();
- EasyMock.replay(stateManager);
+ verify(stateManager, times(1)).checkpoint();
+
when(stateManager.changelogOffsets()).thenReturn(Collections.emptyMap());
Review Comment:
Again here.
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/StandbyTaskTest.java:
##########
@@ -177,10 +181,7 @@ public void
shouldThrowLockExceptionIfFailedToLockStateDirectory() throws IOExce
@Test
public void shouldTransitToRunningAfterInitialization() {
-
EasyMock.expect(stateManager.changelogOffsets()).andStubReturn(Collections.emptyMap());
- stateManager.registerStateStores(EasyMock.anyObject(),
EasyMock.anyObject());
-
EasyMock.expect(stateManager.changelogPartitions()).andReturn(Collections.emptySet()).anyTimes();
- EasyMock.replay(stateManager);
+ stateManager.registerStateStores(any(), any());
Review Comment:
This is also a call on the mock which makes the verification on line 199
always true, independently of the call under test.
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/StandbyTaskTest.java:
##########
@@ -212,31 +212,29 @@ public void shouldThrowIfCommittingOnIllegalState() {
@Test
public void shouldAlwaysCheckpointStateIfEnforced() {
stateManager.flush();
- EasyMock.expectLastCall().once();
+ verify(stateManager, times(1)).flush();
stateManager.checkpoint();
- EasyMock.expectLastCall().once();
-
EasyMock.expect(stateManager.changelogOffsets()).andReturn(Collections.emptyMap()).anyTimes();
- EasyMock.replay(stateManager);
+ verify(stateManager, times(1)).checkpoint();
+
when(stateManager.changelogOffsets()).thenReturn(Collections.emptyMap());
task = createStandbyTask();
task.initializeIfNeeded();
task.maybeCheckpoint(true);
- EasyMock.verify(stateManager);
+ verify(stateManager, atLeastOnce()).flush();
Review Comment:
```suggestion
verify(stateManager).flush();
```
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/ProcessorStateManagerTest.java:
##########
@@ -360,12 +339,11 @@ public void shouldRecycleStoreAndReregisterChangelog() {
reset(context, store);
context.uninitialize();
Review Comment:
I think you should remove this line. It was a verification in EasyMock and
now with Mockito it is a call on the mock, which does not make a lot of sense.
You make the call and in the next line you immediately verify that you made the
call. Instead you should verify the call that is done within
`stateMgr.registerStateStores(singletonList(store), context)`. Additionally to
remove `context.uninitialize();` you need to move
`verify(context).uninitialize();` below
`stateMgr.registerStateStores(singletonList(store), context)`.
--
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
