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

Fix flapping acceptance test #2557

Merged
merged 2 commits into from
Apr 29, 2021

Conversation

QuentinBisson
Copy link
Contributor

This PR fixes the flapping TestRetry acceptance test:

for i in {1..20}
do
  go test . -run TestRetry -count 1
done
ok      github.com/prometheus/alertmanager/test/with_api_v2/acceptance  6.514s
ok      github.com/prometheus/alertmanager/test/with_api_v2/acceptance  6.515s
ok      github.com/prometheus/alertmanager/test/with_api_v2/acceptance  6.513s
ok      github.com/prometheus/alertmanager/test/with_api_v2/acceptance  6.514s
ok      github.com/prometheus/alertmanager/test/with_api_v2/acceptance  6.512s
ok      github.com/prometheus/alertmanager/test/with_api_v2/acceptance  6.513s
ok      github.com/prometheus/alertmanager/test/with_api_v2/acceptance  6.514s
ok      github.com/prometheus/alertmanager/test/with_api_v2/acceptance  6.515s
ok      github.com/prometheus/alertmanager/test/with_api_v2/acceptance  6.512s
ok      github.com/prometheus/alertmanager/test/with_api_v2/acceptance  6.514s
ok      github.com/prometheus/alertmanager/test/with_api_v2/acceptance  6.511s
ok      github.com/prometheus/alertmanager/test/with_api_v2/acceptance  6.513s
ok      github.com/prometheus/alertmanager/test/with_api_v2/acceptance  6.514s
ok      github.com/prometheus/alertmanager/test/with_api_v2/acceptance  6.514s
ok      github.com/prometheus/alertmanager/test/with_api_v2/acceptance  6.513s
ok      github.com/prometheus/alertmanager/test/with_api_v2/acceptance  6.514s
ok      github.com/prometheus/alertmanager/test/with_api_v2/acceptance  6.513s
ok      github.com/prometheus/alertmanager/test/with_api_v2/acceptance  6.515s
ok      github.com/prometheus/alertmanager/test/with_api_v2/acceptance  6.514s
ok      github.com/prometheus/alertmanager/test/with_api_v2/acceptance  6.515s

@QuentinBisson QuentinBisson force-pushed the fix-flapping-acceptance-test branch 2 times, most recently from afae31e to 9fbacd8 Compare April 26, 2021 09:19
@QuentinBisson
Copy link
Contributor Author

cc @roidelapluie as we talked about it in #1682

I am not sure why the checks are not reported though

@beorn7 beorn7 requested a review from roidelapluie April 26, 2021 21:47
@roidelapluie
Copy link
Member

Is it only flapping in go 1.16 or did we miss it?

@QuentinBisson
Copy link
Contributor Author

I cannot reproduce it with go 1.15.11. I tested the code with both group_interval: 1s and group_interval: 2s and the test are running successfully in both cases

@roidelapluie
Copy link
Member

Can you please also update the .circleci.yml to use go 1.16 for the acceptance tests?

Thanks!!

@QuentinBisson QuentinBisson force-pushed the fix-flapping-acceptance-test branch 2 times, most recently from eb63327 to eaafff3 Compare April 28, 2021 09:13
@QuentinBisson
Copy link
Contributor Author

This is causing other tests to fail now :(

@roidelapluie
Copy link
Member

I have relaunched the tests and they fail with another error..

@QuentinBisson QuentinBisson force-pushed the fix-flapping-acceptance-test branch 6 times, most recently from b0b5dd5 to b0157dc Compare April 28, 2021 12:51
@QuentinBisson
Copy link
Contributor Author

After some trials, I see the text passing but the build is failing because go.sum is changing after the make proto step

@roidelapluie
Copy link
Member

prometheus/prometheus@75e505b#diff-beb85cf1fc0ad937994d001f06b66b2a0e87a9416cc722aa97849b0d6ba17cb0

I copy back and forth the go.sum in prometheus. We could do the same.

Signed-off-by: QuentinBisson <quentin@giantswarm.io>
@QuentinBisson QuentinBisson force-pushed the fix-flapping-acceptance-test branch from b0157dc to 4aea456 Compare April 28, 2021 13:04
@QuentinBisson
Copy link
Contributor Author

Oh interesting :)

@QuentinBisson
Copy link
Contributor Author

This seems to have fixed it :D

Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

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

Just a tiny nit.

I'm missing the context to say if the go.sum hack is kosher and if the increased group interval makes sense. But in doubt, I'd say let's merge this and release. We can always improve/revert/…

scripts/genproto.sh Outdated Show resolved Hide resolved
Signed-off-by: QuentinBisson <quentin@giantswarm.io>
@QuentinBisson QuentinBisson force-pushed the fix-flapping-acceptance-test branch from f0cd1c2 to 9c4ebab Compare April 29, 2021 08:44
@beorn7
Copy link
Member

beorn7 commented Apr 29, 2021

Thanks @QuentinBisson .

@roidelapluie @simonpasquier I'm merging this now, to unblock the much needed release. I won't have time to shepherd it before end of next week. Will any of you be able to?

@beorn7 beorn7 merged commit 9a2df80 into prometheus:master Apr 29, 2021
@QuentinBisson QuentinBisson deleted the fix-flapping-acceptance-test branch April 30, 2021 10:38
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.

3 participants