Add support for silencing only one of the outputs of a test#1707
Add support for silencing only one of the outputs of a test#1707Kriskras99 wants to merge 1 commit into
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 Report❌ 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. 🚀 New features to boost your workflow:
|
|
Thanks for the PR! I'm currently quite sick but will look when I'm better. |
|
No worries, get well soon! |
sunshowers
left a comment
sunshowers
left a comment
There was a problem hiding this comment.
Yeah I really like the overall direction. Thanks!
| 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.)
|
Hello! Any update on this? |
|
No update unfortunately. It still needs tests added. It also has some merge conflicts that need to be resolved. I won't get to it anytime soon, feel free to take over the PR. |
|
I've began an attempt to resolve the merge conflicts and port this PR to main. I can't figure out how to get a |
|
Thanks @ArhanChaudhary -- would you be willing to submit a PR even with your WIP? Happy to have a look. |
|
Done! |
3 participants
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