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

2023-05-31 Thread via GitHub 


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


##
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##
@@ -3137,13 +3102,11 @@ public Set changelogPartitions() {
 }
 };
 
-expect(activeTaskCreator.createTasks(anyObject(), 
eq(assignment))).andStubReturn(singletonList(task00));
-activeTaskCreator.closeAndRemoveTaskProducerIfNeeded(eq(taskId00));
-expectLastCall();
-activeTaskCreator.closeThreadProducerIfNeeded();
-expectLastCall().andThrow(new RuntimeException("whatever"));
+when(activeTaskCreator.createTasks(any(), 
Mockito.eq(assignment))).thenReturn(singletonList(task00));
+doThrow(new 
RuntimeException("whatever")).when(activeTaskCreator).closeThreadProducerIfNeeded();
 
expect(standbyTaskCreator.createTasks(eq(emptyMap(.andStubReturn(emptyList());
-replay(activeTaskCreator, standbyTaskCreator);
+
expect(standbyTaskCreator.createTasks(eq(emptyMap(.andStubReturn(emptyList());

Review Comment:
   removed duplicate



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

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

2023-05-31 Thread via GitHub 


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


##
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##
@@ -3636,9 +3586,9 @@ public void shouldCommitViaConsumerIfEosDisabled() {
 @Test
 public void shouldCommitViaProducerIfEosAlphaEnabled() {
 final StreamsProducer producer = EasyMock.mock(StreamsProducer.class);
-
expect(activeTaskCreator.streamsProducerForTask(anyObject(TaskId.class)))
-.andReturn(producer)
-.andReturn(producer);
+when(activeTaskCreator.streamsProducerForTask(any(TaskId.class)))
+.thenReturn(producer)
+.thenReturn(producer);

Review Comment:
   updated, thank you.



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

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

2023-05-31 Thread via GitHub 


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


##
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##
@@ -4448,10 +4396,10 @@ public void 
shouldNotFailForTimeoutExceptionOnCommitWithEosAlpha() {
 final TaskManager taskManager = 
setUpTaskManager(ProcessingMode.EXACTLY_ONCE_ALPHA, tasks, false);
 
 final StreamsProducer producer = mock(StreamsProducer.class);
-
expect(activeTaskCreator.streamsProducerForTask(anyObject(TaskId.class)))
-.andReturn(producer)
-.andReturn(producer)
-.andReturn(producer);
+when(activeTaskCreator.streamsProducerForTask(any(TaskId.class)))
+.thenReturn(producer)
+.thenReturn(producer)

Review Comment:
   updated, thank you



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

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

2023-05-12 Thread via GitHub 


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


##
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##
@@ -845,17 +847,16 @@ public void shouldRecycleTasksRemovedFromStateUpdater() {
 
when(tasks.removePendingTaskToRecycle(task00.id())).thenReturn(taskId00Partitions);
 
when(tasks.removePendingTaskToRecycle(task01.id())).thenReturn(taskId01Partitions);
 taskManager = 
setUpTaskManager(StreamsConfigUtils.ProcessingMode.AT_LEAST_ONCE, tasks, true);
-expect(activeTaskCreator.createActiveTaskFromStandby(eq(task01), 
eq(taskId01Partitions), eq(consumer)))
-.andStubReturn(task01Converted);
-activeTaskCreator.closeAndRemoveTaskProducerIfNeeded(anyObject());
-expectLastCall().once();
+when(activeTaskCreator.createActiveTaskFromStandby(Mockito.eq(task01), 
Mockito.eq(taskId01Partitions),
+Mockito.eq(consumer))).thenReturn(task01Converted);
 expect(standbyTaskCreator.createStandbyTaskFromActive(eq(task00), 
eq(taskId00Partitions)))
 .andStubReturn(task00Converted);
-replay(activeTaskCreator, standbyTaskCreator);
+replay(standbyTaskCreator);
 
 taskManager.checkStateUpdater(time.milliseconds(), noOpResetter);
 
-verify(activeTaskCreator, standbyTaskCreator);
+verify(standbyTaskCreator);
+Mockito.verify(activeTaskCreator, 
times(1)).closeAndRemoveTaskProducerIfNeeded(any());

Review Comment:
   Yes make sense - updated



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

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

2023-05-12 Thread via GitHub 


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


##
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##
@@ -767,20 +770,19 @@ public void 
shouldAssignMultipleTasksInTasksRegistryWithStateUpdaterEnabled() {
 final TasksRegistry tasks = Mockito.mock(TasksRegistry.class);
 final TaskManager taskManager = 
setUpTaskManager(ProcessingMode.AT_LEAST_ONCE, tasks, true);
 when(tasks.allTasks()).thenReturn(mkSet(activeTaskToClose));
-
activeTaskCreator.closeAndRemoveTaskProducerIfNeeded(activeTaskToClose.id());
-expect(activeTaskCreator.createTasks(
-consumer,
-mkMap(mkEntry(activeTaskToCreate.id(), 
activeTaskToCreate.inputPartitions()))
-)).andReturn(emptySet());
 
expect(standbyTaskCreator.createTasks(Collections.emptyMap())).andReturn(emptySet());
-replay(activeTaskCreator, standbyTaskCreator);
+replay(standbyTaskCreator);
 
 taskManager.handleAssignment(
 mkMap(mkEntry(activeTaskToCreate.id(), 
activeTaskToCreate.inputPartitions())),
 Collections.emptyMap()
 );
 
-verify(activeTaskCreator, standbyTaskCreator);
+verify(standbyTaskCreator);
+Mockito.verify(activeTaskCreator).createTasks(
+consumer,

Review Comment:
   updated all the formatting suggestions. As a side note, such formatting 
changes can be handled by code style schemes that are imported to the IDE. Is 
there a schema like that that can be imported to Intellij?



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

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

2023-05-12 Thread via GitHub 


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


##
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##
@@ -845,17 +847,16 @@ public void shouldRecycleTasksRemovedFromStateUpdater() {
 
when(tasks.removePendingTaskToRecycle(task00.id())).thenReturn(taskId00Partitions);
 
when(tasks.removePendingTaskToRecycle(task01.id())).thenReturn(taskId01Partitions);
 taskManager = 
setUpTaskManager(StreamsConfigUtils.ProcessingMode.AT_LEAST_ONCE, tasks, true);
-expect(activeTaskCreator.createActiveTaskFromStandby(eq(task01), 
eq(taskId01Partitions), eq(consumer)))
-.andStubReturn(task01Converted);
-activeTaskCreator.closeAndRemoveTaskProducerIfNeeded(anyObject());
-expectLastCall().once();
+when(activeTaskCreator.createActiveTaskFromStandby(Mockito.eq(task01), 
Mockito.eq(taskId01Partitions),

Review Comment:
   I simply converted the previous EasyMock version that was using the matchers 
to Mockito but what you are saying make sense. Updated it - Thank you



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

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

2023-05-08 Thread via GitHub 


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


##
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:
   yeah good recommendation, I will push a new version with the update



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