Implement #85440 (Random test ordering) #89082

smoelius · 2021-09-18T22:27:33Z

This PR adds --shuffle and --shuffle-seed options to libtest. The options are similar to the -shuffle option that was recently added to Go.

Here are the relevant parts of the help message:

        --shuffle       Run tests in random order
        --shuffle-seed SEED
                        Run tests in random order; seed the random number
                        generator with SEED
...
By default, the tests are run in alphabetical order. Use --shuffle or set
RUST_TEST_SHUFFLE to run the tests in random order. Pass the generated
"shuffle seed" to --shuffle-seed (or set RUST_TEST_SHUFFLE_SEED) to run the
tests in the same order again. Note that --shuffle and --shuffle-seed do not
affect whether the tests are run in parallel.

Is an RFC needed for this?

rust-highfive · 2021-09-18T22:27:35Z

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

smoelius · 2021-09-19T00:07:42Z

r? @ghost

smoelius · 2021-09-19T00:08:32Z

Converting this to a WIP while I figure out this randomness situation.

lukaslueg · 2021-09-19T12:24:50Z

library/test/src/cli.rs

+        shuffle_seed = match env::var("RUST_TEST_SHUFFLE_SEED") {
+            Ok(val) => match val.parse::<u64>() {
+                Ok(n) => Some(n),
+                Err(_) => panic!("RUST_TEST_SHUFFLE_SEED is `{}`, should be a number.", val),


Why the panic! here instead of returning an Error as above?

I was trying to be consistent with get_concurrency: https://github.com/rust-lang/rust/blob/0a0688e6f51229d2860af6317adaef378f4b45a5/library/test/src/helpers/concurrency.rs#L9

But I don't have a strong opinion, and I would be happy to change this.

smoelius · 2021-09-20T01:15:32Z

r? @kennytm

smoelius · 2021-09-20T01:16:15Z

@kennytm I think you can start reviewing this whenever you are ready.

library/test/src/cli.rs

kennytm · 2021-09-20T03:01:21Z

library/test/src/helpers/shuffle.rs

+    }
+
+    fn rand_u64(&mut self) -> u64 {
+        self.state = calculate_hash(&(self.state, self.extra));


can we use a proper RNG library like fastrand or oorandom or rand

there is no proof that state = sip13_hash((state, extra)) is a good RNG.

(and additionally rand_u64() % n produces biased result)

can we use a proper RNG library like fastrand or oorandom or rand

I tried rand and oorandom actually. In both cases, I got can't find crate for core errors.

My impression (based on this, this, and this) is that a crate has to go out of its way to be a dependency of a builtin library. And none of those crates seem to do that. (I could be misunderstanding though.)

Note that oorandom barely uses core, but it does use it for Range (here).

there is no proof that state = sip13_hash((state, extra)) is a good RNG.

(and additionally rand_u64() % n produces biased result)

Your points are very well taken. However, I don't think this random number generator needs to be "strong." Put another way, I think the risk of an attacker trying to manipulate the test ordering is low. I am open to counterarguments though.

(Thanks for doing this, BTW.)

smoelius · 2021-09-30T01:56:19Z

@kennytm Since we seem to have reached a pause, I took the opportunity to do some squashing.

Please let me know if there is anything I can do to make reviewing this PR easier.

library/test/src/cli.rs

kennytm · 2021-10-03T09:19:07Z

please also update /src/doc/rustc/src/tests/index.md to document the new unstable option.

r=me after that.

smoelius · 2021-10-04T22:59:48Z

@kennytm I added some documentation. Please let me know if it seems inadequate.

Thank you again for all of your trouble.

kennytm

basically I just want a link to a tracking issue.

or you could file a different issue for tracking if you like 🙃

kennytm · 2021-10-05T18:15:00Z

src/doc/rustc/src/tests/index.md

+run the tests in random order sequentially, use `--shuffle --test-threads 1`.
+
+⚠️ 🚧 This option is [unstable](#unstable-options), and requires the `-Z
+unstable-options` flag.


Suggested change

unstable-options` flag.

unstable-options` flag. See [tracking issue

#85440](https://github.com/rust-lang/rust/issues/85440) for more

information.

kennytm · 2021-10-05T18:15:14Z

src/doc/rustc/src/tests/index.md

+variable.
+
+⚠️ 🚧 This option is [unstable](#unstable-options), and requires the `-Z
+unstable-options` flag.


Suggested change

unstable-options` flag.

unstable-options` flag. See [tracking issue

#85440](https://github.com/rust-lang/rust/issues/85440) for more

information.

joshtriplett · 2021-10-05T20:51:59Z

Might be worth looking at lld's --shuffle-sections for comparison.

smoelius · 2021-10-06T00:49:23Z

basically I just want a link to a tracking issue.

or you could file a different issue for tracking if you like upside_down_face

Sorry, I misunderstood how these things work.

I opened a new issue so that I can edit it if I have to.

Thanks again, @kennytm.

kennytm · 2021-10-07T11:48:32Z

@bors r+

bors · 2021-10-07T11:48:33Z

📌 Commit ecf4741 has been approved by kennytm

Implement rust-lang#85440 (Random test ordering) This PR adds `--shuffle` and `--shuffle-seed` options to `libtest`. The options are similar to the [`-shuffle` option](https://github.com/golang/go/blob/c894b442d1e5e150ad33fa3ce13dbfab1c037b3a/src/testing/testing.go#L1482-L1499) that was recently added to Go. Here are the relevant parts of the help message: ``` --shuffle Run tests in random order --shuffle-seed SEED Run tests in random order; seed the random number generator with SEED ... By default, the tests are run in alphabetical order. Use --shuffle or set RUST_TEST_SHUFFLE to run the tests in random order. Pass the generated "shuffle seed" to --shuffle-seed (or set RUST_TEST_SHUFFLE_SEED) to run the tests in the same order again. Note that --shuffle and --shuffle-seed do not affect whether the tests are run in parallel. ``` Is an RFC needed for this?

…ingjubilee Rollup of 8 pull requests Successful merges: - rust-lang#87918 (Enable AutoFDO.) - rust-lang#88137 (On macOS, make strip="symbols" not pass any options to strip) - rust-lang#88772 (Fixed confusing wording on Result::map_or_else.) - rust-lang#89025 (Implement `#[link_ordinal(n)]`) - rust-lang#89082 (Implement rust-lang#85440 (Random test ordering)) - rust-lang#89288 (Wrapper for `-Z gcc-ld=lld` to invoke rust-lld with the correct flavor) - rust-lang#89476 (Correct decoding of foreign expansions during incr. comp.) - rust-lang#89622 (Use correct edition for panic in [debug_]assert!().) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned kennytm Sep 18, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 18, 2021

This comment has been minimized.

Sign in to view

smoelius changed the title ~~Implement #85440~~ Implement #85440 (Random test ordering) Sep 18, 2021

This comment has been minimized.

Sign in to view

smoelius changed the title ~~Implement #85440 (Random test ordering)~~ WIP: Implement #85440 (Random test ordering) Sep 19, 2021

lukaslueg reviewed Sep 19, 2021

View reviewed changes

smoelius changed the title ~~WIP: Implement #85440 (Random test ordering)~~ Implement #85440 (Random test ordering) Sep 20, 2021

kennytm reviewed Sep 20, 2021

View reviewed changes

smoelius added 2 commits September 29, 2021 21:51

Implement rust-lang#85440

fa23d4f

Add tests

a6738c7

smoelius force-pushed the master branch from 520bbf6 to a6738c7 Compare September 30, 2021 01:53

kennytm reviewed Sep 30, 2021

View reviewed changes

library/test/src/cli.rs Show resolved Hide resolved

Check allow_unstable before checking environment variables

32b6ac5

This comment has been minimized.

Sign in to view

smoelius force-pushed the master branch from e9b8686 to efc6ba5 Compare October 5, 2021 00:55

Add documentation

e16e15f

smoelius force-pushed the master branch from efc6ba5 to e16e15f Compare October 5, 2021 00:58

kennytm approved these changes Oct 5, 2021

View reviewed changes

smoelius mentioned this pull request Oct 6, 2021

Tracking Issue for shuffle and shuffle-seed #89583

Open

3 tasks

Add tracking issue

ecf4741

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 7, 2021

Manishearth mentioned this pull request Oct 7, 2021

Rollup of 10 pull requests #89645

Closed

Manishearth mentioned this pull request Oct 8, 2021

Rollup of 9 pull requests #89656

Closed

workingjubilee mentioned this pull request Oct 8, 2021

Rollup of 8 pull requests #89659

Merged

bors merged commit 37f17bc into rust-lang:master Oct 8, 2021

rustbot added this to the 1.57.0 milestone Oct 8, 2021

smoelius mentioned this pull request Mar 10, 2023

Random test ordering for cargo test #85440

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement #85440 (Random test ordering) #89082

Implement #85440 (Random test ordering) #89082

smoelius commented Sep 18, 2021 •

edited

Loading

rust-highfive commented Sep 18, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

smoelius commented Sep 19, 2021 •

edited

Loading

smoelius commented Sep 19, 2021

lukaslueg Sep 19, 2021

smoelius Sep 19, 2021

smoelius commented Sep 20, 2021

smoelius commented Sep 20, 2021

kennytm Sep 20, 2021

smoelius Sep 20, 2021 •

edited

Loading

smoelius commented Sep 30, 2021

kennytm commented Oct 3, 2021

smoelius commented Oct 4, 2021

This comment has been minimized.

kennytm left a comment •

edited

Loading

kennytm Oct 5, 2021

kennytm Oct 5, 2021

joshtriplett commented Oct 5, 2021

smoelius commented Oct 6, 2021

kennytm commented Oct 7, 2021

bors commented Oct 7, 2021

-unstable-options` flag.
+unstable-options` flag. See [tracking issue
+#85440](https://github.com/rust-lang/rust/issues/85440) for more
+information.

Implement #85440 (Random test ordering) #89082

Implement #85440 (Random test ordering) #89082

Conversation

smoelius commented Sep 18, 2021 • edited Loading

rust-highfive commented Sep 18, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

smoelius commented Sep 19, 2021 • edited Loading

smoelius commented Sep 19, 2021

lukaslueg Sep 19, 2021

Choose a reason for hiding this comment

smoelius Sep 19, 2021

Choose a reason for hiding this comment

smoelius commented Sep 20, 2021

smoelius commented Sep 20, 2021

kennytm Sep 20, 2021

Choose a reason for hiding this comment

smoelius Sep 20, 2021 • edited Loading

Choose a reason for hiding this comment

smoelius commented Sep 30, 2021

kennytm commented Oct 3, 2021

smoelius commented Oct 4, 2021

This comment has been minimized.

kennytm left a comment • edited Loading

Choose a reason for hiding this comment

kennytm Oct 5, 2021

Choose a reason for hiding this comment

kennytm Oct 5, 2021

Choose a reason for hiding this comment

joshtriplett commented Oct 5, 2021

smoelius commented Oct 6, 2021

kennytm commented Oct 7, 2021

bors commented Oct 7, 2021

smoelius commented Sep 18, 2021 •

edited

Loading

smoelius commented Sep 19, 2021 •

edited

Loading

smoelius Sep 20, 2021 •

edited

Loading

kennytm left a comment •

edited

Loading