You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "cadonna (via GitHub)" <gi...@apache.org> on 2023/04/08 08:54:18 UTC
[GitHub] [kafka] cadonna opened a new pull request, #13529: KAFKA-14133: Migrate topology builder mock in TaskManagerTest to mockito
cadonna opened a new pull request, #13529:
URL: https://github.com/apache/kafka/pull/13529
1. Migrates topology builder mock in TaskManagerTest to mockito.
2. Replaces the unit test to verify if subscribed partitions are added to topology metadata.
3. Modifies signatures of methods for adding subscribed partitions to topology metadata to use sets instead of lists. This makes the intent of the methods clearer and makes the tests more portable.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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
[GitHub] [kafka] cadonna commented on a diff in pull request #13529: KAFKA-14133: Migrate topology builder mock in TaskManagerTest to mockito
Posted by "cadonna (via GitHub)" <gi...@apache.org>.
cadonna commented on code in PR #13529:
URL: https://github.com/apache/kafka/pull/13529#discussion_r1168432918
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##########
@@ -970,6 +971,7 @@ public void shouldHandleMultipleRemovedTasksFromStateUpdater() {
expectLastCall().anyTimes();
final TasksRegistry tasks = mock(TasksRegistry.class);
when(tasks.removePendingTaskToCloseClean(taskToClose.id())).thenReturn(true);
+ when(tasks.removePendingTaskToCloseClean(argThat(taskId -> !taskId.equals(taskToClose.id())))).thenReturn(false);
Review Comment:
I had to add this since Mockito threw an error. The cause of the errors was that this PR set the Mockito stubs to strict. The error was:
```
org.mockito.exceptions.misusing.PotentialStubbingProblem:
Strict stubbing argument mismatch. Please check:
- this invocation of 'removePendingTaskToCloseClean' method:
tasksRegistry.removePendingTaskToCloseClean(
0_3
);
-> at org.apache.kafka.streams.processor.internals.TaskManager.handleRemovedTasksFromStateUpdater(TaskManager.java:876)
- has following stubbing(s) with different arguments:
1. tasksRegistry.removePendingTaskToCloseClean(
0_2
);
-> at org.apache.kafka.streams.processor.internals.TaskManagerTest.shouldHandleMultipleRemovedTasksFromStateUpdater(TaskManagerTest.java:973)
Typically, stubbing argument mismatch indicates user mistake when writing tests.
Mockito fails early so that you can debug potential problem easily.
However, there are legit scenarios when this exception generates false negative signal:
- stubbing the same method multiple times using 'given().will()' or 'when().then()' API
Please use 'will().given()' or 'doReturn().when()' API for stubbing.
- stubbed method is intentionally invoked with different arguments by code under test
Please use default or 'silent' JUnit Rule (equivalent of Strictness.LENIENT).
For more information see javadoc for PotentialStubbingProblem class.
```
In this case, the code under test calls `removePendingTaskToCloseClean()` multiple times with different arguments. That is normal but needs to be considered in the unit test.
--
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
[GitHub] [kafka] cadonna merged pull request #13529: KAFKA-14133: Migrate topology builder mock in TaskManagerTest to mockito
Posted by "cadonna (via GitHub)" <gi...@apache.org>.
cadonna merged PR #13529:
URL: https://github.com/apache/kafka/pull/13529
--
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
[GitHub] [kafka] cadonna commented on a diff in pull request #13529: KAFKA-14133: Migrate topology builder mock in TaskManagerTest to mockito
Posted by "cadonna (via GitHub)" <gi...@apache.org>.
cadonna commented on code in PR #13529:
URL: https://github.com/apache/kafka/pull/13529#discussion_r1165561113
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##########
@@ -2895,10 +2871,8 @@ public void shouldNotCommitOnHandleAssignmentIfNoTaskClosed() {
expect(activeTaskCreator.createTasks(anyObject(), eq(Collections.emptyMap()))).andReturn(Collections.emptySet());
expect(standbyTaskCreator.createTasks(eq(assignmentStandby))).andReturn(singletonList(task10));
expect(standbyTaskCreator.createTasks(eq(Collections.emptyMap()))).andReturn(Collections.emptySet());
- topologyBuilder.addSubscribedTopicsFromAssignment(eq(asList(t1p0)), anyString());
Review Comment:
The idea is to have a unit test that specifically verifies this interaction, i.e., `shouldAddSubscribedTopicsFromAssignmentToTopologyMetadata()` and to not verify this interaction in the other unit tests where we verify other unrelated aspects. I think this interaction was verified in so many unit tests before because the topology builder mock was specified as strict and easy mock requires to verify all interactions with strict mocks even if we actually do not care about this interactions in some unit tests.
--
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
[GitHub] [kafka] clolov commented on a diff in pull request #13529: KAFKA-14133: Migrate topology builder mock in TaskManagerTest to mockito
Posted by "clolov (via GitHub)" <gi...@apache.org>.
clolov commented on code in PR #13529:
URL: https://github.com/apache/kafka/pull/13529#discussion_r1163868654
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##########
@@ -2895,10 +2871,8 @@ public void shouldNotCommitOnHandleAssignmentIfNoTaskClosed() {
expect(activeTaskCreator.createTasks(anyObject(), eq(Collections.emptyMap()))).andReturn(Collections.emptySet());
expect(standbyTaskCreator.createTasks(eq(assignmentStandby))).andReturn(singletonList(task10));
expect(standbyTaskCreator.createTasks(eq(Collections.emptyMap()))).andReturn(Collections.emptySet());
- topologyBuilder.addSubscribedTopicsFromAssignment(eq(asList(t1p0)), anyString());
Review Comment:
Is the idea here that since we do not rely on the return value from this method you do not want to verify this interaction? I noticed that earlier in the pull request we do verify interactions of this type.
--
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
[GitHub] [kafka] clolov commented on a diff in pull request #13529: KAFKA-14133: Migrate topology builder mock in TaskManagerTest to mockito
Posted by "clolov (via GitHub)" <gi...@apache.org>.
clolov commented on code in PR #13529:
URL: https://github.com/apache/kafka/pull/13529#discussion_r1170120571
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##########
@@ -2895,10 +2871,8 @@ public void shouldNotCommitOnHandleAssignmentIfNoTaskClosed() {
expect(activeTaskCreator.createTasks(anyObject(), eq(Collections.emptyMap()))).andReturn(Collections.emptySet());
expect(standbyTaskCreator.createTasks(eq(assignmentStandby))).andReturn(singletonList(task10));
expect(standbyTaskCreator.createTasks(eq(Collections.emptyMap()))).andReturn(Collections.emptySet());
- topologyBuilder.addSubscribedTopicsFromAssignment(eq(asList(t1p0)), anyString());
Review Comment:
Got it, okay, this makes a lot of sense.
--
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