-
Notifications
You must be signed in to change notification settings - Fork 131
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
Update integration test to lower runtimes #883
base: main
Are you sure you want to change the base?
Conversation
cargo-audit/tests/acceptance.rs
Outdated
@@ -26,9 +26,9 @@ static ADVISORY_DB_DIR: Lazy<TempDir> = Lazy::new(|| TempDir::new().unwrap()); | |||
/// be multithreaded invocations as `cargo test` executes tests in | |||
/// parallel by default. | |||
pub static RUNNER: Lazy<CmdRunner> = Lazy::new(|| { | |||
let mut runner = CmdRunner::default(); | |||
let mut runner = CmdRunner::new(env!("CARGO_BIN_EXE_CARGO_AUDIT")); |
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.
What is this environment variable?
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.
Seems it's breaking the tests...
error: environment variable `CARGO_BIN_EXE_CARGO_AUDIT` not defined at compile time
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.
It seems this should be using std::env::var_os
instead of env!
.
I doubt this is going to be any faster, but I'd be happy to be proven wrong.
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.
And what's it supposed to do if that returns None
?
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.
Panic - it's supposed to be always set: https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-sets-for-crates
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.
Nevermind, I got it all wrong. It should be env!("CARGO_BIN_EXE_cargo-audit")
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.
Yeah, my mistake. I could only try it with a tool that didn't have a hyphen in its name, so I was unsure how it would be called. But CARGO_BIN_EXE_cargo-audit
or I guess CARGO_BIN_EXE_CARGO-AUDIT
should be working now.
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.
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.
I haven't investigated exactly why that is, but a first guess seemed like my runtime was long because each stage was cloning RUNNER
, and was rerunning cargo run --
each time. So only the overhead of that was 10s on my machine (Win11) adding up for each test.
Test is here:
https://github.com/simonsan/rustic/blob/refactor/tests/backup_restore.rs#L31
So my understanding was, if I would use the binary path directly, as that shouldn't recompile anything in any case, it should be circumventing that.
Try with binary scanning
TL;DR: this is a worthwhile change, but I'd like to see it on the framework level (i.e. in I've compared this against the main branch, measuring the second run of the tests (once the crates.io index cloning is done, which takes up the vast majority of the time), and it takes 50 seconds regardless of these changes. With an up-to-date crates.io index, running Contrary to @simonsan's suspicion, the compilation is performed exactly once, and cached after that. There's no recurring compilation assuming the Cargo cache works properly. |
Before:Commit: 0a33e37 After: |
It actually seems like there is something wrong with the command runner and the |
This will occur if a test fails while the mutex is held. It should probably use |
Does this have any downsides? It should speed up the integration tests overall?
could also help with #832
related: iqlusioninc/abscissa#845