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.

Sign up for GitHub

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

Add support for silencing only one of the outputs of a test #1707

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+392 −96
Open

Add support for silencing only one of the outputs of a test #1707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 75 additions & 5 deletions cargo-nextest/src/dispatch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@
redact::Redactor,
reporter::{
heuristic_extract_description, highlight_end, structured, DescriptionKind,
FinalStatusLevel, StatusLevel, TestOutputDisplay, TestReporterBuilder,
FinalStatusLevel, StatusLevel, TestOutputDisplay, TestOutputDisplayStreams,
TestReporterBuilder,
},
reuse_build::{archive_to_file, ArchiveReporter, PathMapper, ReuseBuildInfo},
runner::{
Expand All @@ -55,6 +56,7 @@
env::VarError,
fmt,
io::{Cursor, Write},
str::FromStr,
sync::Arc,
};
use swrite::{swrite, SWrite};
Expand Down Expand Up @@ -871,25 +873,31 @@
#[derive(Debug, Default, Args)]
#[command(next_help_heading = "Reporter options")]
struct TestReporterOpts {
/// Output stdout and stderr on failure
/// Output stdout and/or stderr on failure
///
/// Takes the form of: '{value}' or 'stdout={value}' or 'stdout={value},stderr={value}'
/// where {value} is one of: 'immediate', 'immediate-final', 'final', 'never'
#[arg(
long,
value_enum,
conflicts_with_all = &["no-capture", "no-run"],
value_name = "WHEN",
env = "NEXTEST_FAILURE_OUTPUT",
)]
failure_output: Option<TestOutputDisplayOpt>,
failure_output: Option<TestOutputDisplayStreamsOpt>,

/// Output stdout and stderr on success
/// Output stdout and/or stderr on success
///
/// Takes the form of: '{value}' or 'stdout={value}' or 'stdout={value},stderr={value}'
/// where {value} is one of: 'immediate', 'immediate-final', 'final', 'never'
#[arg(
long,
value_enum,
conflicts_with_all = &["no-capture", "no-run"],
value_name = "WHEN",
env = "NEXTEST_SUCCESS_OUTPUT",
)]
success_output: Option<TestOutputDisplayOpt>,
success_output: Option<TestOutputDisplayStreamsOpt>,

// status_level does not conflict with --no-capture because pass vs skip still makes sense.
/// Test statuses to output
Expand Down Expand Up @@ -963,6 +971,68 @@
}
}

#[derive(Debug, Clone, Copy)]
struct TestOutputDisplayStreamsOpt {
stdout: Option<TestOutputDisplayOpt>,
stderr: Option<TestOutputDisplayOpt>,
}

impl FromStr for TestOutputDisplayStreamsOpt {

Check warning on line 980 in cargo-nextest/src/dispatch.rs

View check run for this annotation

Codecov / codecov/patch

cargo-nextest/src/dispatch.rs#L980 

Added line #L980 was not covered by tests
type Err = String;

fn from_str(s: &str) -> Result<Self, Self::Err> {

Check warning on line 983 in cargo-nextest/src/dispatch.rs

View check run for this annotation

Codecov / codecov/patch

cargo-nextest/src/dispatch.rs#L983 

Added line #L983 was not covered by tests
// 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]

Check warning on line 986 in cargo-nextest/src/dispatch.rs

View check run for this annotation

Codecov / codecov/patch

cargo-nextest/src/dispatch.rs#L986 

Added line #L986 was not covered by tests
// - "{stream}={value},{stream=value}": where the two {stream} keys cannot be the same
let (stdout, stderr) = if let Some((left, right)) = s.split_once(',') {
// the "{stream}={value},{stream=value}" case

Check warning on line 989 in cargo-nextest/src/dispatch.rs

View check run for this annotation

Codecov / codecov/patch

cargo-nextest/src/dispatch.rs#L989 

Added line #L989 was not covered by tests
let left = left
.split_once('=')
.map(|l| (l.0, TestOutputDisplayOpt::from_str(l.1, false)));
let right = right
.split_once('=')
.map(|r| (r.0, TestOutputDisplayOpt::from_str(r.1, false)));
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()),
}
Comment on lines +996 to +1006
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, trying to parse it first that way makes sense.

} else if let Some((stream, right)) = s.split_once('=') {
// the "{stream}={value}" case
let value = TestOutputDisplayOpt::from_str(right, false);
match (stream, value) {
("stderr", Ok(stderr)) => (None, Some(stderr)),
("stdout", Ok(stdout)) => (Some(stdout), None),
("stdout" | "stderr", Err(_)) => return Err(format!("\n unrecognized setting for {stream}: [possible values: immediate, immediate-final, final, never]")),
(_, _) => return Err("\n unrecognized output stream, possible values: [stdout={}, stderr={}, stdout={},stderr={}]".to_string())
}
} else if let Ok(value) = TestOutputDisplayOpt::from_str(s, false) {
// the "{value}" case
(Some(value), Some(value))
} else {
// did not recognize one of the three cases
return Err("\n [possible values: immediate, immediate-final, final, never], or specify one or both output streams: stdout={}, stderr={}, stdout={},stderr={}".to_string());
};
Ok(Self { stdout, stderr })
}
}

impl From<TestOutputDisplayStreamsOpt> for TestOutputDisplayStreams {

Check warning on line 1027 in cargo-nextest/src/dispatch.rs

View check run for this annotation

Codecov / codecov/patch

cargo-nextest/src/dispatch.rs#L1012-L1027 

Added lines #L1012 - L1027 were not covered by tests
fn from(value: TestOutputDisplayStreamsOpt) -> Self {
Self {
stdout: value.stdout.map(TestOutputDisplay::from),
stderr: value.stderr.map(TestOutputDisplay::from),
}
}
}

Check warning on line 1035 in cargo-nextest/src/dispatch.rs

View check run for this annotation

Codecov / codecov/patch

cargo-nextest/src/dispatch.rs#L1031-L1035 

Added lines #L1031 - L1035 were not covered by tests
#[derive(Clone, Copy, Debug, ValueEnum)]
enum TestOutputDisplayOpt {
Immediate,
Expand Down
14 changes: 7 additions & 7 deletions nextest-runner/src/config/config_impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use crate::{
},
list::TestList,
platform::BuildPlatforms,
reporter::{FinalStatusLevel, StatusLevel, TestOutputDisplay},
reporter::{FinalStatusLevel, StatusLevel, TestOutputDisplayStreams},
};
use camino::{Utf8Path, Utf8PathBuf};
use config::{builder::DefaultState, Config, ConfigBuilder, File, FileFormat, FileSourceFile};
Expand Down Expand Up @@ -702,14 +702,14 @@ impl<'cfg> NextestProfile<'cfg, FinalConfig> {
}

/// Returns the failure output config for this profile.
pub fn failure_output(&self) -> TestOutputDisplay {
pub fn failure_output(&self) -> TestOutputDisplayStreams {
self.custom_profile
.and_then(|profile| profile.failure_output)
.unwrap_or(self.default_profile.failure_output)
}

/// Returns the failure output config for this profile.
pub fn success_output(&self) -> TestOutputDisplay {
pub fn success_output(&self) -> TestOutputDisplayStreams {
self.custom_profile
.and_then(|profile| profile.success_output)
.unwrap_or(self.default_profile.success_output)
Expand Down Expand Up @@ -905,8 +905,8 @@ pub(super) struct DefaultProfileImpl {
retries: RetryPolicy,
status_level: StatusLevel,
final_status_level: FinalStatusLevel,
failure_output: TestOutputDisplay,
success_output: TestOutputDisplay,
failure_output: TestOutputDisplayStreams,
success_output: TestOutputDisplayStreams,
fail_fast: bool,
slow_timeout: SlowTimeout,
leak_timeout: Duration,
Expand Down Expand Up @@ -1007,9 +1007,9 @@ pub(super) struct CustomProfileImpl {
#[serde(default)]
final_status_level: Option<FinalStatusLevel>,
#[serde(default)]
failure_output: Option<TestOutputDisplay>,
failure_output: Option<TestOutputDisplayStreams>,
#[serde(default)]
success_output: Option<TestOutputDisplay>,
success_output: Option<TestOutputDisplayStreams>,
#[serde(default)]
fail_fast: Option<bool>,
#[serde(default, deserialize_with = "super::deserialize_slow_timeout")]
Expand Down
35 changes: 22 additions & 13 deletions nextest-runner/src/config/overrides.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use crate::{
config::{FinalConfig, PreBuildPlatform, RetryPolicy, SlowTimeout, TestGroup, ThreadsRequired},
errors::{ConfigFiltersetOrCfgParseError, ConfigParseErrorKind},
platform::BuildPlatforms,
reporter::TestOutputDisplay,
reporter::TestOutputDisplayStreams,
};
use guppy::graph::{cargo::BuildPlatform, PackageGraph};
use nextest_filtering::{CompiledExpr, Filterset, FiltersetKind, ParseContext, TestQuery};
Expand All @@ -31,8 +31,8 @@ pub struct TestSettings<Source = ()> {
slow_timeout: (SlowTimeout, Source),
leak_timeout: (Duration, Source),
test_group: (TestGroup, Source),
success_output: (TestOutputDisplay, Source),
failure_output: (TestOutputDisplay, Source),
success_output: (TestOutputDisplayStreams, Source),
failure_output: (TestOutputDisplayStreams, Source),
junit_store_success_output: (bool, Source),
junit_store_failure_output: (bool, Source),
}
Expand Down Expand Up @@ -95,12 +95,12 @@ impl TestSettings {
}

/// Returns the success output setting for this test.
pub fn success_output(&self) -> TestOutputDisplay {
pub fn success_output(&self) -> TestOutputDisplayStreams {
self.success_output.0
}

/// Returns the failure output setting for this test.
pub fn failure_output(&self) -> TestOutputDisplay {
pub fn failure_output(&self) -> TestOutputDisplayStreams {
self.failure_output.0
}

Expand Down Expand Up @@ -497,8 +497,8 @@ pub(super) struct ProfileOverrideData {
slow_timeout: Option<SlowTimeout>,
leak_timeout: Option<Duration>,
pub(super) test_group: Option<TestGroup>,
success_output: Option<TestOutputDisplay>,
failure_output: Option<TestOutputDisplay>,
success_output: Option<TestOutputDisplayStreams>,
failure_output: Option<TestOutputDisplayStreams>,
junit: DeserializedJunitOutput,
}

Expand Down Expand Up @@ -662,9 +662,9 @@ pub(super) struct DeserializedOverride {
#[serde(default)]
test_group: Option<TestGroup>,
#[serde(default)]
success_output: Option<TestOutputDisplay>,
success_output: Option<TestOutputDisplayStreams>,
#[serde(default)]
failure_output: Option<TestOutputDisplay>,
failure_output: Option<TestOutputDisplayStreams>,
#[serde(default)]
junit: DeserializedJunitOutput,
}
Expand Down Expand Up @@ -830,8 +830,14 @@ mod tests {
);
assert_eq!(overrides.leak_timeout(), Duration::from_millis(300));
assert_eq!(overrides.test_group(), &test_group("my-group"));
assert_eq!(overrides.success_output(), TestOutputDisplay::Never);
assert_eq!(overrides.failure_output(), TestOutputDisplay::Final);
assert_eq!(
overrides.success_output(),
TestOutputDisplayStreams::create_never()
);
assert_eq!(
overrides.failure_output(),
TestOutputDisplayStreams::create_final()
);
// For clarity.
#[allow(clippy::bool_assert_comparison)]
{
Expand Down Expand Up @@ -875,9 +881,12 @@ mod tests {
assert_eq!(overrides.test_group(), &test_group("my-group"));
assert_eq!(
overrides.success_output(),
TestOutputDisplay::ImmediateFinal
TestOutputDisplayStreams::create_immediate_final()
);
assert_eq!(
overrides.failure_output(),
TestOutputDisplayStreams::create_final()
);
assert_eq!(overrides.failure_output(), TestOutputDisplay::Final);
// For clarity.
#[allow(clippy::bool_assert_comparison)]
{
Expand Down
Loading
Loading