Fix flaky test TestClusterJoinAndReconnect/TestTLSConnection again (#3278) #4635
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
@gotjosh I think you can review this best. |
siavashs
suggested changes
Nov 5, 2025
siavashs
approved these changes
Nov 5, 2025
SuperQ
approved these changes
Nov 5, 2025
Member
SuperQ
left a comment
SuperQ
left a comment
There was a problem hiding this comment.
Great find! Thanks for fixing this.
TheMeier
approved these changes
Nov 5, 2025
Two commits have already merged in order to address the flakiness of this test. However, I can still reproduce the issue using: ```sh go test -failfast -run "TestClusterJoinAndReconnect/TestJoinLeave" -count 600 ./cluster ``` An easy way to increase the failure rate is to increase CPU load, e.g., ```sh yes > /dev/null &; yes > /dev/null &; yes > /dev/null &; yes > /dev/null & ``` On my machine the combination of these commands fails every time. The underlying reason for the failure is that the test only waits for `p2` to be ready, but this does not reflect whether `p` has updated its memberlist. The test now retries the assertions in question using `Eventually`. Fixes prometheus#3287 Signed-off-by: Solomon Jacobs <[email protected]> Don't modify cluster for TestClusterJoinAndReconnect/TestTLSConnection
Contributor
|
I don't think that this resolved all of the flaky tests. https://github.com/prometheus/alertmanager/actions/runs/19104722659/job/54585342806 |
Contributor
Author
|
@sysadmind That seems very likely, I didn't play around with |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Repeating my commit message for convenience:
Two commits have already merged in order to address the flakiness of this test.
However, I can still reproduce the issue using:
An easy way to increase the failure rate is to increase CPU load, e.g.,
On my machine the combination of these commands fails every time.
The underlying reason for the failure is that the test only waits for
p2to be ready, but this does not reflect whetherphas updated its memberlist.Edit due to the comments I got:
We can ensure thatThe test now retries the assertions in question usingphas updated its memberlist by waiting forNotifyJointo be called. The test is now slightly slower, 0.8 seconds on my machine.Eventually.Side-note
I am new to the project and feedback is very appreciated. It was hard to find something that avoids spin looping and does not change the API of
Peer. Also, theWaitReadyandSettlecalls are redundant now, since we are actually waiting forNotifyJoin. But leaving them in does not hurt either.Fixes #3287