main_binary takes ~300ms per invocation

[epage]

From @rcoh assert-rs/assert_cli#100

• assert_cli version: 0.5
• Rust version: 1.25.0
• OS and version: Ubuntu 16.04
  Unfortunately, cargo run takes at minimum about 300ms on my computer:

➜  angle-grinder git:(master) ✗ time cargo run --bin agrind -- '* | json | count' --file test_files/empty
    Finished dev [unoptimized + debuginfo] target(s) in 0.0 secs
     Running `target/debug/agrind '* | json | count' --file test_files/empty`
No data
cargo run --bin agrind -- '* | json | count' --file test_files/empty  0.22s user 0.06s system 92% cpu 0.307 total

It makes it impractical to run hundreds of integration tests with assert_cli.

I'm not positive what a better way would be -- I guess running the binary in dist directly if it exists?

Workaround

See assert_cmd::cargo's docs

[epage]

From killercup: cargo issue: rust-lang/cargo#5315

[epage]

So a user can work around this in assert_cmd by accessing cargo_bin_path, etc. This causes the 300ms to only be hit once instead of on every run.

lazy_static! {
    const BIN_UNDER_TEST = assert-cmd::cargo_bin_path("bin_fixture");
}

#[test]
fn test_bin() {
    Command::new(BIN_UNDER_TEST).assert().success();
}

Should we provide an API on to simplify this?

Example

lazy_static!{
   const BIN_FIXTURE = assert_cmd::cargo_bin("bin_fixture");
}
...
println!("{:?}", BIN_FIXTURE.path());
BIN_FIXTURE.cmd().arg("stdout", 42).unwrap();

[zetok]

Additions to API aren't needed, but docs that would point out this functionality would be really helpful. Since right now one needs to go to the issue tracker of the crate to find the solution, which is suboptimal.

As for why an additional API is not needed – e.g. regex crate doesn't have an additional API for creating a static regexp, but it has a great documentation where it shows on the "main" page of its docs an example on how to use lazy_static to reduce unnecessary computations. It's nice.

Also, a working example, freshly ~copypasted from a test for anyone else who's interested:

use std::path::PathBuf;

lazy_static! {
    static ref BIN_PATH: PathBuf = assert_cmd::cargo::main_binary_path().unwrap();
}

#[test]
fn test_bin() {
    Command::new(&*BIN_PATH).assert().success();
}

[epage]

As for why an additional API is not needed – e.g. regex crate doesn't have an additional API for creating a static regexp

The difference is the main purpose of this crate is running programs. Its a bit of a wart in the API to have functions that provide paths to programs you could run. It'd be nice if Command supported clone.

[epage]

I think at this point this is mostly kept open to track and be aware of the slow down. We have workarounds but thats all they are.

[killercup]

Have we ever measured this again with newer cargo versions? I recall them adding some caching for this, too.

…

On Sun, 7 Oct 2018, 00:56 Ed Page, ***@***.***> wrote: I think at this point this is mostly kept open to track and be aware of the slow down. We have workarounds but thats all they are. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#6 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABOX9lsA8jsePQJ2AkErzGU37rEu00Xks5uiTUogaJpZM4UQkyB> .

[rcoh]

@killercup , I just reran the original example that started everything. On the same laptop with nightly, it takes 125ms, so twice as fast as before. A big enough change that it actually feels way faster 👍

cargo 1.31.0-nightly (ad6e5c003 2018-09-28)

[epage]

Good idea. I should add a benchmark that compares running the underlying program against running it via cargo.

[epage]

More data

• My computer
• cargo 1.31.0-nightly (de314a8b2 2018-09-21)
• Running chore: Add cargo benchmark #56

test escargot_rebuild ... bench: 368,686,700 ns/iter (+/- 64,925,719)

[sharkdp]

I think we might have run into a related issue in bat. I tried to implement the workaround but couldn't quite figure it out as some of the referenced functions are not available anymore(?). Is there still a workaround for this?

[epage]

@sharkdp sorry, I have been maintaining the work around in the docs but forgot to update this issue. I've done so now.

[sharkdp]

@epage Thank you very much! Sorry for not checking the documentation.

If I understand this correctly, this will still potentially compile (part of) the project in the first unit test, right?

(link to my current implementation: sharkdp/bat#398)

[epage]

If I understand this correctly, this will still potentially compile (part of) the project in the first unit test, right?

To get the path, it will still need to call into cargo. Whether anything builds is a different question. If you match target/release with what you are building, then it should reuse what was built before.

I recommend passing in current_release. An unfortunate thing is if you pass in current_target and you did not pass --target into cargo test, then it will rebuild everything on first test. There was no good answer between correctness and performance, so the default for Command::main_bin and the workaround in the docs is current_target

[sharkdp]

@epage Thank you very much for the explanation.

[sourcefrog]

On Windows, for my program conserve, switching to assert_cmd from my own hacky code seems to have increased the overall test time from ~3:00 to ~4:00.

https://ci.appveyor.com/project/sourcefrog/conserve/builds/20036005

I think it'd at least be nice if this was more prominent in the docs top-level page, where there are examples of using the crate.

[epage]

@sourcefrog looking at your log, it looks like you are more so having problems with #57 (realized I should create a dedicated issue rather than pointing people to #4).

#57 does need to be mentioned in the docs but I'm unsure if the top-level docs are appropriate for each workaround or special case. I also noticed that this issue's workaround is only mentioned in the cargo module and not on the trait or functions. If someone jumps straight to one of those, they need to be aware of these workarounds.

I've created #58 to document the workaround and try to find a way to improve visibility.