Add support for silencing only one of the outputs of a test #1707
Conversation
The old syntax is still supported, the new complete syntax is:
- "{value}": where value is one of [immediate, immediate-final, final, never]
- "{stream}={value}": where {stream} is one of [stdout, stderr]
- "{stream}={value},{stream=value}": where the two {stream} keys cannot be the same
For tests where the output is combined (currently only used for
libtest-json output), only the stdout value is used.
There is a small regression where the CLI won't suggest values that
are close to the incorrect input, i.e. if you input 'imediate' it won't
suggest 'immediate'.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1707 +/- ##
==========================================
- Coverage 79.23% 79.03% -0.20%
==========================================
Files 79 79
Lines 19798 19982 +184
==========================================
+ Hits 15686 15792 +106
- Misses 4112 4190 +78 ☔ View full report in Codecov by Sentry. |
|
Thanks for the PR! I'm currently quite sick but will look when I'm better. |
|
No worries, get well soon! |
| match (left, right) { | ||
| (Some(("stderr", Ok(stderr))), Some(("stdout", Ok(stdout)))) => (Some(stdout), Some(stderr)), | ||
| (Some(("stdout", Ok(stdout))), Some(("stderr", Ok(stderr)))) => (Some(stdout), Some(stderr)), | ||
| (Some((stream @ "stdout" | stream @ "stderr", Err(_))), _) => return Err(format!("\n unrecognized setting for {stream}: [possible values: immediate, immediate-final, final, never]")), | ||
| (_, Some((stream @ "stdout" | stream @ "stderr", Err(_)))) => return Err(format!("\n unrecognized setting for {stream}: [possible values: immediate, immediate-final, final, never]")), | ||
| (Some(("stdout", _)), Some(("stdout", _))) => return Err("\n stdout specified twice".to_string()), | ||
| (Some(("stderr", _)), Some(("stderr", _))) => return Err("\n stderr specified twice".to_string()), | ||
| (Some((stream, _)), Some(("stdout" | "stderr", _))) => return Err(format!("\n unrecognized output stream '{stream}': [possible values: stdout, stderr]")), | ||
| (Some(("stdout" | "stderr", _)), Some((stream, _))) => return Err(format!("\n unrecognized output stream '{stream}': [possible values: stdout, stderr]")), | ||
| (_, _) => return Err("\n [possible values: immediate, immediate-final, final, never], or specify one or both output streams: stdout={}, stderr={}, stdout={},stderr={}".to_string()), | ||
| } |
There was a problem hiding this comment.
Hmm, can this be shortened? This feels excessive.
I'd probably do something like:
- split with
,if possible, getting 1 or 2 entries from this (more than 2 should error out) - if one entry, try parsing via
<stream>=<setting>or then just<setting> - if two entries, always try parsing as
<stream>=<setting>, then at the end ensure that the streams are different
There was a problem hiding this comment.
It can be reduced to the two first lines and the last line, but that would result in significantly less helpful error messages.
I also don't see a way to reduce it without degrading the error messaging.
I could do a priority inversion by first checking if the value is a valid TestOutputDisplayOpt, which should be the most common case. And only then dealing with commas and equal signs.
There was a problem hiding this comment.
Yeah, trying to parse it first that way makes sense.
|
|
||
| /// Which output streams should be output immediately | ||
| /// | ||
| /// # Returns |
There was a problem hiding this comment.
We generally don't use a # Returns header here.
Also I'd rather this return a DisplayOutput rather than Option<DisplayOutput>, to avoid representing the invalid state where DisplayOutput is set to false for both.
| serde::de::Unexpected::Str(value), | ||
| &"'never', 'immediate', 'immediate-final', or 'final'", | ||
| ) | ||
| })?); |
There was a problem hiding this comment.
As the warnings suggest, for a manual deserialize impl it would be good to have tests for all the error cases. (I know you haven't written tests yet, just wanted to mention this.)
The old syntax is still supported, the new complete syntax is:
For tests where the output is combined (currently only used for libtest-json output), only the stdout value is used.
There is a small regression where the CLI won't suggest values that are close to the incorrect input, i.e. if you input 'imediate' it won't suggest 'immediate'.
I've not added tests yet, I first wanted to make sure that the implementation is acceptable for inclusion into nextest.
Closes #1688