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.

Sign up for GitHub

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
mcmire merged 2 commits into from
May 22, 2025
Merged

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
194f2b8
remote-feature-flag-controller: Fix flaky test
mcmire Apr 29, 2025
00b43cf
Merge branch 'main' into fix-rffc-flaky-test
mcmire May 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/remote-feature-flag-controller/src/utils/user-segmentation-utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,9 +130,9 @@ describe('user-segmentation-utils', () => {
distribution[Math.min(distributionIndex, 9)] += 1;
});

// Each range should have roughly 10% of the values and 30% deviation
// Each range should have roughly 10% of the values and 40% deviation
const expectedPerRange = samples / ranges.length;
const allowedDeviation = expectedPerRange * 0.3;
const allowedDeviation = expectedPerRange * 0.4;
Copy link
Contributor

@MajorLift MajorLift May 22, 2025 •
edited
Loading

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

@mcmire mcmire May 22, 2025

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.


// Check distribution
distribution.forEach((count) => {
Expand Down
Loading