rust
rust copied to clipboard
rust-lang
Make libtest logfile respect --format
I recently ran into an issue caused by the libtest output in logfile not respecting the --format flag. I've seen that a bug has been filed (#57147) but no PR was ever sent.
This PR makes the logfile follow the same format used for STDOUT printing. It removes the log_out field from ConsoleTestState and inlines its creation where needed, wrapping the logfile in an OutputFormatter. I've also deleted a couple of (now unused) methods in ConsoleTestState.
Let me know if you think there is a cleaner solution than the direction I've followed. Thanks!
closes #57147
Hey! It looks like you've submitted a new PR for the library teams!
If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.
Examples of T-libs-api changes:
- Stabilizing library features
- Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
- Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
- Changing public documentation in ways that create new stability guarantees
- Changing observable runtime behavior of library APIs
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @yaahc (or someone else) soon.
Please see the contribution instructions for more information.
The job mingw-check failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)
Checking proc_macro v0.0.0 (/checkout/library/proc_macro)
Checking unicode-width v0.1.8
Checking getopts v0.2.21
Checking test v0.0.0 (/checkout/library/test)
error: unused imports: `TestExecTime`, `bench::fmt_bench_samples`
|
|
9 | bench::fmt_bench_samples,
...
...
18 | time::{TestExecTime, TestSuiteExecTime},
|
|
= note: `-D unused-imports` implied by `-D warnings`
error[E0277]: the `?` operator can only be used in a closure that returns `Result` or `Option` (or another type that implements `FromResidual`)
|
|
91 | let mut log_out = opts.logfile.map(|path| OutputLocation::Raw(File::create(path)?));
| ---------------------------------------------^-
| | |
| | cannot use the `?` operator in a closure that returns `OutputLocation<File>`
| this function should return `Result` or `Option` to accept `?`
|
= help: the trait `FromResidual<Result<Infallible, std::io::Error>>` is not implemented for `OutputLocation<File>`
error[E0277]: the `?` operator can only be used in a closure that returns `Result` or `Option` (or another type that implements `FromResidual`)
|
|
243 | let mut log_out: Option<Box<dyn OutputFormatter>> = opts.logfile.map(|path| {
244 | | createFormatter(
244 | | createFormatter(
245 | | OutputLocation::Raw(File::create(path)?),
| | ^ cannot use the `?` operator in a closure that returns `Box<dyn OutputFormatter>`
246 | | opts,
249 | | )
250 | | });
250 | | });
| |_____- this function should return `Result` or `Option` to accept `?`
|
= help: the trait `FromResidual<Result<Infallible, std::io::Error>>` is not implemented for `Box<dyn OutputFormatter>`
error[E0308]: `match` arms have incompatible types
--> library/test/src/console.rs:277:29
|
|
275 | let log_out_result = match log_out {
276 | | None => Ok(()),
| | ------ this is found to be of type `Result<(), _>`
| | ------ this is found to be of type `Result<(), _>`
277 | | Some(log_output) => log_output.write_run_finish(&st),
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `()`, found `bool`
| |_____- `match` arms have incompatible types
|
= note: expected type `Result<(), _>`
found enum `Result<bool, std::io::Error>`
The job mingw-check failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)
| ----^^^^^^^
| |
| help: remove this `mut`
|
= note: `-D unused-mut` implied by `-D warnings`
error[E0507]: cannot move out of `opts.logfile.0` which is behind a shared reference
|
90 | let mut log_out = match opts.logfile {
| ^^^^^^^^^^^^ help: consider borrowing here: `&opts.logfile`
91 | None => None,
91 | None => None,
92 | Some(path) => Some(OutputLocation::Raw(File::create(path)?)),
| |
| data moved here
| data moved here
| move occurs because `path` has type `PathBuf`, which does not implement the `Copy` trait
error[E0382]: use of moved value
--> library/test/src/console.rs:115:21
|
|
115 | if let Some(log_output) = log_out {
| ^^^^^^^^^^ value moved here, in previous iteration of loop
|
= note: move occurs because value has type `OutputLocation<File>`, which does not implement the `Copy` trait
help: borrow this field in the pattern to avoid moving `log_out.0`
|
115 | if let Some(ref log_output) = log_out {
error[E0382]: use of moved value
--> library/test/src/console.rs:136:21
|
|
130 | if let Some(log_output) = log_out {
| ---------- value moved here
...
136 | if let Some(log_output) = log_out {
| ^^^^^^^^^^ value used here after move
|
= note: move occurs because value has type `OutputLocation<File>`, which does not implement the `Copy` trait
help: borrow this field in the pattern to avoid moving `log_out.0`
|
130 | if let Some(ref log_output) = log_out {
error[E0596]: cannot borrow `log_output` as mutable, as it is not declared as mutable
|
498 | / macro_rules! write {
498 | / macro_rules! write {
499 | | ($dst:expr, $($arg:tt)*) => {
500 | | $dst.write_fmt($crate::format_args!($($arg)*))
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot borrow as mutable
502 | | }
| |_- in this expansion of `$crate::write!` (#2)
...
552 | / macro_rules! writeln {
552 | / macro_rules! writeln {
553 | | ($dst:expr $(,)?) => {
554 | | $crate::write!($dst, "\n")
555 | | };
... |
558 | | };
559 | | }
559 | | }
| |_- in this expansion of `writeln!` (#1)
|
::: library/test/src/console.rs:130:25
|
130 | if let Some(log_output) = log_out {
| ---------- help: consider changing this to be mutable: `mut log_output`
131 | writeln!(log_output)?;
error[E0596]: cannot borrow `log_output` as mutable, as it is not declared as mutable
|
552 | / macro_rules! writeln {
552 | / macro_rules! writeln {
553 | | ($dst:expr $(,)?) => {
554 | | $crate::write!($dst, "\n")
555 | | };
556 | | ($dst:expr, $($arg:tt)*) => {
557 | | $dst.write_fmt($crate::format_args_nl!($($arg)*))
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot borrow as mutable
559 | | }
| |_- in this expansion of `writeln!`
|
::: library/test/src/console.rs:115:21
::: library/test/src/console.rs:115:21
|
115 | if let Some(log_output) = log_out {
| ---------- help: consider changing this to be mutable: `mut log_output`
116 | writeln!(log_output, "{name}: {fntype}")?;
error[E0596]: cannot borrow `log_output` as mutable, as it is not declared as mutable
|
552 | / macro_rules! writeln {
552 | / macro_rules! writeln {
553 | | ($dst:expr $(,)?) => {
554 | | $crate::write!($dst, "\n")
555 | | };
556 | | ($dst:expr, $($arg:tt)*) => {
557 | | $dst.write_fmt($crate::format_args_nl!($($arg)*))
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot borrow as mutable
559 | | }
| |_- in this expansion of `writeln!`
|
::: library/test/src/console.rs:136:21
::: library/test/src/console.rs:136:21
|
136 | if let Some(log_output) = log_out {
| ---------- help: consider changing this to be mutable: `mut log_output`
137 | writeln!(log_output, "{}, {}", plural(ntest, "test"), plural(nbench, "benchmark"))?;
error[E0507]: cannot move out of `log_out.0`, as `log_out` is a captured variable in an `FnMut` closure
|
|
245 | let mut log_out: Option<Box<dyn OutputFormatter>> = match opts.logfile {
| ----------- captured outer variable
...
262 | run_tests(opts, tests, |x| {
263 | | on_test_event(
264 | | &x,
265 | | &mut st,
266 | | &mut *out,
266 | | &mut *out,
267 | | match log_out {
| | ^^^^^^^ help: consider borrowing here: `&log_out`
268 | | None => None,
269 | | Some(log_output) => Some(&mut *log_output),
| | |
| | data moved here
| | data moved here
| | move occurs because `log_output` has type `Box<dyn OutputFormatter>`, which does not implement the `Copy` trait
271 | | )
272 | | })?;
272 | | })?;
| |_____- captured by this `FnMut` closure
error[E0596]: cannot borrow `*log_output` as mutable, as `log_output` is not declared as mutable
|
|
269 | Some(log_output) => Some(&mut *log_output),
| ---------- ^^^^^^^^^^^^^^^^ cannot borrow as mutable
| |
| help: consider changing this to be mutable: `mut log_output`
error[E0597]: `*log_output` does not live long enough
|
263 | on_test_event(
| ------------- borrow later used by call
...
...
269 | Some(log_output) => Some(&mut *log_output),
| ^^^^^^^^^^^^^^^^- `*log_output` dropped here while still borrowed
| borrowed value does not live long enough
error: variable does not need to be mutable
--> library/test/src/console.rs:245:9
--> library/test/src/console.rs:245:9
|
245 | let mut log_out: Option<Box<dyn OutputFormatter>> = match opts.logfile {
| |
| help: remove this `mut`
error[E0507]: cannot move out of `opts.logfile.0` which is behind a shared reference
|
|
245 | let mut log_out: Option<Box<dyn OutputFormatter>> = match opts.logfile {
| ^^^^^^^^^^^^ help: consider borrowing here: `&opts.logfile`
246 | None => None,
247 | Some(path) => Some(createFormatter(
| |
| data moved here
| data moved here
| move occurs because `path` has type `PathBuf`, which does not implement the `Copy` trait
error[E0382]: use of moved value
--> library/test/src/console.rs:280:14
|
|
245 | let mut log_out: Option<Box<dyn OutputFormatter>> = match opts.logfile {
| ----------- move occurs because `log_out` has type `Option<Box<dyn OutputFormatter>>`, which does not implement the `Copy` trait
...
262 | run_tests(opts, tests, |x| {
| --- value moved into closure here
267 | match log_out {
| ------- variable moved due to use in closure
...
...
280 | Some(log_output) => log_output.write_run_finish(&st),
| ^^^^^^^^^^ value used here after move
error[E0596]: cannot borrow `*log_output` as mutable, as `log_output` is not declared as mutable
|
|
280 | Some(log_output) => log_output.write_run_finish(&st),
| ---------- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot borrow as mutable
| |
| help: consider changing this to be mutable: `mut log_output`
error[E0310]: the parameter type `T` may not live long enough
--> library/test/src/console.rs:292:33
|
|
292 | OutputFormat::Pretty => Box::new(PrettyFormatter::new(
293 | | output,
293 | | output,
294 | | opts.use_color(),
295 | | max_name_len,
296 | | is_multithreaded,
297 | | opts.time_options,
298 | | )),
|
|
= help: consider adding an explicit lifetime bound `T: 'static`...
error[E0310]: the parameter type `T` may not live long enough
--> library/test/src/console.rs:300:13
|
|
300 | Box::new(TerseFormatter::new(output, opts.use_color(), max_name_len, is_multithreaded))
|
|
= help: consider adding an explicit lifetime bound `T: 'static`...
error[E0310]: the parameter type `T` may not live long enough
--> library/test/src/console.rs:302:31
|
|
302 | OutputFormat::Json => Box::new(JsonFormatter::new(output)),
|
|
= help: consider adding an explicit lifetime bound `T: 'static`...
error[E0310]: the parameter type `T` may not live long enough
--> library/test/src/console.rs:303:32
|
|
303 | OutputFormat::Junit => Box::new(JunitFormatter::new(output)),
|
|
= help: consider adding an explicit lifetime bound `T: 'static`...
Some errors have detailed explanations: E0310, E0382, E0507, E0596, E0597.
For more information about an error, try `rustc --explain E0310`.
error: could not compile `test` due to 18 previous errors
Build completed unsuccessfully in 0:01:30
The job mingw-check failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)
Checking proc_macro v0.0.0 (/checkout/library/proc_macro)
Checking unicode-width v0.1.8
Checking getopts v0.2.21
Checking test v0.0.0 (/checkout/library/test)
error[E0277]: the trait bound `Box<dyn OutputFormatter>: OutputFormatter` is not satisfied
|
|
269 | Some(ref mut log_output) => Some(&mut *log_output),
| ---- ^^^^^^^^^^^^^^^^ the trait `OutputFormatter` is not implemented for `Box<dyn OutputFormatter>`
| required by a bound introduced by this call
|
= note: required for the cast to the object type `dyn OutputFormatter`
The job mingw-check failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)
Checking test v0.0.0 (/checkout/library/test)
error[E0310]: the parameter type `T` may not live long enough
--> library/test/src/console.rs:292:33
|
292 | OutputFormat::Pretty => Box::new(PrettyFormatter::new(
293 | | output,
293 | | output,
294 | | opts.use_color(),
295 | | max_name_len,
296 | | is_multithreaded,
297 | | opts.time_options,
298 | | )),
|
|
= help: consider adding an explicit lifetime bound `T: 'static`...
error[E0310]: the parameter type `T` may not live long enough
--> library/test/src/console.rs:300:13
|
|
300 | Box::new(TerseFormatter::new(output, opts.use_color(), max_name_len, is_multithreaded))
|
|
= help: consider adding an explicit lifetime bound `T: 'static`...
error[E0310]: the parameter type `T` may not live long enough
--> library/test/src/console.rs:302:31
|
|
302 | OutputFormat::Json => Box::new(JsonFormatter::new(output)),
|
|
= help: consider adding an explicit lifetime bound `T: 'static`...
error[E0310]: the parameter type `T` may not live long enough
--> library/test/src/console.rs:303:32
|
|
303 | OutputFormat::Junit => Box::new(JunitFormatter::new(output)),
|
|
= help: consider adding an explicit lifetime bound `T: 'static`...
For more information about this error, try `rustc --explain E0310`.
error: could not compile `test` due to 4 previous errors
Build completed unsuccessfully in 0:01:28
The job mingw-check failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)
Checking test v0.0.0 (/checkout/library/test)
error[E0310]: the parameter type `T` may not live long enough
--> library/test/src/console.rs:293:33
|
293 | OutputFormat::Pretty => Box::new(PrettyFormatter::new(
294 | | output,
294 | | output,
295 | | opts.use_color(),
296 | | max_name_len,
297 | | is_multithreaded,
298 | | opts.time_options,
299 | | )),
|
|
= help: consider adding an explicit lifetime bound `T: 'static`...
error[E0310]: the parameter type `T` may not live long enough
--> library/test/src/console.rs:301:13
|
|
301 | Box::new(TerseFormatter::new(output, opts.use_color(), max_name_len, is_multithreaded))
|
|
= help: consider adding an explicit lifetime bound `T: 'static`...
error[E0310]: the parameter type `T` may not live long enough
--> library/test/src/console.rs:303:31
|
|
303 | OutputFormat::Json => Box::new(JsonFormatter::new(output)),
|
|
= help: consider adding an explicit lifetime bound `T: 'static`...
error[E0310]: the parameter type `T` may not live long enough
--> library/test/src/console.rs:304:32
|
|
304 | OutputFormat::Junit => Box::new(JunitFormatter::new(output)),
|
|
= help: consider adding an explicit lifetime bound `T: 'static`...
For more information about this error, try `rustc --explain E0310`.
error: could not compile `test` due to 4 previous errors
Build completed unsuccessfully in 0:01:26
The job mingw-check failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)
Checking test v0.0.0 (/checkout/library/test)
Checking rand_core v0.5.1
Checking rand_chacha v0.2.2
Checking rand_xorshift v0.2.0
error[E0560]: struct `ConsoleTestState` has no field named `log_out`
|
799 | log_out: None,
| ^^^^^^^ `ConsoleTestState` does not have this field
|
|
= note: available fields are: `total`, `passed`, `failed`, `ignored`, `filtered_out` ... and 7 others
For more information about this error, try `rustc --explain E0560`.
error: could not compile `test` due to previous error
warning: build failed, waiting for other jobs to finish...
Build completed unsuccessfully in 0:01:42
Hi Jane,
Did I forget to do something to mark this PR as ready for review? Since this is my first PR to the Rust codebase I don't know if I missed some steps. Let me know if that's the case :)
Thanks!
Hi Jane,
Did I forget to do something to mark this PR as ready for review? Since this is my first PR to the Rust codebase I don't know if I missed some steps. Let me know if that's the case :)
Thanks!
No, sorry, you didn't do anything wrong. I've just behind on my reviews and actually planning on taking a break from the review rotation. Sorry for the delay, I'll reassign the PR.
r? rust-lang/libs
I think this counts as a behavior change, and thus falls under t-libs-api rather than t-libs. So I'm going to reassign. This may be an overabundance of caution, though.
r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs
Hey, is the process for a T-libs-api contribution the same as a T-libs one or do I need to do something else before this PR can be reviewed? :)
Thanks!
Changes to the flags of test are a public API change, so require an FCP. Can you summarize the publicly visible change, maybe with a clear "before vs after" showcase?
Can we make this change without breaking any users?
Hi Mara,
Thanks for the reply!
The behavior only changes when a user runs a test or benchmark and sets both the --format and the --logfile flags. Currently the content printed to logfile is just a list of test case names with the strings "ok" or "failed" printed before them (depending on each test case result); while the content printed to STDOUT is always in the format given by the --format flag. This PR makes the content printed to the logfile also be in the format specified in the --format flag, just like STDOUT.
Example:
Create a new Rust project with a single file ./src/lib.rs with the following content:
#[test]
fn it_works() {
let result = 2 + 2;
assert_eq!(result, 4);
}
#[test]
fn it_does_not_work() {
let result = 2 + 2;
assert_eq!(result, 5);
}
Now run the following command:
cargo +nightly test -- -Z unstable-options --format=json --logfile=logfile.txt 1>stdout.txt
Current behavior:
logfile.txt contains:
ok it_works
failed it_does_not_work
stdout.txt contains:
{ "type": "suite", "event": "started", "test_count": 2 }
{ "type": "test", "event": "started", "name": "it_does_not_work" }
{ "type": "test", "event": "started", "name": "it_works" }
{ "type": "test", "name": "it_works", "event": "ok" }
{ "type": "test", "name": "it_does_not_work", "event": "failed", "stdout": "thread 'it_does_not_work' panicked at 'assertion failed: `(left == right)`\n left: `4`,\n right: `5`', src/lib.rs:10:3\nnote: run with `RUST_BACKTRACE=1` environment variable to display a backtrace\n" }
{ "type": "suite", "event": "failed", "passed": 1, "failed": 1, "ignored": 0, "measured": 0, "filtered_out": 0, "exec_time": 0.000431658 }
New behavior:
Both stdout.txt and logfile.txt contain:
{ "type": "suite", "event": "started", "test_count": 2 }
{ "type": "test", "event": "started", "name": "it_does_not_work" }
{ "type": "test", "event": "started", "name": "it_works" }
{ "type": "test", "name": "it_works", "event": "ok" }
{ "type": "test", "name": "it_does_not_work", "event": "failed", "stdout": "thread 'it_does_not_work' panicked at 'assertion failed: `(left == right)`\n left: `4`,\n right: `5`', src/lib.rs:10:3\nnote: run with `RUST_BACKTRACE=1` environment variable to display a backtrace\n" }
{ "type": "suite", "event": "failed", "passed": 1, "failed": 1, "ignored": 0, "measured": 0, "filtered_out": 0, "exec_time": 0.000431658 }
Re: Can we make this change without breaking any users?
Currently this change breaks users who rely on the current logfile output format.
I've thought about protecting this change behind a flag, thus not breaking users who rely on the current logfile behavior. The problem is that in this PR I've restructured the code to reuse the stdout-writing logic for the logfile writing as well, but the old logfile output didn't follow any specific format (as per the --format flag). Therefore I don't know if it's possible to restructure the code in a way that both the old and new behavior co-exist (at least I don't easily see how to do it). I could spend a few hours thinking hard about this if you'd like me to try. Just let me know how essential you think this is.
Please let me know how to proceed from here, especially since I don't know how the FCP process works :)
Thanks!
@m-ou-se - triage: can you help with this?
Please let me know how to proceed from here, especially since I don't know how the FCP process works :)
:umbrella: The latest upstream changes (presumably #103755) made this pull request unmergeable. Please resolve the merge conflicts.
Hey! It looks like you've submitted a new PR for the library teams!
If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.
Examples of T-libs-api changes:
- Stabilizing library features
- Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
- Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
- Changing public documentation in ways that create new stability guarantees
- Changing observable runtime behavior of library APIs
cc @calebcartwright since you were looking in to doing more libtest stuff and may be able to review this
Friendly ping @m-ou-se or @calebcartwright, it would be great to get this merged, thank you!
Speaking for myself, with my testing-devex hat on, I was hoping to hold off on changes like this until we laid out a vision document so we can see what aligns with that vision.
For instance, in my own personal view of how all of this would go, we'd be shifting a lot of these responsibilities from test harnesses (e.g. libtest) to test reporters (e.g. cargo-test, cargo-nextest, cargo-criterion) and de-emphasizing, maybe even deprecating, reporter related flags (obviously not removing them as that would be a breaking change).
@epage Hm. I think that's fair, but this is effectively fixing a bug in existing functionality, and the requisite work to make it possible to shift focus over hasn't happened yet. This PR has been open for more than a year, I really don't think it should wait on all that happening.
If people have determined this to be a bug and want to move forward with merging it, go for it. As for testing-devex, we've not done any prerequisite work to make any determinations like that on our side.
cc @felipeamp from triage: I am marking this PR as waiting-on-author because merge conflicts. @rustbot author
I'm a bit busy in the next couple of weeks (going on vacation and all), but will fix the merge conflicts around Easter.
@felipeamp triage - pinging again, to be sure you haven't forgotten this 🙂
Hi John,
I've taken a look and the merge conflict (and moving this PR forward in general) requires a bit more work than I have the capacity at the moment. Luckily aspotashev has taken over the work in https://github.com/rust-lang/rust/pull/123365, which is current under review by T-testing-devex folks.
Thanks