Skip to content

Conversation

dsabsay
Copy link
Contributor

@dsabsay dsabsay commented Jan 24, 2025

What this PR does:

These tests were flaky: sometimes they get evaluated before the test
issues the GET request, sometimes they do not.

The reproducers are:

go test -count=1000 -run TestRuler_rules_limit ./pkg/ruler/
go test -count=1000 -run TestRuler_rules ./pkg/ruler/
go test -count=1000 -run TestRuler_rules_special_characters ./pkg/ruler/

This fixes the flakiness by removing evaluation-related fields from the response before comparing.

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

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Daniel Sabsay added 2 commits January 24, 2025 14:36
These tests were flaky: sometimes they get evaluated before the test
issues the GET request, sometimes they do not.

Signed-off-by: Daniel Sabsay <[email protected]>
Signed-off-by: Daniel Sabsay <[email protected]>
@dosubot dosubot bot added component/rules Bits & bobs todo with rules and alerts: the ruler, config service etc. type/flaky-test labels Jan 24, 2025
@dsabsay
Copy link
Contributor Author

dsabsay commented Jan 24, 2025

I realize this adds quite a bit of code to fix a very small problem... It could be simplified by comparing go structures directly using go-cmp with custom comparison functions to ignore certain fields. I like that the current tests compare JSON strings directly since that is the user-facing output, which is why I've done it like this. If anyone disagrees or sees a better way, I'm happy to adjust it.

@yeya24 yeya24 requested a review from rajagopalanand January 26, 2025 08:49
Copy link
Contributor

@rajagopalanand rajagopalanand left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@friedrichg friedrichg left a comment

Choose a reason for hiding this comment

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

thanks

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jan 31, 2025
Copy link
Member

@alanprot alanprot left a comment

Choose a reason for hiding this comment

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

thanks

@alanprot alanprot merged commit 9e4fd0d into cortexproject:master Feb 4, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/rules Bits & bobs todo with rules and alerts: the ruler, config service etc. lgtm This PR has been approved by a maintainer size/L type/flaky-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky test: TestRuler_rules_special_characters
4 participants