Skip to content

fix a flaky test in purger #2841

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

Merged

Conversation

sandeepsukhani
Copy link
Contributor

What this PR does:
In TestPurger_Metrics test I was starting the purger service here after pulling the delete requests for processing here which usually should never happen because we always start the service first. This was done to capture an expected metric value.
This was causing Purger to work in an unexpected way causing it to fail the test sometimes. I have changed the test to stop existing purger and create a new one to start the service for delete request processing.

Which issue(s) this PR fixes:
Fixes #2840

Checklist

  • Tests updated

Signed-off-by: Sandeep Sukhani <[email protected]>
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! LGTM

@@ -414,9 +417,17 @@ func TestPurger_Metrics(t *testing.T) {
require.InDelta(t, float64(2*86400), testutil.ToFloat64(purger.metrics.oldestPendingDeleteRequestAgeSeconds), 1)
require.Equal(t, float64(2), testutil.ToFloat64(purger.metrics.pendingDeleteRequestsCount))

// start loop to process requests
// stop the existing purger
require.NoError(t, services.StopAndAwaitTerminated(context.Background(), purger))
Copy link
Contributor

Choose a reason for hiding this comment

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

At the beginning of the test there's a deferred call to purger.StopAsync(). Given we're now terminating it here, I believe that call should be removed.

Copy link
Contributor

@jtlisi jtlisi left a comment

Choose a reason for hiding this comment

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

LGTM

@pracucci pracucci merged commit dc51be3 into cortexproject:master Jul 8, 2020
@sandeepsukhani sandeepsukhani deleted the fix-flaky-purger-test branch January 7, 2021 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flake in TestPurger_Metrics
3 participants