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(core): make randomString deterministic and allow larger values #2342

Conversation

bernardobridge
Copy link
Contributor

@bernardobridge bernardobridge commented Nov 30, 2023

Description

There are two issues with the current $randomString function:

  • It will sometimes return a different length than specified, due to trailing zeroes
  • It actually doesn't work for large lengths (n>10 I believe). It just always returns up to 10 chars

I tried and benchmarked quite a few possible variations of the function (e.g. using crypto.randomBytes, lodash utilities, etc). This solution was by far the fastest I got, even surpassing the existing function.

Notes:

  • This solution allows capital letters, but we can remove that if we want (I believe the previous solution didn't).
  • Defaults to length=10 if no length is specified to keep it backwards compatible, since the previous function implicitly did this

Testing

  • Added thorough unit test that checks for the regression. I also ran it with the old function, and it catches the issue (i.e. fails successfully)
  • Benchmarks to make sure it works at load-test scale

Pre-merge checklist

  • Does this require an update to the docs? These functions aren't mentioned in the docs. I think maybe we should add $randomString and $randomNumber to the docs as part of this
  • Does this require a changelog entry? Yes

@bernardobridge bernardobridge requested a review from a team November 30, 2023 19:24
@bernardobridge bernardobridge merged commit d7109dd into main Jan 18, 2024
10 checks passed
@bernardobridge bernardobridge deleted the bernardobridge/art-1457-randomstring-utility-from-cli-doesnt-produce-deterministic branch January 18, 2024 13:37
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.

None yet

1 participant