-
-
Notifications
You must be signed in to change notification settings - Fork 120
Add support for silencing only one of the outputs of a test (cont.) #2517
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
base: main
Are you sure you want to change the base?
Add support for silencing only one of the outputs of a test (cont.) #2517
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #2517 +/- ##
==========================================
- Coverage 79.42% 0.00% -79.43%
==========================================
Files 107 12 -95
Lines 23971 2286 -21685
==========================================
- Hits 19038 0 -19038
+ Misses 4933 2286 -2647 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I apologize but there are now merge conflicts again due to recent changes. Would you be willing to resolve them? I'll try and hold off on other changes before getting this done. |
nextest-runner/src/reporter/mod.rs
Outdated
pub mod displayer; | ||
mod error_description; | ||
pub mod events; | ||
mod helpers; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than making displayer
pub
, could you export the necessary types in the pub use displayer::
below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Sure! Let me do that right now |
013807c
to
d46ed4e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your patience (have been extremely busy recently). This is in pretty good shape -- it mostly needs tests and some code updates.
(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]")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should return a structured error (TestOutputDisplayParseError
or similar) and the formatting should be handled in the Display
impl for the error.
// expected input has three forms | ||
// - "{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 | ||
let (stdout, stderr) = if let Some((left, right)) = s.split_once(',') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's got to be a much better way to write this. One way would be to write a small left-to-right parser using winnow for this, reading characters until you reach a comma or equals sign. Another would be to first decompose the string into components, and then match on them.
This also needs tests.
/// When to display test output in the reporter. | ||
#[derive(Copy, Clone, Debug, Default, Eq, PartialEq)] | ||
#[cfg_attr(test, derive(test_strategy::Arbitrary))] | ||
pub struct TestOutputDisplayStreams { | ||
/// When to display the stdout output of a test in the reporter | ||
pub stdout: Option<TestOutputDisplay>, | ||
/// When to display the stderr output of a test in the reporter | ||
pub stderr: Option<TestOutputDisplay>, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need some tests for this.
A port of the implementation #1707 to resolve the merge conflicts, of which there were many. It currently does not compile, because I'm unfamiliar with the codebase, but it is something.