8000 Fix order-dependent flaky tests related to UTF-8 support by dg98 · Pull Request #1093 · prometheus/client_python · GitHub
[go: up one dir, main page]

Skip to content

Fix order-dependent flaky tests related to UTF-8 support #1093

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
merged 2 commits into from
Mar 3, 2025

Conversation

dg98
Copy link
Contributor
@dg98 dg98 commented Feb 24, 2025

For a university course, I tested this project for flaky tests, which are tests that can both pass or fail without changes to their code. I noticed that the tests changed in #1070 fail when their execution order is changed because the legacy validation is enabled in some tests but disabled in the test that follows. Since the execution order is not ensured, it's safer to disable them in the same test.
With these changes applied, the whole test suit passes every time even when executed in random order.

Signed-off-by: Dennis Gaebler <dennis.gaebler@uni-ulm.de>
Copy link
Member
@csmarchbanks csmarchbanks left a comment

Choose a reason for hiding this comment

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

Thanks!

The downside to this approach is that if one of the assertions raise then the next test will also fail since disable_legacy_validation() will no longer be called. A couple options would be to:

  1. Wrap the validation enablement/disabling with a With Statement Context Manager
  2. Create a test fixture to be used when we need to use legacy validation.

Signed-off-by: Dennis Gaebler <dennis.gaebler@uni-ulm.de>
@dg98
Copy link
Contributor Author
dg98 commented Mar 3, 2025

Thanks for your feedback, I didn't think of that. I added a context manager that enables and disables legacy validation and changed all uses of these functions to with statements

Copy link
Member
@csmarchbanks csmarchbanks left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@csmarchbanks csmarchbanks merged commit de8bb4a into prometheus:master Mar 3, 2025
11 checks passed
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
0