-
Notifications
You must be signed in to change notification settings - Fork 9
No more build before test and lock dependencies #39
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
Conversation
472747a to
47cc9ce
Compare
tcharding
left a comment
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.
ACK 47cc9ce
|
The new warning: unused variable: `name`
--> cargo-rbmt/src/test.rs:174:21
|
174 | let name = parts[0];
| ^^^^
175 | cargo(sh, "run --example {name}").run()?;
| ---------------------- you might have meant to use string interpolation in this string literal
...
183 | cargo(sh, "run --no-default-features --example {name}").run()?;
| -------------------------------------------- you might have meant to use string interpolation in this string literal
...
186 | cargo(sh, "run --example {name} --features={features}").run()?;
| -------------------------------------------- you might have meant to use string interpolation in this string literal
|
= note: `#[warn(unused_variables)]` (part of `#[warn(unused)]`) on by default
help: string interpolation only works in `format!` invocations
|
175 | cargo(sh, format!("run --example {name}")).run()?;
| ++++++++ +
help: string interpolation only works in `format!` invocations
|
183 | cargo(sh, format!("run --no-default-features --example {name}")).run()?;
| ++++++++ +
help: string interpolation only works in `format!` invocations
|
186 | cargo(sh, format!("run --example {name} --features={features}")).run()?;
| ++++++++ +
help: if this is intentional, prefix it with an underscore |
Ah dang, my bad. I have been testing changes to this by just installing it locally which clearly isn't good enough now. Let me take a pass at cleaning things up. |
|
Cheers, legend. |
|
In 07d9757: Why is the build redundant? It builds the crate without |
|
For the record: second commit was reverted in #40 |
Oh, I totally missed this, my bad. Is it possible to write code that some how only works with a test dependency or something? I'll add back the separate build step. |
Yes, by gating some critical part of the code on |
My mistake too, I reviewed. |
c37a853 cargo-rbmt: bring back build before test to catch gating issues (Nick Johnson) Pull request description: As pointed out by @apoelstra in #39 (comment), this isn't a performance thing, it attempts to catch issues where gated test code is actually depended on. ACKs for top commit: tcharding: ACK c37a853 Tree-SHA512: 4df1b9f2261576b05f2c50e97bbf7cb0003f5bf29b3d63e31c0eff863c0094c548cb7e57280854206db16f79eb0005ff54e6452ffea85348f003b2eec3421b95
Two little cleanups which I hope make it easier to add some release/debug_assert settings.
First patch drops the
builds before runningtest. I don't think there is any benefit of this, but maybe there was historically or I am just missing something?Second patch makes sure we are using
--lockedeverywhere so that dependencies are never silently upgraded in CI.