-
-
Notifications
You must be signed in to change notification settings - Fork 122
Make it easier to compose a subset of nextest in code (as opposed to through CLI) #2622
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
Draft
TobiasvdVen
wants to merge
22
commits into
nextest-rs:main
Choose a base branch
from
TobiasvdVenOrg:non-cli-use
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
c7386cc
Minimal setup to use nextest API without CLI (uses bon::builder)
TobiasvdVen 3fb5062
Remove bon
TobiasvdVen 6264dd1
Add some documentation comments to things that are now public
TobiasvdVen b855116
Add CargoOptions::new()
TobiasvdVen 1af82e1
Remove unnecessary whitespace change
TobiasvdVen c3e9be0
Fix a comment
TobiasvdVen 20dbfcb
Added a newline, as I think that may be tripping up the lint step
TobiasvdVen 23a8bc6
Revert changes to cargo-nextest lib.rs, pub changes must be achieve i…
TobiasvdVen 9ffbc75
Remove apparent unnecessary dependencies on OutputContext
TobiasvdVen 098839a
Remove unnecessary 'use OutputContext'
TobiasvdVen facbc2e
First draft moving cargo_cli.rs and updating ExpectedError accordingly
TobiasvdVen e314ba7
The same for 'acquire_graph_data'
TobiasvdVen 4c0cc44
Extract duplication of TargetTripleError stderr writing
TobiasvdVen a063be8
Fix casing of CargoMetadataError
TobiasvdVen e8099e3
Reduce diff
TobiasvdVen ba7fde4
Reduce diff in dispatch.rs due to auto formatting
TobiasvdVen de04cf4
Bit more cleanup
TobiasvdVen a69d4fc
Fix some new warnings due to missing documentation
TobiasvdVen 08e7d93
Some minor cleanup
TobiasvdVen 023003d
Bit of extra whitespace
TobiasvdVen 55d8015
Remove unnecessary clap optional features
TobiasvdVen 6e50ae2
Fix typo in: nextest-runner/src/errors.rs
TobiasvdVen File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,15 +39,10 @@ pub enum ExpectedError { | |
#[source] | ||
err: std::io::Error, | ||
}, | ||
#[error("cargo metadata exec failed")] | ||
CargoMetadataExecFailed { | ||
command: String, | ||
err: std::io::Error, | ||
}, | ||
#[error("cargo metadata failed")] | ||
CargoMetadataFailed { | ||
command: String, | ||
exit_status: ExitStatus, | ||
#[from] | ||
err: CargoMetadataError | ||
}, | ||
#[error("cargo locate-project exec failed")] | ||
CargoLocateProjectExecFailed { | ||
|
@@ -165,16 +160,10 @@ pub enum ExpectedError { | |
#[source] | ||
err: CreateTestListError, | ||
}, | ||
#[error("failed to execute build command")] | ||
BuildExecFailed { | ||
command: String, | ||
#[source] | ||
err: std::io::Error, | ||
}, | ||
#[error("build failed")] | ||
BuildFailed { | ||
command: String, | ||
exit_code: Option<i32>, | ||
#[from] | ||
err: CreateBinaryListError | ||
}, | ||
#[error("building test runner failed")] | ||
TestRunnerBuildError { | ||
|
@@ -292,26 +281,6 @@ pub enum ExpectedError { | |
} | ||
|
||
impl ExpectedError { | ||
pub(crate) fn cargo_metadata_exec_failed( | ||
command: impl IntoIterator<Item = impl AsRef<str>>, | ||
err: std::io::Error, | ||
) -> Self { | ||
Self::CargoMetadataExecFailed { | ||
command: shell_words::join(command), | ||
err, | ||
} | ||
} | ||
|
||
pub(crate) fn cargo_metadata_failed( | ||
command: impl IntoIterator<Item = impl AsRef<str>>, | ||
exit_status: ExitStatus, | ||
) -> Self { | ||
Self::CargoMetadataFailed { | ||
command: shell_words::join(command), | ||
exit_status, | ||
} | ||
} | ||
|
||
pub(crate) fn cargo_locate_project_exec_failed( | ||
command: impl IntoIterator<Item = impl AsRef<str>>, | ||
err: std::io::Error, | ||
|
@@ -365,26 +334,6 @@ impl ExpectedError { | |
Self::ExperimentalFeatureNotEnabled { name, var_name } | ||
} | ||
|
||
pub(crate) fn build_exec_failed( | ||
command: impl IntoIterator<Item = impl AsRef<str>>, | ||
err: std::io::Error, | ||
) -> Self { | ||
Self::BuildExecFailed { | ||
command: shell_words::join(command), | ||
err, | ||
} | ||
} | ||
|
||
pub(crate) fn build_failed( | ||
command: impl IntoIterator<Item = impl AsRef<str>>, | ||
exit_code: Option<i32>, | ||
) -> Self { | ||
Self::BuildFailed { | ||
command: shell_words::join(command), | ||
exit_code, | ||
} | ||
} | ||
|
||
pub(crate) fn filter_expression_parse_error(all_errors: Vec<FiltersetParseErrors>) -> Self { | ||
Self::FiltersetParseError { all_errors } | ||
} | ||
|
@@ -404,8 +353,7 @@ impl ExpectedError { | |
/// Returns the exit code for the process. | ||
pub fn process_exit_code(&self) -> i32 { | ||
match self { | ||
Self::CargoMetadataExecFailed { .. } | ||
| Self::CargoMetadataFailed { .. } | ||
Self::CargoMetadataFailed { .. } | ||
| Self::CargoLocateProjectExecFailed { .. } | ||
| Self::CargoLocateProjectFailed { .. } => NextestExitCode::CARGO_METADATA_FAILED, | ||
Self::WorkspaceRootInvalidUtf8 { .. } | ||
|
@@ -452,7 +400,7 @@ impl ExpectedError { | |
Self::FromMessagesError { .. } | Self::CreateTestListError { .. } => { | ||
NextestExitCode::TEST_LIST_CREATION_FAILED | ||
} | ||
Self::BuildExecFailed { .. } | Self::BuildFailed { .. } => { | ||
Self::BuildFailed { .. } => { | ||
NextestExitCode::BUILD_FAILED | ||
} | ||
Self::SetupScriptFailed => NextestExitCode::SETUP_SCRIPT_FAILED, | ||
|
@@ -485,20 +433,27 @@ impl ExpectedError { | |
error!("failed to get current executable"); | ||
Some(err as &dyn Error) | ||
} | ||
Self::CargoMetadataExecFailed { command, err } => { | ||
error!("failed to execute `{}`", command.style(styles.bold)); | ||
Some(err as &dyn Error) | ||
} | ||
Self::CargoMetadataFailed { | ||
command, | ||
exit_status, | ||
} => { | ||
error!( | ||
"command `{}` failed with {}", | ||
command.style(styles.bold), | ||
exit_status | ||
); | ||
None | ||
Self::CargoMetadataFailed { err } => { | ||
match err { | ||
CargoMetadataError::CommandExecFail { command, err } => { | ||
error!("failed to execute `{}`", command.style(styles.bold)); | ||
Some(err as &dyn Error) | ||
} | ||
CargoMetadataError::CommandFail { | ||
command, | ||
exit_status | ||
} => { | ||
error!( | ||
"command `{}` failed with {}", | ||
command.style(styles.bold), | ||
exit_status | ||
); | ||
None | ||
} | ||
CargoMetadataError::TargetTriple { err } => { | ||
display_target_triple_error_to_stderr(err) | ||
sunshowers marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
} | ||
Self::CargoLocateProjectExecFailed { command, err } => { | ||
error!("failed to execute `{}`", command.style(styles.bold)); | ||
|
@@ -741,14 +696,7 @@ impl ExpectedError { | |
Some(err as &dyn Error) | ||
} | ||
Self::TargetTripleError { err } => { | ||
if let Some(report) = err.source_report() { | ||
// Display the miette report if available. | ||
error!(target: "cargo_nextest::no_heading", "{report:?}"); | ||
None | ||
} else { | ||
error!("{err}"); | ||
err.source() | ||
} | ||
display_target_triple_error_to_stderr(err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The original printing of |
||
} | ||
Self::RemapAbsoluteError { | ||
arg_name, | ||
|
@@ -822,25 +770,33 @@ impl ExpectedError { | |
error!("creating test list failed"); | ||
Some(err as &dyn Error) | ||
} | ||
Self::BuildExecFailed { command, err } => { | ||
error!("failed to execute `{}`", command.style(styles.bold)); | ||
Some(err as &dyn Error) | ||
} | ||
Self::BuildFailed { command, exit_code } => { | ||
let with_code_str = match exit_code { | ||
Some(code) => { | ||
format!(" with code {}", code.style(styles.bold)) | ||
} | ||
None => "".to_owned(), | ||
}; | ||
Self::BuildFailed { err } => { | ||
match err { | ||
CreateBinaryListError::CommandExecFail { command, error: _ } => { | ||
error!("failed to execute `{}`", command.style(styles.bold)); | ||
Some(err as &dyn Error) | ||
}, | ||
CreateBinaryListError::CommandFail { command, exit_code } => { | ||
let with_code_str = match exit_code { | ||
Some(code) => { | ||
format!(" with code {}", code.style(styles.bold)) | ||
} | ||
None => "".to_owned(), | ||
}; | ||
|
||
error!( | ||
"command `{}` exited{}", | ||
command.style(styles.bold), | ||
with_code_str, | ||
); | ||
error!( | ||
"command `{}` exited{}", | ||
command.style(styles.bold), | ||
with_code_str, | ||
); | ||
|
||
None | ||
None | ||
}, | ||
CreateBinaryListError::FromMessages { error: _ } => { | ||
error!("failed to parse messages generated by Cargo"); | ||
Some(err as &dyn Error) | ||
} | ||
} | ||
} | ||
Self::TestRunnerBuildError { err } => { | ||
error!("failed to build test runner"); | ||
|
@@ -997,3 +953,14 @@ impl ExpectedError { | |
} | ||
} | ||
} | ||
|
||
fn display_target_triple_error_to_stderr(err: &TargetTripleError) -> Option<&(dyn Error + 'static)> { | ||
if let Some(report) = err.source_report() { | ||
// Display the miette report if available. | ||
error!(target: "cargo_nextest::no_heading", "{report:?}"); | ||
None | ||
} else { | ||
error!("{err}"); | ||
err.source() | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,6 @@ | |
|
||
#![warn(missing_docs)] | ||
|
||
mod cargo_cli; | ||
mod dispatch; | ||
#[cfg(unix)] | ||
mod double_spawn; | ||
|
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
Removal of
OutputContext
, was passed intoCargoCli::new
, where its only usage was inCargoCli::to_expression
to add the--color
argument.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.
Where is the
--color
argument passed in now?Uh oh!
There was an error while loading. Please reload this page.
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.
Well, it isn't 😄
The way I went about this is noticing that the
--color
argument seemed unnecessary, as all usages ofto_expression
(the only place--color
was added) are in places that don't display their output - it is captured and only used internally (specifically incompute_binary_list
andacquire_graph_data
).There is this comment in
CargoCli
, which may or may not be relevant:// --color is handled by runner
.I'm not entirely sure what it's intended to communicate, but I think it's explaining why there is no
color
field on theCargoCli
struct. Maybe if we're no longer "internally" adding the--color
argument it should become a field ofCargoCli
, so that users can set it? I'm not sure ifCargoCli
is intended for use beyond what it is currently used for, though, in which case it could be superfluous.My concern would be that I missed some way in which it was relevant. Presumably it was there for a reason, at least at one point, but I was unable to identify one.
Uh oh!
There was an error while loading. Please reload this page.
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.
Well -- when the user runs
cargo nextest run
, we runcargo test --no-run
to build all the tests -- we want to forward the--color
argument to that underlyingcargo test
execution. Totally fine with it being factored out to be easier to do, but we need to maintain this property.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.
Ah, I went and had another look, and I misinterpreted things before.
I thought the output of
cargo test --no-run
(incompute_binary_list
) was being captured, in the sense that it was only used for parsing to a list of binaries. But its output is also sent to stdout, so--color
is relevant.My bad, I'll have to take a different approach.
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 -- stdout (machine-readable output) is captured, but stderr (human-readable output) is passed through.