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

Really fix flaky TestMultipleClients tests #603

Merged
merged 4 commits into from
Oct 10, 2024

Conversation

julienduchesne
Copy link
Member

@julienduchesne julienduchesne commented Oct 9, 2024

#599 went in a wrong direction, so I revert it here: it makes all TestMultipleClients tests flaky since not all members are necessarily joined after the updates

Instead, to fix the TestMultipleClientsWithMixedLabelsAndExpectFailure test, I add a couple more labeled members, that way, even if we reach 3 updates (which happened some times, hence the flakes), we'll never get to 5 with a single member

Also, try to fix the TestTLSServerWithLocalhostCertWithClientCertificateEnforcementUsingClientCA1 test by re-doing requests that have been reset by the server, but only if we were expecting an error. Note that this is not newly flaky, this comment has been there since the beginning:

// TODO: Investigate why we don't really receive the error about the
// bad certificate from the server side and just see connection
// closed/reset instead

Flaky tests run: https://github.com/grafana/dskit/actions/runs/11257895128 🟢

@julienduchesne julienduchesne marked this pull request as ready for review October 9, 2024 01:46
@julienduchesne julienduchesne force-pushed the julienduchesne/really-fix-member-tests branch from 63a41b2 to 27414ec Compare October 9, 2024 02:02
#599 went in a wrong direction, so I revert it here: it makes all `TestMultipleClients` tests flaky since not all members are necessarily joined after the updates

Instead, to fix the `TestMultipleClientsWithMixedLabelsAndExpectFailure` test, I add a couple more labeled members, that way, even if we reach 3 updates (which happened some times), we'll never get to 5 with a single member

Also, try to fix the `TestTLSServerWithLocalhostCertWithClientCertificateEnforcementUsingClientCA1` test by closing GRPC connections. Maybe this (golang/go#36700) is the issue?
@julienduchesne julienduchesne force-pushed the julienduchesne/really-fix-member-tests branch from 27414ec to 42fcc5d Compare October 9, 2024 02:06
@julienduchesne julienduchesne requested a review from a team October 9, 2024 03:55
@julienduchesne julienduchesne force-pushed the julienduchesne/really-fix-member-tests branch 2 times, most recently from 79ff135 to 5d8c8e8 Compare October 9, 2024 14:08
@julienduchesne julienduchesne force-pushed the julienduchesne/really-fix-member-tests branch from 5d8c8e8 to 3d87265 Compare October 9, 2024 14:52
Copy link
Contributor

@aldernero aldernero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@julienduchesne julienduchesne merged commit f4d4811 into main Oct 10, 2024
85 checks passed
@julienduchesne julienduchesne deleted the julienduchesne/really-fix-member-tests branch October 10, 2024 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants