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: also measure time in Miri #123548

Merged
merged 1 commit into from
Apr 15, 2024
Merged

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Apr 6, 2024

A long time ago we disabled timekeeping of the default test harness in Miri, as otherwise it would fail to run without -Zmiri-disable-isolation. However, since then Miri gained a "fake clock" that lets it present some deterministic notion of time when isolation is enabled.

So we could now let libtest do timekeeping again when running in Miri. That's nice as it can help detect tests that run too long. However it can also be confusing as the results with isolation can be quite different than the real time.

@rust-lang/miri what do you think?

@rustbot
Copy link
Collaborator

rustbot commented Apr 6, 2024

r? @joboet

rustbot has assigned @joboet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 6, 2024
Copy link
Contributor

@joboet joboet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes itself look good to me, but I can't speak for the miri side of things.

r=me once you reach consensus (and after deleting the outdated comment)

library/test/src/console.rs Outdated Show resolved Hide resolved
@joboet joboet added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). A-miri Area: The miri tool and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 15, 2024
@RalfJung
Copy link
Member Author

@oli-obk @saethlin what do you think?

@oli-obk
Copy link
Contributor

oli-obk commented Apr 15, 2024

@bors r=joboet

@bors
Copy link
Contributor

bors commented Apr 15, 2024

📌 Commit 510720e has been approved by joboet

It is now in the queue for this repository.

@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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Apr 15, 2024
@saethlin
Copy link
Member

🤷 This seems... interesting I suppose. We'll know how confusing the output is once it's landed.

jieyouxu added a commit to jieyouxu/rust that referenced this pull request Apr 15, 2024
libtest: also measure time in Miri

A long time ago we disabled timekeeping of the default test harness in Miri, as otherwise it would fail to run without `-Zmiri-disable-isolation`. However, since then Miri gained a "fake clock" that lets it present some deterministic notion of time when isolation is enabled.

So we could now let libtest do timekeeping again when running in Miri. That's nice as it can help detect tests that run too long. However it can also be confusing as the results with isolation can be quite different than the real time.

`@rust-lang/miri` what do you think?
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 15, 2024
Rollup of 12 pull requests

Successful merges:

 - rust-lang#123423 (Distribute LLVM bitcode linker as a preview component)
 - rust-lang#123548 (libtest: also measure time in Miri)
 - rust-lang#123666 (Fix some typos in doc)
 - rust-lang#123864 (Remove a HACK by instead inferring opaque types during expected/formal type checking)
 - rust-lang#123896 (Migrate some diagnostics in `rustc_resolve` to session diagnostic)
 - rust-lang#123919 (builtin-derive: tag → discriminant)
 - rust-lang#123922 (Remove magic constants when using `base_n`.)
 - rust-lang#123931 (Don't leak unnameable types in `-> _` recover)
 - rust-lang#123933 (move the LargeAssignments lint logic into its own file)
 - rust-lang#123934 (`rustc_data_structures::graph` mini refactor)
 - rust-lang#123941 (Fix UB in LLVM FFI when passing zero or >1 bundle)
 - rust-lang#123957 (disable create_dir_all_bare test on all(miri, windows))

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2074631 into rust-lang:master Apr 15, 2024
11 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 15, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 15, 2024
Rollup merge of rust-lang#123548 - RalfJung:what-is-time, r=joboet

libtest: also measure time in Miri

A long time ago we disabled timekeeping of the default test harness in Miri, as otherwise it would fail to run without `-Zmiri-disable-isolation`. However, since then Miri gained a "fake clock" that lets it present some deterministic notion of time when isolation is enabled.

So we could now let libtest do timekeeping again when running in Miri. That's nice as it can help detect tests that run too long. However it can also be confusing as the results with isolation can be quite different than the real time.

``@rust-lang/miri`` what do you think?
@RalfJung RalfJung deleted the what-is-time branch April 16, 2024 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-miri Area: The miri tool S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants