-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
jest-circus runs children in shuffled order #12922
Conversation
Hi @jhwang98! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super exciting stuff!
Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
Hi, any update on future of this, will this PR get adopted in the future? |
Hi @SimenB is there anything else that I need to change before we can get this merged? Happy to improve this PR 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code is shaping up well!
Main thing missing is tests and docs 🙂 Tests for the shffleArray
function would be great, plus of course that seed
works and works consistently. Probably integration test?
Also, can you update the changelog?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this.
If that is only jest-circus
option, it would be good to mentioned that in documentation. For example, like here: https://github.com/facebook/jest/blob/e57496455437a1d3d0d9cafc4cbc6a8eb90bcfa7/docs/GlobalAPI.md?plain=1#L752-L756
docs/CLI.md
Outdated
@@ -28,6 +28,18 @@ Run tests related to changed files based on hg/git (uncommitted files): | |||
jest -o | |||
``` | |||
|
|||
Run tests in a file in a random order (this will print the seed used for random number generation): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get the idea, but what if this would be moved to --randomize
? If a user is interested to read about --randomize
, current text does not provide usage details. It is not really clear where to look for it and the details here are important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've copied what I've written here to be part of the CLI description of the --randomize
option.
I've done the same with --seed
as well.
Was this what you were looking for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I was trying to say that this usage information could be more useful, if placed next to --randomize
description. That’s there I would expect to find it, but this is just an opinion (;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh gotcha. Yeah that makes sense.
I initially put a description near the beginning because this might be a popular features. On further thinking it's probably not that popular.
Hey @SimenB were there any additional changes you want made to this PR? |
@jhwang98 I just released pure-rand v6, no change expected on your code but probably worth to update it before merging. It brings a slight performance benefit on uniform distributions. |
This is the second time the tests have been failing 😕. I'm not sure what the problem could be here. |
Hey @jhwang98 , the tests are failing because of a conflict between the lock file (yarn.lock) and your package.json. I took a quick glance & it looks like when you merged the recent changes from main, those included a yarn.lock file that overwrote your previous one. As a result, when the CI/CD install script runs in the GitHub actions build environment, it sees that your package.json is instructing it to install pure-rand, but finds that that package doesn’t exist in the corresponding yarn.lock file, and throws an error as a result. Running |
The check that failed was for installing Node. Don't think it was related to my change. |
I recently was sent this and it made me think of this PR 😅 let's get it in! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a few nits 🙂
const upperBoundSeedValue = 2 ** 31; | ||
if (seed < -upperBoundSeedValue || seed > upperBoundSeedValue - 1) { | ||
throw new Error( | ||
`seed value must be between \`-0x80000000\` and \`0x7fffffff\` inclusive instead it is ${seed}`, | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this check? isn't seed
from config already guaranteed to be within these bounds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess so.
I was thinking if the shuffleArray
ever gets used elsewhere. If not then I guess we're fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not exported outside of jest circus, so we should be fine
Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
Resolves #4386
Test plan
This PR is a work in progress based on #4386