Skip to content
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

fix: update ruby SDK to increase the possible random numbers used for stickiness id #200

Merged

Conversation

thomasheartman
Copy link
Collaborator

@thomasheartman thomasheartman commented Sep 19, 2024

Same fix as done in some other sdks, such as the node one at
Unleash/unleash-client-node#417

Fixes an issue where 1% rollout would not yield any results with
random rollout for certain group ids

… stickiness id

Same fix as done in some other sdks, such as the node one at
https://github.com/Unleash/unleash-client-node/pull/417o

Fixes an issue where 1% rollout would not yield any results with
random rollout for certain group ids
@ivarconr
Copy link
Member

did you consider a unit test for this?
https://github.com/Unleash/unleash-client-node/blob/main/src/test/strategy/gradual-rollout-random.test.ts#L10-L33

@coveralls
Copy link

coveralls commented Sep 19, 2024

Pull Request Test Coverage Report for Build 10956081535

Details

  • 15 of 15 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.09%) to 97.25%

Totals Coverage Status
Change from base Build 10919587464: 0.09%
Covered Lines: 2582
Relevant Lines: 2655

💛 - Coveralls

@thomasheartman
Copy link
Collaborator Author

I did not (because there was none in the node sdk where I took it from), but lemme see what I can cook up

@thomasheartman
Copy link
Collaborator Author

@ivarconr : that test does not test the fix in the node SDK, which was only applied to the src/strategy/flexible-rollout-strategy.ts file. In fact, the src/strategy/gradual-rollout-random.ts, which this test presumably tests, still only goes to 101. And interestingly, as a follow-up, if I increase the multiplication to 10k (as we've done in the other file), that test starts failing.

However, the same thing in Ruby does appear to work (fails at 100, works at 10k). I've added the test; will merge when everything goes green unless anyone speaks up.

@thomasheartman thomasheartman merged commit bd74691 into main Sep 20, 2024
37 checks passed
@thomasheartman thomasheartman deleted the fix/1-2864-investigate-rollout-distribution-issues branch September 20, 2024 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants