Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KAFKA-14133: Migrate various mocks in TaskManagerTest to Mockito #13874

Merged
merged 2 commits into from Jul 20, 2023

Conversation

clolov
Copy link
Collaborator

@clolov clolov commented Jun 19, 2023

This pull requests migrates the StreamTask, StandByTask, ProcessorStateManager and StreamProducer mocks in TaskManagerTest from EasyMock to Mockito.
The change is restricted to a single mock to minimize the scope and make it easier for review.

The reasoning as to why we would like to migrate a single mock rather than all mocks in the file at the same time has been discussed in #12607 (comment)

It takes the same approach as in:
#13529
#13621
#13681
#13711

@clolov clolov added tests Test fixes (including flaky tests) streams labels Jun 19, 2023

when(activeTaskCreator.createTasks(any(), Mockito.eq(taskId00Assignment))).thenReturn(singletonList(activeTask));
activeTask.prepareRecycle();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verify please

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this because it was never invoked.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for this is that the method in which prepareRecycle is used is the mocked createStandbyTaskFromActive of standbyTaskCreator. Let me know in case I am wrong


shouldCommitViaProducerIfEosEnabled(ProcessingMode.EXACTLY_ONCE_V2, producer, offsetsT01, offsetsT02);
Mockito.verify(producer).commitTransaction(allOffsets, new ConsumerGroupMetadata("appId"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, verifyNoMoreInteractions since the old easymock verify(producer) was verifying all invocations.

Copy link
Contributor

@divijvaidya divijvaidya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the changes Christo. Looks good to me.

(I have verified that TaskManagerTest is successful in the CI, rest all failures are unrelated to this change)

@divijvaidya divijvaidya merged commit 8f313ea into apache:trunk Jul 20, 2023
1 check failed
jeqo pushed a commit to jeqo/kafka that referenced this pull request Jul 21, 2023
Cerchie pushed a commit to Cerchie/kafka that referenced this pull request Jul 25, 2023
jeqo pushed a commit to aiven/kafka that referenced this pull request Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
streams tests Test fixes (including flaky tests)
Projects
None yet
2 participants