KAFKA-14133: Migrate Consumer mock in TaskManagerTest to Mockito by clolov · Pull Request #13873 · apache/kafka

clolov · 2023-06-19T10:37:44Z

This pull requests migrates the Consumer mock 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

divijvaidya

I took an initial look but it seems like a lot of verification is missing. Can you please ensure that whenever we remove a easymock stub, we are adding a powermock verification for it?

divijvaidya · 2023-06-20T09:12:58Z

why are we checking for atLeastOnce and not the exact times? Isn't this relaxing constrains from what we were doing with easy mock?

(same for verifyResumeWasCalledWith)

There are a couple of tests where consumer.resume() is called twice. I will amend this to be stricter if possible

I would inline this function.

divijvaidya · 2023-06-20T09:13:51Z

s/verifyResumeWasCalledWith /verifyResumeWasCalledWithAssignment

I would inline this function.

divijvaidya · 2023-06-20T09:15:16Z

verify this?

This is not called, this is why I removed it

divijvaidya · 2023-06-20T09:16:47Z

This is just not called, that's why I removed it originally

divijvaidya · 2023-06-20T09:17:53Z

~~I don't think there is a need for verification here. The consumer was only replayed in EasyMock and the replayed behaviour comes by default in Mockito~~
This is just not called, that's why I removed it originally

divijvaidya · 2023-06-20T09:18:51Z

I actually do not know why these were being verified with EasyMock given that taskXY are not mocks

Yeah, that seems weird...

cadonna

Hi @clolov ! Thanks for the PR!

I started to review the PR and have the following feedback so far.

cadonna

Thanks for the updates, @clolov !

Here some more comments.

cadonna · 2023-06-26T10:45:48Z

I would inline this function since it became an one-liner.

when(consumer.assignment()).thenReturn(singleton(new TopicPartition("assignment", 0)));

cadonna · 2023-06-26T10:48:40Z

I would inline this function.

cadonna · 2023-06-26T10:48:50Z

I would inline this function.

cadonna · 2023-06-26T10:52:22Z

Why do you union a single set?

Suggested change

final Set<TopicPartition> partitions = union(HashSet::new, taskId00Partitions);

when(consumer.assignment()).thenReturn(partitions);

when(consumer.assignment()).thenReturn(taskId00Partitions);

... and isn't this a duplicate of line 1923 (except the differing partitions).

expectAssignmentToBeCalled(consumer);

Could it be that you can remove line 1923?

~~Because I couldn't figure out a better way to create a set with only one element and Collections does not have a singletonSet~~ 😞

~~Oh, silly me, there is a mkSet method in Utils. Okay, I will remedy this.~~
Silly me x2, the thing is already a set

You are correct, it appears I can remove line 1923

cadonna · 2023-06-26T10:57:48Z

Same as above also here

cadonna · 2023-06-26T12:13:50Z

Why do you verify? It was not verified in the original code.

I added a couple of this comments below. However, I am not sure whether the original author did not want to verify the consumer or if they forgot about it. I will leave it to you if you want to keep them or not.

cadonna · 2023-06-26T12:14:36Z

Why do you verify? It was not verified in the original code.

cadonna · 2023-06-26T12:14:53Z

Why do you verify? It was not verified in the original code.

cadonna · 2023-06-26T12:15:03Z

Why do you verify? It was not verified in the original code.

cadonna · 2023-06-26T12:23:46Z

Suggested change

Mockito.verifyNoMoreInteractions(consumer);

Mockito.verifyNoInteractions(consumer);

github-actions · 2023-10-09T03:33:12Z

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.

clolov · 2023-10-19T09:10:42Z

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

Co-authored-by: Bruno Cadonna <cadonna@apache.org>

mimaison · 2024-06-10T09:41:56Z

Can this be closed now? It looks like this has been done in a33c47e / 8b72a2c

cadonna · 2024-06-10T10:16:50Z

Yes

clolov requested review from cadonna and divijvaidya June 19, 2023 10:38

divijvaidya added the tests Test fixes (including flaky tests) label Jun 19, 2023

divijvaidya requested changes Jun 20, 2023

View reviewed changes

cadonna reviewed Jun 21, 2023

View reviewed changes

cadonna reviewed Jun 26, 2023

View reviewed changes

clolov force-pushed the KAFKA-14133-PART10 branch from 2a5ccfd to 349a9d0 Compare June 26, 2023 13:53

clolov requested review from cadonna and divijvaidya June 27, 2023 13:00

cadonna reviewed Jul 3, 2023

View reviewed changes

Comment thread streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java Outdated

github-actions Bot added stale Stale PRs and removed stale Stale PRs labels Oct 9, 2023

clolov and others added 3 commits October 19, 2023 10:19

KAFKA-14133: Migrate Consumer mock in TaskManagerTest to Mockito

78c60e7

Address comments

fcb4db0

Co-authored-by: Bruno Cadonna <cadonna@apache.org>

Rebase + remove extra imports

7c917f0

clolov force-pushed the KAFKA-14133-PART10 branch from 910effb to 7c917f0 Compare October 19, 2023 09:20

cadonna closed this Jun 10, 2024

clolov deleted the KAFKA-14133-PART10 branch February 3, 2025 13:27

	final Set<TopicPartition> partitions = union(HashSet::new, taskId00Partitions);
	when(consumer.assignment()).thenReturn(partitions);
	when(consumer.assignment()).thenReturn(taskId00Partitions);

	Mockito.verifyNoMoreInteractions(consumer);
	Mockito.verifyNoInteractions(consumer);

Conversation

clolov commented Jun 19, 2023

Uh oh!

divijvaidya left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clolov Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clolov Jun 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cadonna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cadonna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clolov Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clolov Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clolov Jun 26, 2023 •

edited

Loading

clolov Jun 23, 2023 •

edited

Loading

clolov Jun 26, 2023 •

edited

Loading

clolov Jun 26, 2023 •

edited

Loading