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-16737: Enable unit tests for new consumer & cleanup TODOs #16032

Closed

Conversation

lianetm
Copy link
Collaborator

@lianetm lianetm commented May 22, 2024

KafkaConsumerTest contained lots of unit tests that were not enabled for the new consumer, with TODOs for verifying if they could be enabled. This PR enables all the unit tests that could be applied to the new consumer, and cleans up the TODOs.

The tests that remain being applied to the legacy consumer only cannot run for the new consumer, due to:

  1. bugs in the new consumer. Only 8 tests in this case, for those I left TODOs just to reference the Jira I filed to make sure the tests are enabled as soon as the bugs are fixed.
  2. the way the test is written or what it is testing (specific to the legacy consumer).

@lianetm lianetm closed this May 23, 2024
@lianetm lianetm reopened this May 23, 2024
@lianetm
Copy link
Collaborator Author

lianetm commented May 24, 2024

Follow-up task KAFKA-16823 to refactor the test common functionality, and move out the unit tests that apply to the LegacyConsumer only, so we end up with KafkaConsumerTest for tests that apply to both consumers, AsyncKafkaConsumerTest (already exists for new consumer tests only) and LegacyKafkaConsumerTest (to be created for legacy consumer tests only). It will be addressed once the remaining bugs for the new consumer are fixed, so we can decide if the tests that are blocked should be moved out or not.

@lianetm
Copy link
Collaborator Author

lianetm commented May 29, 2024

Hey @lucasbru , could you take a look at this small one when you have a chance? Thanks!

--Edit--
Let me keep an eye on the build and play with it repeating it a bit. I'll ping you when it completes confidently. I had mixed unrelated build failures hiding some real ones with the tests that did not show locally. Thanks!

@lianetm lianetm marked this pull request as draft May 29, 2024 14:28
@lianetm
Copy link
Collaborator Author

lianetm commented May 29, 2024

Leaving as a draft for now as this seems to be affected by the timeout partial fix merged recently a98c9be. Will investigate and reopen this when ready.

@chia7712
Copy link
Contributor

@lianetm sorry that I overlook this PR, but those tests covered by this PR are enable now (see #16370 and #16566). Maybe we can let this PR rest in peace 😄

@lianetm
Copy link
Collaborator Author

lianetm commented Jul 15, 2024

Hey @chia7712 , totally, I'll close this. It was only an initial exercise (helpful because it revealed bugs even though I didn't re-enabled the test that needed more work, I didn't have the bandwidth back then. Great that you and the other contributors could take on this!

@lianetm lianetm closed this Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants