KAFKA-7938: Fix test flakiness in DeleteConsumerGroupsTest#6306
KAFKA-7938: Fix test flakiness in DeleteConsumerGroupsTest#6306gwenshap wants to merge 1 commit intoapache:2.2from
Conversation
|
Forgot to mention:
|
|
cc @mjsax |
gwenshap
changed the title
|
I heard you like flaky tests, so I put some flaky test failures in your flaky test fixes... I think the failures are unrelated :) |
vahidhashemian
left a comment
vahidhashemian
left a comment
There was a problem hiding this comment.
Thanks @gwenshap for the PR. I'm fine with removing those flaky "short initialization" tests. They make more sense for describing groups anyway (where they were originally added).
I left a couple of minor comments inline.
| }, "The group did not initialize as expected.", maxRetries = 3) | ||
|
|
||
| val result = service.deleteGroups() | ||
| println(result) |
There was a problem hiding this comment.
Unintentional line here?
There was a problem hiding this comment.
actually, quite intentional :) Those failures are difficult to reproduce, so more info in the console output of the test will help us next time.
I can add a comment if this is confusing.
There was a problem hiding this comment.
But this prints the output even when the test passes. If we want the output on failures only maybe that output can be added to the assertion error messages that follow. Unless there is another reason to have the output in either scenario?
There was a problem hiding this comment.
yeah, that makes sense. Will send the improvement tomorrow morning :)
|
|
||
| TestUtils.waitUntilTrue(() => { | ||
| service.listGroups().contains(group) | ||
| service.collectGroupMembers(false)._2.get.size == 1 |
There was a problem hiding this comment.
Thanks for improving this check. Do you think it's worth doing the same in a few other unit tests in this file?
There was a problem hiding this comment.
I went over the other tests, and none of them required a non-empty group specifically. Did I miss something?
There was a problem hiding this comment.
You're right, sorry my mistake.
Why not to the PR against |
will do. |
|
Replaced by #6312 which is against trunk. |
3 participants
This attempts to fix KAFKA-7938 and KAFKA-7946.
I removed two tests:
I thought the tests had limited value relative to the effort of figuring out a way to make the timing work.
I also fixed testDeleteCmdNonEmptyGroup and testDeleteNonEmptyGroup so they will validate that the group both exists and is non-empty before starting the test itself. I also added some extra information for future debugging sessions :)
I ran the tests LOTS of times to validate, but with flaky tests, it is hard to tell :)
Committer Checklist (excluded from commit message)