-
Couldn't load subscription status.
- Fork 14.7k
KAFKA-19683: More cleanup and rewrite [4/N] #20777
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
base: trunk
Are you sure you want to change the base?
Conversation
|
@lucasbru tagging for review :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR is part 4 of a cleanup and rewrite effort (KAFKA-19683) that removes tagged-for-deletion tests and rewrites existing test cases in the TaskManagerTest file. The changes modernize test structure by replacing legacy StateMachineTask mock implementations with builder-based task creation and Mockito mocks, moving toward a more maintainable and clearer testing approach.
Key Changes:
- Removed legacy test methods that relied on custom
StateMachineTasksubclasses with inline behavior overrides - Rewrote tests using builder patterns (
statefulTask(),standbyTask()) and Mockito verification for cleaner test setup - Removed exactly-once-v2 (EOS V2) specific tests that are no longer needed
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two high-level qeustions for now
|
|
||
| @SuppressWarnings("removal") | ||
| @Test | ||
| public void shouldCommitAllActiveTasksThatNeedCommittingOnHandleRevocationWithEosV2() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you removing this without replacement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I removed them blindly only since they were marked for removal. Another oversight by me 😓
I'll replace them in another commit.
|
|
||
| @SuppressWarnings("removal") | ||
| @Test | ||
| public void shouldCommitAllActiveTasksThatNeedCommittingOnHandleRevocationWithEosV2() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you removing this without replacement?
|
|
||
| @SuppressWarnings("removal") | ||
| @Test | ||
| public void shouldCommitViaProducerIfEosV2Enabled() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you removing this without replacement?
|
|
||
| @SuppressWarnings("removal") | ||
| @Test | ||
| public void shouldCommitAllActiveTasksThatNeedCommittingOnHandleRevocationWithEosV2() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you removing this without replacement?
Reviewers: Lucas Brutschy [email protected]