Skip to content
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

libtest: Use deterministic HashMap, avoid spawning thread if there is no concurrency #56243

Merged
merged 5 commits into from
Dec 11, 2018

Conversation

RalfJung
Copy link
Member

It seems desirable to make a test and bench runner deterministic, which this achieves by using a deterministic hasher. Also, we we only have 1 thread, we don't bother spawning one and just use the main thread.

The motivation for this is to be able to run the test harness in miri, where we can neither access the OS RNG, nor spawn threads.

@rust-highfive
Copy link
Collaborator

r? @sfackler

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 26, 2018
@Centril
Copy link
Contributor

Centril commented Nov 26, 2018

Hmm... isn't the non-determinism we have now on purpose so that we expose more bugs in the system under test that relies on orderings in ways it should not?

@rust-highfive

This comment has been minimized.

@RalfJung
Copy link
Member Author

Hmm... isn't the non-determinism we have now on purpose so that we expose more bugs in the system under test that relies on orderings in ways it should not?

Really? Is that documented anywhere? It looks like a mere "HashMap is the default" to me.

Also, for benchmarks, you want them as reproducible as possible, so there the non-determinism is certainly not desired.

@Centril
Copy link
Contributor

Centril commented Nov 26, 2018

Really? Is that documented anywhere? It looks like a mere "HashMap is the default" to me.

I thought I read a blog post about this somewhere but I cannot find it right now... There's http://jakegoulding.com/blog/2012/10/18/run-your-tests-in-a-deterministic-random-order/ tho.

@RalfJung
Copy link
Member Author

There's http://jakegoulding.com/blog/2012/10/18/run-your-tests-in-a-deterministic-random-order/ tho.

Interesting. However, libtest did not provide deterministic random order for sure, and also I'd say the problem of global state is much smaller in Rust.

@Centril
Copy link
Contributor

Centril commented Nov 26, 2018

@RalfJung Sure; Having some way for libtest to spit out the seed before it starts testing would be good; you could then reuse that seed for the determinism angle. This is essentially what tools such as QuickCheck in Haskell and proptest do.

In this case, the global mutable state wouldn't be an object in Rust per se, but rather some object such as a file system or database that the test may write and read from; if you then have a deterministic ordering of the tests, that could cause tests to pass when they shouldn't; randomizing the order would in some cases expose the dependencies and cause warranted failures.

@rust-highfive

This comment has been minimized.

@RalfJung
Copy link
Member Author

Having some way for libtest to spit out the seed before it starts testing would be good; you could then reuse that seed for the determinism angle. This is essentially what tools such as QuickCheck in Haskell and proptest do.

Not just spit out, one would also have to be able to set the seed.

However, this entire discussion has nothing to do with the patch.^^ The affected HashMap serves to keep track of which tests are still running, and the only time it is iterated is in get_timed_out_tests to compute which tests timed out.

@Dylan-DPC-zz
Copy link

Ping from triage @sfackler awaiting your review on this.

@RalfJung
Copy link
Member Author

RalfJung commented Dec 8, 2018

@alexcrichton could you have a look, or pick someone who could have a look?

@alexcrichton
Copy link
Member

This looks fine by me to land, thanks @RalfJung! If we're gonna comment /* concurrency= */ in most places, though, perhaps an enum could be used like Concurrent::Yes or Concurrent::No? Other than that r=me!

@Dylan-DPC-zz
Copy link

r? @alexcrichton

@RalfJung
Copy link
Member Author

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Dec 11, 2018

📌 Commit c28c287 has been approved by alexcrichton

@bors 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 Dec 11, 2018
@asomers
Copy link
Contributor

asomers commented Mar 5, 2019

I believe this PR is the cause of #58907 . The switch to running all tests in a single-thread is a user-visible change, and it needs to be mentioned in the Rust release notes.

@RalfJung
Copy link
Member Author

RalfJung commented Mar 5, 2019

This PR didn't introduce a thread pool, so at least that issue description cannot be entirely right. But indeed, it seems like the libtest code used to spawn a new thread per test; now it's all happening in the main thread.

bors added a commit that referenced this pull request Mar 9, 2019
Rollup of 24 pull requests

Successful merges:

 - #58080 (Add FreeBSD armv6 and armv7 targets)
 - #58204 (On return type `impl Trait` for block with no expr point at last semi)
 - #58269 (Add librustc and libsyntax to rust-src distribution.)
 - #58369 (Make the Entry API of HashMap<K, V> Sync and Send)
 - #58861 (Expand where negative supertrait specific error is shown)
 - #58877 (Suggest removal of `&` when borrowing macro and appropriate)
 - #58883 (Suggest appropriate code for unused field when destructuring pattern)
 - #58891 (Remove stray ` in the docs for the FromIterator implementation for Option)
 - #58893 (race condition in thread local storage example)
 - #58906 (Monomorphize generator field types for debuginfo)
 - #58911 (Regression test for #58435.)
 - #58912 (Regression test for #58813)
 - #58916 (Fix release note problems noticed after merging.)
 - #58918 (Regression test added for an async ICE.)
 - #58921 (Add an explicit test for issue #50582)
 - #58926 (Make the lifetime parameters of tcx consistent.)
 - #58931 (Elide invalid method receiver error when it contains TyErr)
 - #58940 (Remove JSBackend from config.toml)
 - #58950 (Add self to mailmap)
 - #58961 (On incorrect cfg literal/identifier, point at the right span)
 - #58963 (libstd: implement Error::source for io::Error)
 - #58970 (delay_span_bug in wfcheck's ty.lift_to_tcx unwrap)
 - #58984 (Teach `-Z treat-err-as-bug` to take a number of errors to emit)
 - #59007 (Add a test for invalid const arguments)

Failed merges:

 - #58959 (Add release notes for PR #56243)

r? @ghost
asomers added a commit to asomers/rust that referenced this pull request Mar 9, 2019
kennytm added a commit to kennytm/rust that referenced this pull request Mar 16, 2019
bors added a commit that referenced this pull request Mar 16, 2019
Rollup of 37 pull requests

Successful merges:

 - #58854 (appveyor: Use VS2017 for all our images)
 - #58855 (std: Spin for a global malloc lock on wasm32)
 - #58873 (Fix "Auto-hide item methods documentation" setting)
 - #58901 (Change `std::fs::copy` to use `copyfile` on MacOS and iOS)
 - #58933 (Move alloc::prelude::* to alloc::prelude::v1, make alloc a subset of std)
 - #58938 (core: ensure VaList passes improper_ctypes lint)
 - #58941 (MIPS: add r6 support)
 - #58949 (SGX target: Expose thread id function in os module)
 - #58959 (Add release notes for PR #56243)
 - #58976 (Default to integrated `rust-lld` linker for UEFI targets)
 - #59009 (Fix SGX implementations of read/write_vectored.)
 - #59025 (Fix generic argument lookup for Self)
 - #59036 (Fix ICE in MIR pretty printing)
 - #59037 (Avoid some common false positives in intra doc link checking)
 - #59072 (we can now skip should_panic tests with the libtest harness)
 - #59079 (add suggestions to invalid macro item error)
 - #59082 (A few improvements to comments in user-facing crates)
 - #59102 (Consistent naming for duration_float methods and additional f32 methods)
 - #59118 (rustc: fix ICE when trait alias has bare Self)
 - #59139 (Unregress using scalar unions in constants.)
 - #59146 (Suggest return lifetime when there's only one named lifetime)
 - #59147 (Make std time tests more robust for platform differences)
 - #59152 (Stabilize Range*::contains.)
 - #59156 ([wg-async-await] Add regression test for #55809.)
 - #59158 (Revert "Don't generate minification variable if minification disabled")
 - #59169 (Add `-Z allow_features=...` flag)
 - #59173 (bootstrap: Default to a sensible llvm-suffix.)
 - #59175 (Don't run test launching `echo` since that doesn't exist on Windows)
 - #59180 (Use try blocks in rustc_codegen_ssa)
 - #59185 (No old chestnuts in iter::repeat docs)
 - #59201 (Remove restriction on isize/usize in repr(simd))
 - #59204 (Output diagnostic information for rustdoc)
 - #59206 (Improved test output)
 - #59208 (Reduce a Code Repetition Related to Bit Operation)
 - #59212 (Add x86_64 musl host to the manifest)
 - #59221 (Option and Result: Add references to documentation of as_ref and as_mut)
 - #59231 (Stabilize Option::copied)
pietroalbini pushed a commit to pietroalbini/rust that referenced this pull request Mar 19, 2019
bors added a commit that referenced this pull request Mar 19, 2019
[beta] Rollup backports

Rolled up:

* [beta] Move to static.r-l.o stable release #58896

Cherry-picked:

* Add release notes for PR #56243 #58959
* resolve: Account for new importable entities #59047
* bootstrap: Default to a sensible llvm-suffix. #59173

r? @ghost
@Centril Centril added this to the 1.33 milestone Apr 26, 2019
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 4, 2022
libtest: run all tests in their own thread, if supported by the host

This reverts the threading changes of rust-lang#56243, which made it so that with `-j1`, the test harness does not spawn any threads. Those changes were done to enable Miri to run the test harness, but Miri supports threads nowadays, so this is no longer needed. Using a thread for each test is useful because the thread's name can be set to the test's name which makes panic messages consistent between `-j1` and `-j2` runs and also a bit more readable.

I did not revert the HashMap changes of rust-lang#56243; using a deterministic map seems fine for the test harness and the more deterministic testing is the better.

Fixes rust-lang#59122
Fixes rust-lang#70492
thomcc pushed a commit to tcdi/postgrestd that referenced this pull request Feb 10, 2023
libtest: run all tests in their own thread, if supported by the host

This reverts the threading changes of rust-lang/rust#56243, which made it so that with `-j1`, the test harness does not spawn any threads. Those changes were done to enable Miri to run the test harness, but Miri supports threads nowadays, so this is no longer needed. Using a thread for each test is useful because the thread's name can be set to the test's name which makes panic messages consistent between `-j1` and `-j2` runs and also a bit more readable.

I did not revert the HashMap changes of rust-lang/rust#56243; using a deterministic map seems fine for the test harness and the more deterministic testing is the better.

Fixes rust-lang/rust#59122
Fixes rust-lang/rust#70492
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants