assert-rs / assert_cli

See instead assert_cmd
https://docs.rs/assert_cmd
Apache License 2.0
149 stars 21 forks source link

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

Open mssun opened 6 years ago

mssun commented 6 years ago

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

killercup commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.