KAFKA-19683: Some more test replacements [7/N] #20889
Conversation
github-actions
bot
added
triage
|
@lucasbru, tagging for review :) |
There was a problem hiding this comment.
Pull Request Overview
This pull request continues a series of test refactorings in TaskManagerTest.java, replacing tests that use StateMachineTask with tests that use mocked StreamTask instances created via the statefulTask builder pattern. This modernizes the testing approach to use better test utilities and mocking frameworks.
Key Changes:
- Consolidated two separate tests (
shouldComputeOffsetSumFromCheckpointFileForUninitializedTaskandshouldComputeOffsetSumFromCheckpointFileForClosedTask) into a single parameterized test - Replaced multiple tests that used
StateMachineTaskwith mockedStreamTaskinstances configured via builder pattern - Removed unused
assertNullimport
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| when(task01.process(anyLong())) | ||
| .thenThrow(timeoutException.get()) // throws TimeoutException | ||
| .thenReturn(true) | ||
| .thenReturn(true) | ||
| .thenReturn(false); |
There was a problem hiding this comment.
The mock behavior is evaluated at setup time, so timeoutException.get() will always return the TimeoutException. Setting timeoutException.set(null) on line 4218 won't change the mock's behavior. The second call to taskManager.process(1, time) will still throw the TimeoutException instead of processing records.
Consider using thenThrow(...).thenReturn(true).thenReturn(true).thenReturn(false) without the AtomicReference, or reconfigure the mock after clearing the timeout.
There was a problem hiding this comment.
Yes. I don't think the atomic reference makes sense now
| timeoutException.set(null); | ||
| assertThat(taskManager.process(1, time), is(3)); |
There was a problem hiding this comment.
Setting timeoutException.set(null) doesn't update the mock's behavior. The mock's thenThrow() at line 4198 was configured at setup time with the exception object and will continue to throw it. The test expects the retry on line 4219 to succeed, but it will still encounter the TimeoutException.
The test needs to either reconfigure the mock after line 4218 or avoid using the AtomicReference approach entirely.
There was a problem hiding this comment.
Yes - this is not working as you intended it
lucasbru
left a comment
lucasbru
left a comment
There was a problem hiding this comment.
Thanks for the PR! Minor comments only
| when(task01.process(anyLong())) | ||
| .thenThrow(timeoutException.get()) // throws TimeoutException | ||
| .thenReturn(true) | ||
| .thenReturn(true) | ||
| .thenReturn(false); |
There was a problem hiding this comment.
Yes. I don't think the atomic reference makes sense now
| timeoutException.set(null); | ||
| assertThat(taskManager.process(1, time), is(3)); |
There was a problem hiding this comment.
Yes - this is not working as you intended it
| mkEntry(new TopicPartition("changelog", 1), 10L) | ||
| ); | ||
| final Map<TaskId, Long> expectedOffsetSums = mkMap(mkEntry(taskId00, 15L)); | ||
| when(tasks.allTasksPerId()).thenReturn(mkMap(mkEntry(taskId00, task))); |
There was a problem hiding this comment.
It's a bit weird that you add tasks to the stateUpdater first, and then mock it. I would reverse the order
shashankhs11
force-pushed
the
KAFKA-19683-7
branch
from
62a0dc1 to
931852e
Compare
2 participants
Rewrote more tests in
TaskManagerTest.javaReviewers: Lucas Brutschy [email protected]