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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
