Skip to content

remote-feature-flag-controller: Fix flaky test #5730

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
May 22, 2025
Merged

Conversation

mcmire
Copy link
Contributor

@mcmire mcmire commented Apr 29, 2025

Explanation

The test for generateDeterministicRandomNumber sometimes fails because it relies on the behavior of uuidv4, which is non-deterministic, and needs to be more lenient in the range of acceptable return values.

References

Changelog

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've communicated my changes to consumers by updating changelogs for packages I've changed, highlighting breaking changes as necessary
  • I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

The test for `generateDeterministicRandomNumber` sometimes fails because
it relies on the behavior of `uuidv4`, which is non-deterministic, and
needs to be more lenient in the range of acceptable return values.
@mcmire mcmire marked this pull request as ready for review April 29, 2025 19:05
@mcmire mcmire requested review from a team as code owners April 29, 2025 19:05
@mcmire mcmire requested a review from a team May 22, 2025 20:01
@mcmire
Copy link
Contributor Author

mcmire commented May 22, 2025

I've ran this test locally a few times and didn't get a failure, so I feel like this should help things.

@github-project-automation github-project-automation bot moved this to Needs dev review in PR review queue May 22, 2025
Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

LGTM

@github-project-automation github-project-automation bot moved this from Needs dev review to Review finalised - Ready to be merged in PR review queue May 22, 2025
Copy link
Contributor

@MajorLift MajorLift left a comment

Choose a reason for hiding this comment

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

Looks like a reasonable fix for flakiness.

const expectedPerRange = samples / ranges.length;
const allowedDeviation = expectedPerRange * 0.3;
const allowedDeviation = expectedPerRange * 0.4;
Copy link
Contributor

@MajorLift MajorLift May 22, 2025

Choose a reason for hiding this comment

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

Is 40% a safe range for the test's requirements? It seems like this might relax the test into validating a very-roughly uniform distribution.😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a great question. I'm not sure 😅 It seems like the range of values is wider than we expect. It also seems that if generateDeterministicRandomNumber is supposed to be deterministic, we don't have to use random UUIDs or user IDs in the test, we can use known values. But I assume that there is a specific reason we wrote the test this way and fixing it further seemed out of scope.

@DDDDDanica
Copy link
Contributor

LGTM. !

Copy link
Contributor

@cryptodev-2s cryptodev-2s left a comment

Choose a reason for hiding this comment

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

LGTM!

@mcmire mcmire merged commit 6369a54 into main May 22, 2025
203 checks passed
@mcmire mcmire deleted the fix-rffc-flaky-test branch May 22, 2025 20:44
@github-project-automation github-project-automation bot moved this from Review finalised - Ready to be merged to Merged, Closed or Archived in PR review queue May 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants