[GitHub] [kafka] divijvaidya commented on a diff in pull request #13681: KAFKA-14133: Migrate ActiveTaskCreator mock in TaskManagerTest to Mockito

["divijvaidya (via GitHub)" <gi...@apache.org>]

divijvaidya commented on code in PR #13681:
URL: https://github.com/apache/kafka/pull/13681#discussion_r1187177289


##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##########


Review Comment:
   we still seem to have easymock imports. Can you please remove them and replace with Mockito as well?
   
   ```
   import static org.easymock.EasyMock.anyObject;
   import static org.easymock.EasyMock.eq;
   import static org.easymock.EasyMock.expect;
   import static org.easymock.EasyMock.expectLastCall;
   import static org.easymock.EasyMock.replay;
   import static org.easymock.EasyMock.reset;
   import static org.easymock.EasyMock.resetToStrict;
   import static org.easymock.EasyMock.verify;
   ````



##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##########
@@ -291,16 +292,16 @@ public void shouldPrepareActiveTaskInStateUpdaterToBeRecycled() {
         final TasksRegistry tasks = Mockito.mock(TasksRegistry.class);
         final TaskManager taskManager = setUpTaskManager(ProcessingMode.AT_LEAST_ONCE, tasks, true);
         when(stateUpdater.getTasks()).thenReturn(mkSet(activeTaskToRecycle));
-        expect(activeTaskCreator.createTasks(consumer, Collections.emptyMap())).andReturn(emptySet());
         expect(standbyTaskCreator.createTasks(Collections.emptyMap())).andReturn(emptySet());
-        replay(activeTaskCreator, standbyTaskCreator);
+        replay(standbyTaskCreator);

Review Comment:
   Mockito doesn't require to call `replay` (unlike EasyMock)
   
   (same comment for rest of the places in this file where we are using replay)



##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##########
@@ -291,16 +292,16 @@ public void shouldPrepareActiveTaskInStateUpdaterToBeRecycled() {
         final TasksRegistry tasks = Mockito.mock(TasksRegistry.class);
         final TaskManager taskManager = setUpTaskManager(ProcessingMode.AT_LEAST_ONCE, tasks, true);
         when(stateUpdater.getTasks()).thenReturn(mkSet(activeTaskToRecycle));
-        expect(activeTaskCreator.createTasks(consumer, Collections.emptyMap())).andReturn(emptySet());
         expect(standbyTaskCreator.createTasks(Collections.emptyMap())).andReturn(emptySet());
-        replay(activeTaskCreator, standbyTaskCreator);
+        replay(standbyTaskCreator);
 
         taskManager.handleAssignment(
             Collections.emptyMap(),
             mkMap(mkEntry(activeTaskToRecycle.id(), activeTaskToRecycle.inputPartitions()))
         );
 
-        verify(activeTaskCreator, standbyTaskCreator);
+        verify(standbyTaskCreator);

Review Comment:
   We probably want to verify a method invocation here. If you use Mockito's verify() here instead of EasyMock, you might see a compilation error.
   
   (same comment for rest of the usage of verify(mock) in this PR)



##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##########
@@ -2579,7 +2563,9 @@ public void shouldUpdateInputPartitionsAfterRebalance() {
         assertThat(taskManager.tryToCompleteRestoration(time.milliseconds(), null), is(true));
         assertThat(task00.state(), is(Task.State.RUNNING));
         assertEquals(newPartitionsSet, task00.inputPartitions());
-        verify(activeTaskCreator, consumer, changeLogReader);
+        verify(consumer, changeLogReader);
+        Mockito.verify(activeTaskCreator).createTasks(any(), Mockito.eq(taskId00Assignment));

Review Comment:
   Mockito has a mode called STRICT_STUBS which fails the test if a defined stub is not invoked. We can greatly simplify code using that annotation since we don't have to do both `when()` and `verify()`. Using `when()` would suffice since the test will fail if the stub is not used (or using verify() would suffice for cases with void return). We use STRICT_STUBS in a bunch of places in Kafka code such as [this](https://github.com/apache/kafka/blob/347238948b86882a47faee4a2916d1b01333d95f/streams/src/test/java/org/apache/kafka/streams/state/internals/metrics/RocksDBMetricsRecorderTest.java#L60).
   
   Please consider using it in this file as it will greatly remove boilerplate code verification.
   
   



-- 
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