You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "clolov (via GitHub)" <gi...@apache.org> on 2023/04/20 15:01:51 UTC
[GitHub] [kafka] clolov opened a new pull request, #13621: KAFKA-14133: Migrate ChangeLogReader mock in TaskManagerTest to Mockito
clolov opened a new pull request, #13621:
URL: https://github.com/apache/kafka/pull/13621
This pull request takes a similar approach as the one outlined in https://github.com/apache/kafka/pull/13529 to move each mock separately to ease reviewing the code.
Once https://github.com/apache/kafka/pull/13529 is merged this pull request will be rebased on top (as some of the changes are present in both pull requests to make the tests pass).
--
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 pull request #13621: KAFKA-14133: Migrate ChangeLogReader mock in TaskManagerTest to Mockito
Posted by "clolov (via GitHub)" <gi...@apache.org>.
clolov commented on PR #13621:
URL: https://github.com/apache/kafka/pull/13621#issuecomment-1516499410
Heya @cadonna! I hope this attempt is what you had in mind? Unless I am wrong the removals detailed in the pull request are sensible as Mockito should be returning empty collections for method invocations on mocks based on "By default, for all methods that return a value, a mock will return either null, a primitive/primitive wrapper value, or an empty collection, as appropriate" (source: https://javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/Mockito.html#stubbing)
--
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 pull request #13621: KAFKA-14133: Migrate ChangeLogReader mock in TaskManagerTest to Mockito
Posted by "clolov (via GitHub)" <gi...@apache.org>.
clolov commented on PR #13621:
URL: https://github.com/apache/kafka/pull/13621#issuecomment-1517474714
I believe the related test failures are due to the same problem as https://github.com/apache/kafka/pull/13529#discussion_r1168432918. Once that PR is merged and this one is rebased those tests should pass.
--
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 pull request #13621: KAFKA-14133: Migrate ChangeLogReader mock in TaskManagerTest to Mockito
Posted by "clolov (via GitHub)" <gi...@apache.org>.
clolov commented on PR #13621:
URL: https://github.com/apache/kafka/pull/13621#issuecomment-1521580408
Great! Thank you @cadonna, I will aim to address the comments either today or tomorrow!
--
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 #13621: KAFKA-14133: Migrate ChangeLogReader mock in TaskManagerTest to Mockito
Posted by "cadonna (via GitHub)" <gi...@apache.org>.
cadonna merged PR #13621:
URL: https://github.com/apache/kafka/pull/13621
--
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 #13621: KAFKA-14133: Migrate ChangeLogReader mock in TaskManagerTest to Mockito
Posted by "cadonna (via GitHub)" <gi...@apache.org>.
cadonna commented on code in PR #13621:
URL: https://github.com/apache/kafka/pull/13621#discussion_r1175663484
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##########
@@ -2602,23 +2608,21 @@ 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(activeTaskCreator, consumer);
}
@Test
public void shouldAddNewActiveTasks() {
final Map<TaskId, Set<TopicPartition>> assignment = taskId00Assignment;
final Task task00 = new StateMachineTask(taskId00, taskId00Partitions, true);
- expect(changeLogReader.completedChangelogs()).andReturn(emptySet());
expect(consumer.assignment()).andReturn(emptySet());
consumer.resume(eq(emptySet()));
expectLastCall();
- changeLogReader.enforceRestoreActive();
expectLastCall();
Review Comment:
This line can also be removed.
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##########
@@ -2653,15 +2658,13 @@ public void initializeIfNeeded() {
consumer.commitSync(Collections.emptyMap());
expectLastCall();
- expect(changeLogReader.completedChangelogs()).andReturn(emptySet());
expect(consumer.assignment()).andReturn(emptySet());
consumer.resume(eq(emptySet()));
expectLastCall();
- changeLogReader.enforceRestoreActive();
expectLastCall();
Review Comment:
This line can also be removed.
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##########
@@ -3045,17 +3048,14 @@ public void closeDirty() {
}
};
- resetToStrict(changeLogReader);
- changeLogReader.enforceRestoreActive();
- expect(changeLogReader.completedChangelogs()).andReturn(emptySet());
Review Comment:
Since the mock was reset to strict, I think you need to verify or explicitly stub the call to `changeLogReader.completedChangelogs()`. Otherwise, the test would not fail, if `completedChangelogs()` were not called.
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##########
@@ -3168,16 +3167,13 @@ public Set<TopicPartition> changelogPartitions() {
}
};
- resetToStrict(changeLogReader);
- changeLogReader.enforceRestoreActive();
- expect(changeLogReader.completedChangelogs()).andReturn(emptySet());
Review Comment:
Same here
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##########
@@ -3117,16 +3118,13 @@ public Set<TopicPartition> changelogPartitions() {
final Map<TopicPartition, OffsetAndMetadata> offsets = singletonMap(t1p0, new OffsetAndMetadata(0L, null));
task00.setCommittableOffsetsAndMetadata(offsets);
- resetToStrict(changeLogReader);
- changeLogReader.enforceRestoreActive();
- expect(changeLogReader.completedChangelogs()).andReturn(emptySet());
Review Comment:
Same here.
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##########
@@ -3297,16 +3294,13 @@ public void suspend() {
}
};
- resetToStrict(changeLogReader);
- changeLogReader.enforceRestoreActive();
- expect(changeLogReader.completedChangelogs()).andReturn(emptySet());
Review Comment:
Same here
--
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 pull request #13621: KAFKA-14133: Migrate ChangeLogReader mock in TaskManagerTest to Mockito
Posted by "cadonna (via GitHub)" <gi...@apache.org>.
cadonna commented on PR #13621:
URL: https://github.com/apache/kafka/pull/13621#issuecomment-1531348081
@clolov Thanks for the updates! I just merged https://github.com/apache/kafka/pull/13529. Could you please rebase this PR?
--
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