Re: [PR] KAFKA-14133: Migrate Consumer mock in TaskManagerTest to Mockito [kafka]

2024-06-10 Thread via GitHub 


cadonna commented on PR #13873:
URL: https://github.com/apache/kafka/pull/13873#issuecomment-2157945450

   Yes


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

Re: [PR] KAFKA-14133: Migrate Consumer mock in TaskManagerTest to Mockito [kafka]

2024-06-10 Thread via GitHub 


cadonna closed pull request #13873: KAFKA-14133: Migrate Consumer mock in 
TaskManagerTest to Mockito
URL: https://github.com/apache/kafka/pull/13873


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

Re: [PR] KAFKA-14133: Migrate Consumer mock in TaskManagerTest to Mockito [kafka]

2024-06-10 Thread via GitHub 


mimaison commented on PR #13873:
URL: https://github.com/apache/kafka/pull/13873#issuecomment-2157861358

   Can this be closed now? It looks like this has been done in 
https://github.com/apache/kafka/commit/a33c47ea4ddc810d66b6ed17ab74e40c5b7668fb 
/ 
https://github.com/apache/kafka/commit/8b72a2c72f09838fdd2e7416c98d30fe876b4078


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

Re: [PR] KAFKA-14133: Migrate consumer mock in TaskManagerTest to Mockito [kafka]

2024-01-10 Thread via GitHub 


divijvaidya merged PR #15112:
URL: https://github.com/apache/kafka/pull/15112


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

Re: [PR] KAFKA-14133: Migrate consumer mock in TaskManagerTest to Mockito [kafka]

2024-01-10 Thread via GitHub 


divijvaidya commented on PR #15112:
URL: https://github.com/apache/kafka/pull/15112#issuecomment-1884860357

   The test modified in this PR is successful in CI - 
https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-15112/5/testReport/org.apache.kafka.streams.processor.internals/TaskManagerTest/


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

Re: [PR] KAFKA-14133: Migrate consumer mock in TaskManagerTest to Mockito [kafka]

2024-01-05 Thread via GitHub 


clolov commented on code in PR #15112:
URL: https://github.com/apache/kafka/pull/15112#discussion_r1443172726


##
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##
@@ -4904,7 +4870,8 @@ public void suspend() {
 final Map> assignment = new 
HashMap<>(taskId00Assignment);
 assignment.putAll(taskId01Assignment);
 when(activeTaskCreator.createTasks(any(), 
Mockito.eq(assignment))).thenReturn(asList(task00, task01));
-replay(consumer);
+
+taskManager.setMainConsumer(mockitoConsumer);

Review Comment:
   Yes, we are using the old consumer in all places making calls to
   ```
   private static void expectConsumerAssignmentPaused(final 
Consumer consumer) {
   final Set assignment = singleton(new 
TopicPartition("assignment", 0));
   expect(consumer.assignment()).andReturn(assignment);
   consumer.pause(assignment);
   }
   ```
   This is also why I took the approach of having a separate Mockito consumer. 
Part 2 of this pull request will migrate all of those usages.



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

Re: [PR] KAFKA-14133: Migrate consumer mock in TaskManagerTest to Mockito [kafka]

2024-01-05 Thread via GitHub 


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


##
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##
@@ -4904,7 +4870,8 @@ public void suspend() {
 final Map> assignment = new 
HashMap<>(taskId00Assignment);
 assignment.putAll(taskId01Assignment);
 when(activeTaskCreator.createTasks(any(), 
Mockito.eq(assignment))).thenReturn(asList(task00, task01));
-replay(consumer);
+
+taskManager.setMainConsumer(mockitoConsumer);

Review Comment:
   Is there a place where we are using old consumer with taskManager? If not, 
can we se this to new consumer in @before methods itself where we construct 
taskManager



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

Re: [PR] KAFKA-14133: Migrate consumer mock in TaskManagerTest to Mockito [kafka]

2024-01-05 Thread via GitHub 


clolov commented on code in PR #15112:
URL: https://github.com/apache/kafka/pull/15112#discussion_r1442836205


##
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##
@@ -4809,11 +4768,14 @@ public void 
shouldNotFailForTimeoutExceptionOnCommitWithEosAlpha() {
 exception.corruptedTasks(),
 equalTo(Collections.singleton(taskId00))
 );
+
+Mockito.verify(mockitoConsumer, times(2)).groupMetadata();

Review Comment:
   I verified that if I change
   ```
   expect(consumer.groupMetadata()).andStubReturn(null);
   ```
   to
   ```
   expect(consumer.groupMetadata()).andReturn(null).times(2);
   ```
   the test still passes. I suspect it was always called twice, just never 
verified



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

Re: [PR] KAFKA-14133: Migrate consumer mock in TaskManagerTest to Mockito [kafka]

2024-01-05 Thread via GitHub 


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


##
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##
@@ -1354,14 +1346,12 @@ public void 
shouldHandleTimeoutExceptionInTransitRestoredTaskToRunning() {
 final TaskManager taskManager = 
setUpTransitionToRunningOfRestoredTask(task, tasks);

Review Comment:
   don't we want to do a `taskManager.setMainConsumer(mockitoConsumer);` here? 
(unless setUpTransitionToRunningOfRestoredTask is already doing so)



##
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##
@@ -4954,8 +4910,6 @@ public void shouldConvertStandbyTaskToActiveTask() {
 
when(activeTaskCreator.createActiveTaskFromStandby(Mockito.eq(standbyTask), 
Mockito.eq(taskId00Partitions), any()))
 .thenReturn(activeTask);
 
-replay(consumer);

Review Comment:
   I assume that consumer is never used in these tests where we have removed 
the replay?
   
   In that case, should we do something similar to what you did in 
shouldReturnFalseWhenThereAreStillNonRunningTasks , i.e. test for no 
interactions



##
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##
@@ -4809,11 +4768,14 @@ public void 
shouldNotFailForTimeoutExceptionOnCommitWithEosAlpha() {
 exception.corruptedTasks(),
 equalTo(Collections.singleton(taskId00))
 );
+
+Mockito.verify(mockitoConsumer, times(2)).groupMetadata();

Review Comment:
   should it be called twice as per code?



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

Re: [PR] KAFKA-14133: Migrate consumer mock in TaskManagerTest to Mockito [kafka]

2024-01-03 Thread via GitHub 


clolov commented on code in PR #15112:
URL: https://github.com/apache/kafka/pull/15112#discussion_r1440569384


##
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##
@@ -197,6 +196,8 @@ public class TaskManagerTest {
 @Mock(type = MockType.STRICT)
 private Consumer consumer;
 @org.mockito.Mock
+private Consumer mockitoConsumer;

Review Comment:
   I ran into quite a lot of problems when I tried migrating the whole mock, so 
I decided to do the migration test-by-test. This way problems could be flushed 
out. By introducing this mock and using `setMainConsumer` on a test-by-test 
basis things are easier to go through (at least in my opinion )



##
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##
@@ -4809,11 +4768,14 @@ public void 
shouldNotFailForTimeoutExceptionOnCommitWithEosAlpha() {
 exception.corruptedTasks(),
 equalTo(Collections.singleton(taskId00))
 );
+
+Mockito.verify(mockitoConsumer, times(2)).groupMetadata();

Review Comment:
   Mockito claims the method is called twice. This wasn't specified in EasyMock 
world, but I decided to make it explicit now.



##
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##
@@ -3010,13 +2993,9 @@ public void initializeIfNeeded() {
 }
 };
 
-consumer.commitSync(Collections.emptyMap());
-expectLastCall();
-expect(consumer.assignment()).andReturn(emptySet());
-consumer.resume(eq(emptySet()));
-expectLastCall();

Review Comment:
   Same as above, Mockito reported these are unused. I added an assertion in 
Mockito world at the end (...I should probably add one in the first test as 
well, but I can do this in part 2 of this pull request)



##
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##
@@ -1168,13 +1166,10 @@ public void 
shouldHandleMultipleRemovedTasksFromStateUpdater() {
 when(stateUpdater.drainRemovedTasks())
 .thenReturn(mkSet(taskToRecycle0, taskToRecycle1, taskToClose, 
taskToUpdateInputPartitions));
 when(stateUpdater.restoresActiveTasks()).thenReturn(true);
-when(activeTaskCreator.createActiveTaskFromStandby(taskToRecycle1, 
taskId01Partitions, consumer))
+when(activeTaskCreator.createActiveTaskFromStandby(taskToRecycle1, 
taskId01Partitions, mockitoConsumer))
 .thenReturn(convertedTask1);
 when(standbyTaskCreator.createStandbyTaskFromActive(taskToRecycle0, 
taskId00Partitions))
 .thenReturn(convertedTask0);
-expect(consumer.assignment()).andReturn(emptySet()).anyTimes();
-consumer.resume(anyObject());
-expectLastCall().anyTimes();

Review Comment:
   According to Mockito these were unused. I deleted them in EasyMock world and 
the test still passed. Hence I removed them.



##
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##
@@ -3047,14 +3027,9 @@ public void completeRestoration(final 
java.util.function.Consumer> assignment = 
singletonMap(taskId00, taskId00Partitions);
 final Task task00 = new StateMachineTask(taskId00, taskId00Partitions, 
false, stateManager);
 
+taskManager.setMainConsumer(mockitoConsumer);
+
 // `handleAssignment`
 
when(standbyTaskCreator.createTasks(assignment)).thenReturn(singletonList(task00));
 
-// `tryToCompleteRestoration`
-expect(consumer.assignment()).andReturn(emptySet());
-consumer.resume(eq(emptySet()));
-expectLastCall();
-
-// `shutdown`
-consumer.commitSync(Collections.emptyMap());
-expectLastCall();

Review Comment:
   Ditto



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

[PR] KAFKA-14133: Migrate consumer mock in TaskManagerTest to Mockito [kafka]

2024-01-03 Thread via GitHub 


clolov opened a new pull request, #15112:
URL: https://github.com/apache/kafka/pull/15112

   This pull request migrates the consumer mock in TaskManagerTest test by test 
for easier reviews.


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

Re: [PR] KAFKA-14133: Migrate Consumer mock in TaskManagerTest to Mockito [kafka]

2023-10-19 Thread via GitHub 


clolov commented on PR #13873:
URL: https://github.com/apache/kafka/pull/13873#issuecomment-1770389966

   I will provide an update on this pull request in order to not close it!


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

Re: [PR] KAFKA-14133: Migrate Consumer mock in TaskManagerTest to Mockito [kafka]

2023-10-08 Thread via GitHub 


github-actions[bot] commented on PR #13873:
URL: https://github.com/apache/kafka/pull/13873#issuecomment-1752300357

   This PR is being marked as stale since it has not had any activity in 90 
days. If you would like to keep this PR alive, please ask a committer for 
review. If the PR has  merge conflicts, please update it with the latest from 
trunk (or appropriate release branch)  If this PR is no longer valid or 
desired, please feel free to close it. If no activity occurs in the next 30 
days, it will be automatically closed.


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