Skip to content
This repository has been archived by the owner on Dec 29, 2021. It is now read-only.

Setting current_dir() will affect the execution of assert_cli::Assert::main_binary() #118

Open
mssun opened this issue Jul 13, 2018 · 8 comments

Comments

@mssun
Copy link

mssun commented Jul 13, 2018

assert_cli::Assert::main_binary() will use cargo run --quiet -- to execute the CLI. However, if setting current directory outside the project (e.g., /tmp), cargo will not find the corresponding binary.

assert_cli::Assert::main_binary()
        .current_dir("/tmp")
        .unwrap();
---- test stdout ----
	thread 'test' panicked at 'Assertion failed for `cargo run --quiet --`
with: Unexpected return status: failure
stdout=``````
stderr=``````'

Since it is executed "--quiet", there is no output in stderr but returns a failure status.

Can we get the binary

  • assert_cli version: 0.6.2
  • Rust version: 1.26.2
  • OS and version: OS X 10.13
@killercup
Copy link
Collaborator

The "solution" from #116 was to use the assert_cmd crate instead, which future versions of assert_cli will be built up and that doesn't have this problem.

@mssun
Copy link
Author

mssun commented Jul 13, 2018

Thank you for the quick reply. Sorry, I didn't see this closed issue.

How about I leave this issue open? In case other people get confused when using assert_cli. (I spent hours to figure out what's wrong).

@killercup
Copy link
Collaborator

Yeah, that sounds good to me. I've tagged this both "bug" and "assigned" so people can also see that we are working on this/have a solution we need to integrate.

@mssun
Copy link
Author

mssun commented Jul 13, 2018

I found that assert_cli is deprecated.

I'm writing test cases for my CLI, should I use assert_cmd or wait for assert_cli 1.0? Thanks.

@matklad
Copy link

matklad commented Dec 4, 2018

If you, like me, don't find assert_cmd too intuitive to use, here's a snippet of code to make assert_cli to run the binary directly, bypassing cargo run (which I think is the right way to do this, b/c cargo test is gurantees that binaries are built):

// Adapted from
// https://github.com/rust-lang/cargo/blob/485670b3983b52289a2f353d589c57fae2f60f82/tests/testsuite/support/mod.rs#L507
fn target_dir() -> PathBuf {
    env::current_exe().ok().map(|mut path| {
        path.pop();
        if path.ends_with("deps") {
            path.pop();
        }
        path
    }).unwrap()
}

fn cargo_review_deps_exe() -> PathBuf {
    target_dir().join(format!("cargo-review-deps{}", env::consts::EXE_SUFFIX))
}

fn base_cmd() -> Assert {
    Assert::command(&[&cargo_review_deps_exe()])
        .with_args(&["review-deps"])
}

@epage
Copy link
Collaborator

epage commented Dec 4, 2018

If you, like me, don't find assert_cmd too intuitive to use

Mind creating an issue with this feedback so we can explore, what if anything, can be done to improve it?

bypassing cargo run (which I think is the right way to do this, b/c cargo test is gurantees that binaries are built):

Is building binaries as part of cargo test an implementation side effect or designed behavior we can rely on?

Are the paths working in this way an implementation detail that can change?

assert_cmd uses escargot to call cargo build and get the binary location from it. This seems more correct than cargo run but also has its downsides (e.g. cargo acting different with and without --target) while your target_dir could bypass those problems.

Granted, people were also interested in testing examples and so I assume an escargot based approach would still be needed for that.

@matklad
Copy link

matklad commented Dec 4, 2018

Is building binaries as part of cargo test an implementation side effect or designed behavior we can rely on?

That's guaranteed for integration tests:

https://users.rust-lang.org/t/how-do-you-test-binaries-not-libraries/9554/11

Are the paths working in this way an implementation detail that can change?

I think that's guaranteed as well. I bet at this point changing the layout of the target dir will break a ton of code in the wild, starting with Cargo's own tests.

assert_cmd uses escargot to call cargo build and get the binary location from it.

I am not sure that recursively running Cargo during tests is a great idea. Even no-op Cargo-build is O(size of the project), and can easily get to the range of hundreds of milliseconds for large projects. Running the binaries directly should be the right approach. If that doesn't work for some reason, then opening a Cargo issue makes sense (but it does work for cargo itself).

Mind creating an issue with this feedback so we can explore, what if anything, can be done to improve it?

Sure, will do!

@epage
Copy link
Collaborator

epage commented Dec 4, 2018

Even no-op Cargo-build is O(size of the project), and can easily get to the range of hundreds of milliseconds for large projects.

We have it documented how to cache the binary location so the use can make the call to cargo only once.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants