Allow multiple executions for test targets #29
Conversation
|
Thanks for the PR and I'm glad you find this project useful! Just so that I understand you correctly: You want to build a project and run all (as in |
|
Yes, you understand what I'm doing here. I did this quickly, and it works for my use case. I think the restriction that was there before is correct for other targets, like For an example of it's usage, see here: https://github.com/bluejekyll/trust-dns/blob/3b6b50e99fbe7503e0248deb74bf4664b381923f/Makefile.toml#L415-L416 This will end up expanding to these, as can be seen in the logs: |
| @@ -61,7 +61,7 @@ impl<'a> CargoCmd<'a> { | |||
|
|
|||
| /// Run the cargo command and get the output back as a vector | |||
| pub(crate) fn run(&self) -> Result<CargoBuildOutput, Error> { | |||
| debug!( | |||
| println!( | |||
There was a problem hiding this comment.
Not sure we want to print this by default
There was a problem hiding this comment.
See the above output. Without this line, we end up with very little information about what command is being run that executes immediately after that.
There was a problem hiding this comment.
You could use info instead of debug here.
| } | ||
|
|
||
| /// Selects the buildopt which fits with the requirements | ||
| /// | ||
| /// If there are multiple possible candidates, this will return an error | ||
| fn select_buildopt(&self) -> Result<&CargoBuildOutputElement, Error> { | ||
| fn select_buildopt(&self) -> Result<Vec<PathBuf>, Error> { |
There was a problem hiding this comment.
Now that this function can return several paths the docs and names should be updated
| @@ -201,14 +201,14 @@ struct Target { | |||
| } | |||
|
|
|||
| impl<'a> CargoBuildOutput<'a> { | |||
| pub(crate) fn artifact(&self) -> Result<PathBuf, Error> { | |||
| Ok(self.select_buildopt()?.artifact()) | |||
| pub(crate) fn artifact(&self) -> Result<Vec<PathBuf>, Error> { | |||
There was a problem hiding this comment.
Function name should reflect that this returns multiple artifacts
| @@ -233,33 +233,51 @@ impl<'a> CargoBuildOutput<'a> { | |||
| // We expect exactly one candidate; everything else is an error | |||
There was a problem hiding this comment.
With this change, we should also update the comments in this regard.
| @@ -233,33 +233,51 @@ impl<'a> CargoBuildOutput<'a> { | |||
| // We expect exactly one candidate; everything else is an error | |||
| match candidates.as_slice() { | |||
| [] => Err(err_msg("No suitable build artifacts found.")), | |||
| [ref the_one] => Ok(the_one), | |||
| [ref the_one] => Ok(vec![the_one.artifact()]), | |||
There was a problem hiding this comment.
Is there a reason to still differentiate between one and several artifacts here?
| @@ -26,7 +26,7 @@ fn main() { | |||
| } | |||
|
|
|||
| // Make a separate runner to print errors using Display instead of Debug | |||
| fn try_main() -> Result<Void, Error> { | |||
| fn try_main() -> Result<(), Error> { | |||
There was a problem hiding this comment.
I can't remember the reason for using Void, but thanks for getting rid of that dependency! Could you also fix the unused import and the Cargo.toml file?
There was a problem hiding this comment.
Before we used exec in the sense that it "took over" the current process to run the new command. That way instead of using pipes etc. for stdin and stdout, the started command (such as gdb) would be executed in place of the cargo with command. Not doing this might have unfortunate consequences when using interactive commands, hence we have to make sure that this PR doesn't break that usecase.
There was a problem hiding this comment.
that's a very good point. I could leave this function alone, possibly, and execute in a different manner with one of the below options.
|
As an aside, this is a breaking change hence I think it should be "hidden" behind a cli option. |
Is it? Previously execution aborted with non-zero exit code in these cases. Now such cases succeed. So its not breaking something which was working previously, but enables a few things which failed previously. I don't think that stability guarantees should extend to failures - at least not for a small |
|
That's a fair point. I was just thinking about the situation where you would expect the tool to fail, but now it continues. Let's say you want to examine the tests of your library, but not your binary, with the same name. Previously it would error explaining you to be more specific, but now it will just throw you into a gdb environment which debugs both the binary and library. I am not sure what classifies a breaking change using minor versions though, so I may be wrong. EDIT: And as you say, turning an error into success is probably not a breaking change after all. |
I've been working with
cargo-makerecently, and coverage reports weren't accurate. I came across this project, and found the requirement to specify which test you wanted to run to be a little onerous.This change essentially loops over all targets that might have tests, we might want the others, but right now,
lib,tests,bin, andexamplesare supported. Not sure if we'll want to use this. When this is combined with--all-targets,--bins,--examples, and/or--teststhis should work.I'm only posting a draft for feedback at the moment. I need to add a test to cover this before it's ready.
Thanks for the great project!