Add test for impl of available_parallelism()
#104095
Conversation
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
flba-eb
force-pushed
the
test_available_parallelism
branch
from
80222ca
to
61d80c1
Compare
This comment has been minimized.
This comment has been minimized.
flba-eb
force-pushed
the
test_available_parallelism
branch
from
61d80c1
to
8af12eb
Compare
|
Any reason this is a UI test instead of a library test? |
|
Yeah I think this should be moved into a unit or integration test in library/std, if possible? I suppose it might be the case that those tests don't work on tier 3 targets due to the use of |
|
Being able to run in on a tier 3 target would be important -- purpose of the test is to find a missing implementation of a new target OS. If required I can move the test, but it would be more helpful as part of the UI tests. |
|
Yeah, we should really fix the issue that the stdlib's tests can't run on most tier3 targets because of our @bors r+ |
bors
added
S-waiting-on-bors
…, r=thomcc Add test for impl of `available_parallelism()` This checks that `std::thread::available_parallelism()` returns `Ok(NonZeroUsize)`. With this test maintainers of targets(OS) can check if they have implemented this function (or decide to ignore it). Not sure how to deal with the ignore list right now: - tier 1's should all implement the feature already ✔️ - tier 2's mostly do as well, but not e.g. `ios`, `wasi` -- should we add them to the ignore list or let the maintainers do that (so that they are informed)? - tier 3's will certainly sometimes not implement it, my suggestion is to not add them to the ignore list - I've added only `vxworks`, `redox` and `l4re` to the ignore list because this is documented already in `library/std/src/sys/unix/thread.rs` ([code](https://github.com/rust-lang/rust/blob/9b735a7132acd58b3bd34c084e9ca5b4ca7450a2/library/std/src/sys/unix/thread.rs#L403)) We could also check the value against e.g. `cat /proc/cpuinfo | grep process or | wc` but I don't know if this is required (how much sense it makes) and if this would play well with e.g. cgroups on Linux. r? `@thomcc`
bors
added
S-waiting-on-author
Is there some place that we are tracking moving std tests from ui to unit tests, that this could be added to? I'm a big fan of that btw, since it is much easier to run unit tests in Miri than ui tests. |
Part of my motivation for updating rand is being able to run libstd unit tests on tier3 targets. Right now even if those targets have std, we can't run the stdlib tests against them unless they also have |
|
Yeah I got that. But what I was asking is, is there a tracking issue for "move library tests from ui to unit"? Looks like we should list PRs like this there so once the getrandom issue is resolved, we don't forget to move the tests. |
|
Good idea. I don't think so. I'll try to make one tomorrow or so (it's a bit late here right now and I don't feel up to digging through our existing uitests to make a list). |
|
Created #104676 (only saw your edit after opening the issue... oops) |
Thank you for making it and saving me the time. |
…, r=Mark-Simulacrum Update `rand` in the stdlib tests, and remove the `getrandom` feature from it. The main goal is actually removing `getrandom`, so that eventually we can allow running the stdlib test suite on tier3 targets which don't have `getrandom` support. Currently those targets can only run the subset of stdlib tests that exist in uitests, and (generally speaking), we prefer not to test libstd functionality in uitests, which came up recently in rust-lang#104095 and rust-lang#104185. Additionally, the fact that we can't update `rand`/`getrandom` means we're stuck with the old set of tier3 targets, so can't test new ones. ~~Anyway, I haven't checked that this actually does allow use on tier3 targets (I think it does not, as some work is needed in stdlib submodules) but it moves us slightly closer to this, and seems to allow at least finally updating our `rand` dep, which definitely improves the status quo.~~ Checked and works now. For the most part, our tests and benchmarks are fine using hard-coded seeds. A couple tests seem to fail with this (stuff manipulating the environment expecting no collisions, for example), or become pointless (all inputs to a function become equivalent). In these cases I've done a (gross) dance (ab)using `RandomState` and `Location::caller()` for some extra "entropy". Trying to share that code seems *way* more painful than it's worth given that the duplication is a 7-line function, even if the lines are quite gross. (Keeping in mind that sharing it would require adding `rand` as a non-dev dep to std, and exposing a type from it publicly, all of which sounds truly awful, even if done behind a perma-unstable feature). See also some previous attempts: - rust-lang#86963 (in particular rust-lang#86963 (comment) which explains why this is non-trivial) - rust-lang#89131 - rust-lang#96626 (comment) (I tried in that PR at the same time, but settled for just removing the usage of `thread_rng()` from the benchmarks, since that was the main goal). - rust-lang#104185 - Probably more. It's very tempting of a thing to "just update". r? `@Mark-Simulacrum`
|
ping from triage - can you post your status on this PR? There hasn't been an update in a few months. Thanks! FYI: when a PR is ready for review, send a message containing |
|
Closing this inactive |
Dylan-DPC
added
S-inactive
…Simulacrum Update `rand` in the stdlib tests, and remove the `getrandom` feature from it. The main goal is actually removing `getrandom`, so that eventually we can allow running the stdlib test suite on tier3 targets which don't have `getrandom` support. Currently those targets can only run the subset of stdlib tests that exist in uitests, and (generally speaking), we prefer not to test libstd functionality in uitests, which came up recently in rust-lang/rust#104095 and rust-lang/rust#104185. Additionally, the fact that we can't update `rand`/`getrandom` means we're stuck with the old set of tier3 targets, so can't test new ones. ~~Anyway, I haven't checked that this actually does allow use on tier3 targets (I think it does not, as some work is needed in stdlib submodules) but it moves us slightly closer to this, and seems to allow at least finally updating our `rand` dep, which definitely improves the status quo.~~ Checked and works now. For the most part, our tests and benchmarks are fine using hard-coded seeds. A couple tests seem to fail with this (stuff manipulating the environment expecting no collisions, for example), or become pointless (all inputs to a function become equivalent). In these cases I've done a (gross) dance (ab)using `RandomState` and `Location::caller()` for some extra "entropy". Trying to share that code seems *way* more painful than it's worth given that the duplication is a 7-line function, even if the lines are quite gross. (Keeping in mind that sharing it would require adding `rand` as a non-dev dep to std, and exposing a type from it publicly, all of which sounds truly awful, even if done behind a perma-unstable feature). See also some previous attempts: - rust-lang/rust#86963 (in particular rust-lang/rust#86963 (comment) which explains why this is non-trivial) - rust-lang/rust#89131 - rust-lang/rust#96626 (comment) (I tried in that PR at the same time, but settled for just removing the usage of `thread_rng()` from the benchmarks, since that was the main goal). - rust-lang/rust#104185 - Probably more. It's very tempting of a thing to "just update". r? `@Mark-Simulacrum`
This is a redo of (this PR)[rust-lang#104095]. Add test for available_parallelism
This is a redo of (this PR)[rust-lang#104095]. Add test for available_parallelism Add test for available_parallelism
This checks that
std::thread::available_parallelism()returnsOk(NonZeroUsize). With this test maintainers of targets(OS) can check if they have implemented this function (or decide to ignore it).Not sure how to deal with the ignore list right now:
ios,wasi-- should we add them to the ignore list or let the maintainers do that (so that they are informed)?vxworks,redoxandl4reto the ignore list because this is documented already inlibrary/std/src/sys/unix/thread.rs(code)We could also check the value against e.g.
cat /proc/cpuinfo | grep process or | wcbut I don't know if this is required (how much sense it makes) and if this would play well with e.g. cgroups on Linux.r? @thomcc