Re: [PR] KAFKA-14133: Migrate Consumer mock in TaskManagerTest to Mockito [kafka]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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