test_runner: support test order randomization#61747
test_runner: support test order randomization#61747pmarchini wants to merge 5 commits intonodejs:mainfrom
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
pmarchini
force-pushed
the
test_runner/deterministic-test-randomizer
branch
2 times, most recently
from
5556788 to
e6f9a59
Compare
pmarchini
added
test_runner
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #61747 +/- ##
==========================================
- Coverage 91.63% 89.66% -1.97%
==========================================
Files 337 676 +339
Lines 140376 206533 +66157
Branches 21768 39554 +17786
==========================================
+ Hits 128627 185180 +56553
- Misses 11527 13499 +1972
- Partials 222 7854 +7632
🚀 New features to boost your workflow:
|
pmarchini
force-pushed
the
test_runner/deterministic-test-randomizer
branch
from
e6f9a59 to
b3b2388
Compare
pmarchini
force-pushed
the
test_runner/deterministic-test-randomizer
branch
from
b3b2388 to
08db017
Compare
pmarchini
force-pushed
the
test_runner/deterministic-test-randomizer
branch
2 times, most recently
from
08db017 to
4d626b6
Compare
JakobJingleheimer
left a comment
JakobJingleheimer
left a comment
There was a problem hiding this comment.
🙌
I see I'm late to the party. None of my comments are pressing :)
| ``` | ||
|
|
||
| Using suite-style APIs such as `describe()`/`it()` or `suite()`/`test()` | ||
| still allows randomization, because sibling tests are queued together. |
There was a problem hiding this comment.
nit: I thiiink it should be "enqueued"? "queued" together means they wait together; enqueued together means they're added to a queue one-by-one setting up a sequence.
| @@ -191,6 +198,12 @@ function getRunArgs(path, { forceExit, | |||
| if (rerunFailuresFilePath) { | |||
| ArrayPrototypePush(runArgs, `--test-rerun-failures=${rerunFailuresFilePath}`); | |||
| } | |||
| if (randomize === true) { | |||
There was a problem hiding this comment.
Related to the skip and todo discussion: should this check truthiness rather than strict?
|
I couldn't find the actualCI failure, but Richard did: https://ci.nodejs.org/job/node-test-binary-windows-js-suites/38847/RUN_SUBSET=3,nodes=win11-arm64-COMPILED_BY-vs2022_clang-arm64/testReport/junit/(root)/parallel/test_domain_abort_on_uncaught/ The failure looks legit? |
JakobJingleheimer
added
commit-queue
pmarchini
force-pushed
the
test_runner/deterministic-test-randomizer
branch
from
84b6260 to
d013ca5
Compare
github-actions
bot
removed
the
request-ci
pmarchini
removed
author ready
| ✔ describe-it 1-1-1 (*ms) | ||
| ✔ describe-it 1-1-3 (*ms) | ||
| ✔ describe-it 1-1-2 (*ms) | ||
| ✔ describe-it 1-1 (*ms) | ||
| ▶ describe-it 1-3 | ||
| ✔ describe-it 1-3-1 (*ms) | ||
| ✔ describe-it 1-3-3 (*ms) | ||
| ✔ describe-it 1-3-2 (*ms) | ||
| ✔ describe-it 1-3 (*ms) | ||
| ▶ describe-it 1-2 |
There was a problem hiding this comment.
Checking the pattern I've noticed that the first test is always executed before the shuffle.
A fix is coming!
thanks @hiimjako
Ethan-Arrowood
left a comment
Ethan-Arrowood
left a comment
There was a problem hiding this comment.
lgtm - there is a lint failure and I have some docs edits but wouldn't consider them blocking
| enable this mode. | ||
|
|
||
| ```bash | ||
| node --test --test-randomize |
There was a problem hiding this comment.
Are you sure this is valid? I thought everything after the --test is considered as an entry path?
| so `--test-randomize` is optional when a seed is provided: | ||
|
|
||
| ```bash | ||
| node --test --test-randomize --test-random-seed=12345 |
There was a problem hiding this comment.
| node --test --test-randomize --test-random-seed=12345 | |
| node --test --test-random-seed=12345 |
There was a problem hiding this comment.
this change is in reference to the previous line of text that states the --test-randomize is not needed when --test-random-seed is provided.
| * `randomize` {boolean} Randomize execution order for test files and queued tests. | ||
| This option is not supported with `watch: true`. | ||
| **Default:** `false`. | ||
| * `randomSeed` {number} Seed used when randomizing execution order. If this |
There was a problem hiding this comment.
Lets mention the 0 to 2^32 - 1 limit here.
6 participants
This PR adds deterministic randomization to the Node.js test runner at both levels:
--test-randomizeenables randomized execution, and--test-random-seed=<seed>replays the same order deterministically (and implicitly enables randomization).When no seed is provided, one is generated and printed in the summary so runs are easy to reproduce.