-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Implement #85440 (Random test ordering) #89082
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,8 @@ pub struct TestOpts { | |
pub nocapture: bool, | ||
pub color: ColorConfig, | ||
pub format: OutputFormat, | ||
pub shuffle: bool, | ||
pub shuffle_seed: Option<u64>, | ||
pub test_threads: Option<usize>, | ||
pub skip: Vec<String>, | ||
pub time_options: Option<TestTimeOptions>, | ||
|
@@ -138,6 +140,13 @@ fn optgroups() -> getopts::Options { | |
|
||
`CRITICAL_TIME` here means the limit that should not be exceeded by test. | ||
", | ||
) | ||
.optflag("", "shuffle", "Run tests in random order") | ||
.optopt( | ||
"", | ||
"shuffle-seed", | ||
"Run tests in random order; seed the random number generator with SEED", | ||
"SEED", | ||
); | ||
opts | ||
} | ||
|
@@ -155,6 +164,12 @@ By default, all tests are run in parallel. This can be altered with the | |
--test-threads flag or the RUST_TEST_THREADS environment variable when running | ||
tests (set it to 1). | ||
|
||
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. | ||
|
||
All tests have their standard output and standard error captured by default. | ||
This can be overridden with the --nocapture flag or setting RUST_TEST_NOCAPTURE | ||
environment variable to a value other than "0". Logging is not captured by default. | ||
|
@@ -218,6 +233,21 @@ macro_rules! unstable_optflag { | |
}}; | ||
} | ||
|
||
// Gets the option value and checks if unstable features are enabled. | ||
macro_rules! unstable_optopt { | ||
($matches:ident, $allow_unstable:ident, $option_name:literal) => {{ | ||
let opt = $matches.opt_str($option_name); | ||
if !$allow_unstable && opt.is_some() { | ||
return Err(format!( | ||
"The \"{}\" option is only accepted on the nightly compiler with -Z unstable-options", | ||
$option_name | ||
)); | ||
} | ||
|
||
opt | ||
}}; | ||
} | ||
|
||
// Implementation of `parse_opts` that doesn't care about help message | ||
// and returns a `Result`. | ||
fn parse_opts_impl(matches: getopts::Matches) -> OptRes { | ||
|
@@ -227,6 +257,8 @@ fn parse_opts_impl(matches: getopts::Matches) -> OptRes { | |
let force_run_in_process = unstable_optflag!(matches, allow_unstable, "force-run-in-process"); | ||
let exclude_should_panic = unstable_optflag!(matches, allow_unstable, "exclude-should-panic"); | ||
let time_options = get_time_options(&matches, allow_unstable)?; | ||
let shuffle = get_shuffle(&matches, allow_unstable)?; | ||
let shuffle_seed = get_shuffle_seed(&matches, allow_unstable)?; | ||
|
||
let include_ignored = matches.opt_present("include-ignored"); | ||
let quiet = matches.opt_present("quiet"); | ||
|
@@ -260,6 +292,8 @@ fn parse_opts_impl(matches: getopts::Matches) -> OptRes { | |
nocapture, | ||
color, | ||
format, | ||
shuffle, | ||
shuffle_seed, | ||
test_threads, | ||
skip, | ||
time_options, | ||
|
@@ -303,6 +337,46 @@ fn get_time_options( | |
Ok(options) | ||
} | ||
|
||
fn get_shuffle(matches: &getopts::Matches, allow_unstable: bool) -> OptPartRes<bool> { | ||
let mut shuffle = unstable_optflag!(matches, allow_unstable, "shuffle"); | ||
if !shuffle { | ||
shuffle = match env::var("RUST_TEST_SHUFFLE") { | ||
Ok(val) => &val != "0", | ||
Err(_) => false, | ||
}; | ||
} | ||
|
||
Ok(shuffle) | ||
} | ||
|
||
fn get_shuffle_seed(matches: &getopts::Matches, allow_unstable: bool) -> OptPartRes<Option<u64>> { | ||
let mut shuffle_seed = match unstable_optopt!(matches, allow_unstable, "shuffle-seed") { | ||
Some(n_str) => match n_str.parse::<u64>() { | ||
Ok(n) => Some(n), | ||
Err(e) => { | ||
return Err(format!( | ||
"argument for --shuffle-seed must be a number \ | ||
(error: {})", | ||
e | ||
)); | ||
} | ||
}, | ||
None => None, | ||
}; | ||
|
||
if shuffle_seed.is_none() { | ||
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), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was trying to be consistent with But I don't have a strong opinion, and I would be happy to change this. |
||
}, | ||
Err(_) => None, | ||
}; | ||
} | ||
|
||
Ok(shuffle_seed) | ||
} | ||
|
||
fn get_test_threads(matches: &getopts::Matches) -> OptPartRes<Option<usize>> { | ||
let test_threads = match matches.opt_str("test-threads") { | ||
Some(n_str) => match n_str.parse::<usize>() { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,3 +5,4 @@ pub mod concurrency; | |
pub mod exit_code; | ||
pub mod isatty; | ||
pub mod metrics; | ||
pub mod shuffle; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
use crate::cli::TestOpts; | ||
use crate::types::{TestDescAndFn, TestId, TestName}; | ||
use std::collections::hash_map::DefaultHasher; | ||
use std::hash::Hasher; | ||
use std::time::{SystemTime, UNIX_EPOCH}; | ||
|
||
pub fn get_shuffle_seed(opts: &TestOpts) -> Option<u64> { | ||
opts.shuffle_seed.or_else(|| { | ||
if opts.shuffle { | ||
Some( | ||
SystemTime::now() | ||
.duration_since(UNIX_EPOCH) | ||
.expect("Failed to get system time") | ||
.as_nanos() as u64, | ||
) | ||
} else { | ||
None | ||
} | ||
}) | ||
} | ||
|
||
pub fn shuffle_tests(shuffle_seed: u64, tests: &mut [(TestId, TestDescAndFn)]) { | ||
let test_names: Vec<&TestName> = tests.iter().map(|test| &test.1.desc.name).collect(); | ||
let test_names_hash = calculate_hash(&test_names); | ||
let mut rng = Rng::new(shuffle_seed, test_names_hash); | ||
shuffle(&mut rng, tests); | ||
} | ||
|
||
// `shuffle` is from `rust-analyzer/src/cli/analysis_stats.rs`. | ||
fn shuffle<T>(rng: &mut Rng, slice: &mut [T]) { | ||
for i in 0..slice.len() { | ||
randomize_first(rng, &mut slice[i..]); | ||
} | ||
|
||
fn randomize_first<T>(rng: &mut Rng, slice: &mut [T]) { | ||
assert!(!slice.is_empty()); | ||
let idx = rng.rand_range(0..slice.len() as u64) as usize; | ||
slice.swap(0, idx); | ||
} | ||
} | ||
|
||
struct Rng { | ||
state: u64, | ||
extra: u64, | ||
} | ||
|
||
impl Rng { | ||
fn new(seed: u64, extra: u64) -> Self { | ||
Self { state: seed, extra } | ||
} | ||
|
||
fn rand_range(&mut self, range: core::ops::Range<u64>) -> u64 { | ||
self.rand_u64() % (range.end - range.start) + range.start | ||
} | ||
|
||
fn rand_u64(&mut self) -> u64 { | ||
self.state = calculate_hash(&(self.state, self.extra)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we use a proper RNG library like there is no proof that (and additionally There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I tried 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
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.) |
||
self.state | ||
} | ||
} | ||
|
||
// `calculate_hash` is from `core/src/hash/mod.rs`. | ||
fn calculate_hash<T: core::hash::Hash>(t: &T) -> u64 { | ||
let mut s = DefaultHasher::new(); | ||
t.hash(&mut s); | ||
s.finish() | ||
} |
Uh oh!
There was an error while loading. Please reload this page.