Add unit test checking to `cargo check` #4592

ehuss · 2017-10-07T17:05:02Z

This is an extension of PR #4039, fixing #3431, #4003, #4059, #4330. The fixes for #4059 can potentially be separated into a separate PR, though there may be some overlap.

The general gist of the changes:

Add --profile test flag to cargo check that will enable checking of unit tests.
--tests will implicitly enable checking of unit tests within the lib (checks the same targets as cargo test). This affects the check, test, and build commands.
--benches behaves similarly by using the same targets as cargo bench.
Fix erroneously linking tests when run with --test.

There is one thing I did not do because I wanted more feedback on what others think the expected behavior should be. What should the behavior of --all-targets be? This patch does not (yet) make any changes to its behavior. My initial thinking is that it should add a target of --lib --bins --profile test, but that essentially means the lib and bin targets will be checked twice (and thus any errors/warnings outside of #[cfg(test)] will appear twice, which may be confusing, and generally take twice as long to run). I can add that, but I think it would require adding a new All variant to CompileFilter so that the code in generate_targets can detect this scenario. I wanted feedback before making a more extensive change like that. The downside of not adding it is that --all-targets will ignore unit tests (if you don't specify --profile test).

Summary of the profiles used with this patch:

Command	Lib	Bin foo	Test t1	Example e1	Bench b1
`check`	check	check	-	-	-
`check --profile test`	check_test†	check_test†	-	-	-
`check --lib`	check	-	-	-	-
`check --lib --profile test`	check_test†	-	-	-	-
`check --bin foo`	check	check	-	-	-
`check -–bin foo –profile test`	check_test†	check_test†	-	-	-
`check --bins`	check	check	-	-	-
`check --test t1`	check	check	check_test	-	-
`check --tests`	check, check_test†	check, check_test†	check_test	check†, check_test†	-
`check --all-targets`	check, check_test†	check, check_test†	check_test	check, check_test†	check_test

† = different behavior from today

rust-highfive · 2017-10-07T17:05:07Z

r? @matklad

(rust_highfive has picked a reviewer for you, use r? to override)

ehuss · 2017-10-08T00:46:46Z

~~Closing till I fix this.~~ Fixed the broken tests.

ehuss · 2017-10-08T20:50:09Z

I've discovered some problems with this PR, I will be fixing them soon.
EDIT: Ok, I think it's fixed now.

bors · 2017-10-14T15:51:47Z

☔ The latest upstream changes (presumably #4616) made this pull request unmergeable. Please resolve the merge conflicts.

- Add `--profile test` flag to `cargo check` that will enable checking of unit tests. - `--tests` will implicitly enable checking of unit tests within the lib. - Don't implicitly compile binaries when using just `--test` filters. - Fix erroneously linking tests when run with `--test`. Fixes rust-lang#3431, rust-lang#4003, rust-lang#4059, rust-lang#4330.

alexcrichton

Awesome thanks so much and sorry for the delay!

alexcrichton · 2017-10-19T19:47:42Z

src/bin/check.rs

-        filter: ops::CompileFilter::new(options.flag_lib,
-                                        &options.flag_bin, options.flag_bins,
+        filter: ops::CompileFilter::new(options.flag_lib || options.flag_tests,
+                                        &options.flag_bin, options.flag_bins || options.flag_tests,


Could you add a comment to why these two || conditions were added?

alexcrichton · 2017-10-19T19:48:56Z

src/bin/check.rs

@@ -106,6 +111,17 @@ pub fn execute(options: Options, config: &mut Config) -> CliResult {
                                    &options.flag_exclude,
                                    &options.flag_package)?;

+    let test = options.flag_tests ||


This logic may cause subtle oddities, although that's maybe ok for now? but for example:

cargo check --tests --bin foo

I think that'll check the binary foo in "test mode" rather than "do you compile mode", right?

You're right. The more I think about it, it is probably wrong. check --tests should not check bins with the test profile. I'll fix it.

alexcrichton · 2017-10-19T19:49:31Z

src/cargo/ops/cargo_rustc/context.rs

@@ -862,7 +862,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
        ret.extend(self.maybe_lib(unit));

        // Integration tests/benchmarks require binaries to be built
-        if unit.profile.test &&
+        if unit.profile.test && !unit.profile.check &&


Could you expand the above comment to explain this negation?

ehuss · 2017-10-21T15:15:27Z

OK, I have updated it with a different approach. After thinking about it more, I think people expect check --tests to check all tests, similar to cargo test. So I have reworked it so that --tests uses the same behavior as cargo test. This means it will also affect other commands that use --tests (like build, bench, and test). This also makes test --tests somewhat redundant, but consistent across the commands. I'm not entirely sure if that's for the best, I'm not sure which behavior is more surprising.

I also updated --benches to have similar behavior (mimicking what happens with cargo bench). Finally, --all-targets now really does all targets. I updated the summary above.

Also, I have so far been ignoring doctests. Since I believe that will rely on rustdoc, it might be outside of the domain of cargo check?

alexcrichton · 2017-10-30T15:57:04Z

Gah sorry for the delay! In any case that all sounds great to me, thanks so much!

@bors: r+

bors · 2017-10-30T15:57:05Z

📌 Commit efc0dd1 has been approved by alexcrichton

Add unit test checking to `cargo check` This is an extension of PR #4039, fixing #3431, #4003, #4059, #4330. The fixes for #4059 can potentially be separated into a separate PR, though there may be some overlap. The general gist of the changes: - Add `--profile test` flag to `cargo check` that will enable checking of unit tests. - `--tests` will implicitly enable checking of unit tests within the lib (checks the same targets as `cargo test`). This affects the `check`, `test`, and `build` commands. - `--benches` behaves similarly by using the same targets as `cargo bench`. - Fix erroneously linking tests when run with `--test`. There is one thing I did not do because I wanted more feedback on what others think the expected behavior should be. What should the behavior of `--all-targets` be? This patch does not (yet) make any changes to its behavior. My initial thinking is that it should *add* a target of `--lib --bins --profile test`, but that essentially means the lib and bin targets will be checked twice (and thus any errors/warnings outside of `#[cfg(test)]` will appear twice, which may be confusing, and generally take twice as long to run). I can add that, but I think it would require adding a new `All` variant to `CompileFilter` so that the code in `generate_targets` can detect this scenario. I wanted feedback before making a more extensive change like that. The downside of not adding it is that `--all-targets` will ignore unit tests (if you don't specify `--profile test`). Summary of the profiles used with this patch: Command | Lib | Bin foo | Test t1 | Example e1 | Bench b1 | ------- | --- | ------- | ------- | ---------- | -------- | `check` | check | check | - | - | - | `check --profile test` | check_test† | check_test† | - | - | - | `check --lib` | check | - | - | - | - | `check --lib --profile test` | check_test† | - | - | - | - | `check --bin foo` | check | check | - | - | - | `check -–bin foo –profile test` | check_test† | check_test† | - | - | - | `check --bins` | check | check | - | - | - | `check --test t1` | check | check | check_test | - | - | `check --tests` | check, check_test† | check, check_test† | check_test | check†, check_test† | - | `check --all-targets` | check, check_test† | check, check_test† | check_test | check, check_test† | check_test | † = different behavior from today

bors · 2017-10-30T16:50:21Z

⌛ Testing commit efc0dd1 with merge 8be175e...

bors · 2017-10-30T17:31:16Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 8be175e to master...

ehuss · 2017-10-30T18:47:07Z

Thanks @alexcrichton, I know you are super busy (I'm still not convinced you are just one human). I had some questions:

Is there anything I can do to help with the release notes process?
I noticed bors only closed one of the issues. Should I add comments to each one explaining how it is fixed?

I assume this will get picked up in nightly the next time Cargo is updated in the main rust repo for people to test. I feel like this is a subtle, but significant change in behavior. Hopefully most people will be happy with the changes.

alexcrichton · 2017-10-30T22:46:21Z

Is there anything I can do to help with the release notes process?

Certainly! There's a PR upstream for the 1.22 release (which this won't be included in), but you can start a new section for the 1.23 release which would be awesome!

Should I add comments to each one explaining how it is fixed?

If you're willing, that'd be awesome! If not, feel free to just leave a comment saying it should be closed now.

I assume this will get picked up in nightly the next time Cargo is updated in the main rust repo for people to test.

Indeed! If you'd like to accelerate that, feel free to send a PR yourself to rust-lang/rust :)

ehuss · 2017-10-30T23:28:38Z

It looks like @Aaronepower has been writing the release notes. I'll just leave this here for when he gets started on 1.23, and let him format/wordsmith as desired.

Cargo
-----
- [`cargo check --profile test` added to check unit tests][cargo/4592]
- [`--tests`, `--benches`, and `--all-targets` flags will now include all
  potential targets for their respective type][cargo/4592]

[cargo/4592]: https://github.com/rust-lang/cargo/pull/4592

jonhoo · 2018-01-24T04:12:29Z

@ehuss how does --all-targets interact with --all? Can you specify both?

ehuss · 2018-01-24T04:36:43Z

@ehuss how does --all-targets interact with --all? Can you specify both?

@jonhoo It should work (it should build every target of every package). Are you experiencing any issues?

jonhoo · 2018-01-24T22:13:11Z

Sorry, no, this was a case of asking before trying. Seems to work as expected! It just wasn't documented anywhere that these work together, though I guess there isn't a good reason why they shouldn't. I suspect it would have helped if the docs somewhere said that selecting target crates and selecting profiles are orthogonal actions, but meh.

ehuss · 2018-01-24T23:42:11Z

selecting target crates and selecting profiles are orthogonal actions, but meh.

I've seen that target and profile selection is a common source of confusion. Along with improving documentation, I wonder if it might be helpful to display each target/profile being built when -v is used (trying to understand that from the rustc command-line isn't exactly easy).

rust-highfive assigned matklad Oct 7, 2017

ehuss closed this Oct 8, 2017

ehuss reopened this Oct 8, 2017

ehuss mentioned this pull request Oct 8, 2017

cargo check --examples shouldn't link binaries #4529

Closed

ehuss added 3 commits October 14, 2017 18:41

Fix broken tests due to different profile selection.

390e957

Fix wrong profile selection for cargo build and cargo rustc.

0acc67c

ehuss force-pushed the check_test branch from 070ae38 to 0acc67c Compare October 15, 2017 02:09

alexcrichton reviewed Oct 19, 2017

View reviewed changes

ehuss added 3 commits October 20, 2017 14:34

Rework handling of --tests and --benches with implied targets.

3dd21d8

Fix broken bench test for new --benches behavior.

e171e00

Don't change order of tests and benches for cleaner diff.

efc0dd1

alexcrichton added the relnotes Release-note worthy label Oct 30, 2017

bors merged commit efc0dd1 into rust-lang:master Oct 30, 2017

orium mentioned this pull request Oct 30, 2017

Errors in tests are not shown intellij-rust/intellij-rust#1433

Closed

This was referenced Oct 30, 2017

Build subcommand or option that builds all targets (including tests) #2495

Closed

Semantic errors ignored in tests with cargo check --tests and cargo build --tests #4003

Closed

cargo check --test foo compiles too much #4059

Closed

ehuss mentioned this pull request Oct 30, 2017

cargo check --tests not working #4330

Closed

jonhoo mentioned this pull request Oct 31, 2017

Show errors in Rust test code dense-analysis/ale#626

Closed

alexcrichton mentioned this pull request Oct 31, 2017

Make cargo check also check unit tests #4039

Closed

Xanewok mentioned this pull request Nov 1, 2017

Test directory not yet analysed rust-lang/rls#191

Closed

arthurprs mentioned this pull request Nov 27, 2017

'check all' option is not working correctly AtomLinter/linter-rust#120

Closed

epage mentioned this pull request Feb 16, 2018

[DISCUSS] Method of #![deny(current_warnings)] crate-ci/example-warn#1

Closed

jonhoo mentioned this pull request Dec 12, 2018

Cargo check does not check doctests #6424

Open

fmdkdd mentioned this pull request Jan 31, 2019

rust-cargo: prefer "check --tests" to "test --no-run" flycheck/flycheck#1540

Open

thomaseizinger mentioned this pull request Feb 15, 2019

cargo test --all-targets does not run doc tests #6669

Open

lopopolo mentioned this pull request Jan 6, 2021

Add --all-targets flag to cargo check calls in CI artichoke/artichoke#1009

Closed

ehuss added this to the 1.23.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit test checking to `cargo check` #4592

Add unit test checking to `cargo check` #4592

ehuss commented Oct 7, 2017 •

edited

Loading

rust-highfive commented Oct 7, 2017

ehuss commented Oct 8, 2017 •

edited

Loading

ehuss commented Oct 8, 2017 •

edited

Loading

bors commented Oct 14, 2017

alexcrichton left a comment

alexcrichton Oct 19, 2017

alexcrichton Oct 19, 2017

ehuss Oct 19, 2017

alexcrichton Oct 19, 2017

ehuss commented Oct 21, 2017

alexcrichton commented Oct 30, 2017

bors commented Oct 30, 2017

bors commented Oct 30, 2017

bors commented Oct 30, 2017

ehuss commented Oct 30, 2017

alexcrichton commented Oct 30, 2017

ehuss commented Oct 30, 2017

jonhoo commented Jan 24, 2018

ehuss commented Jan 24, 2018

jonhoo commented Jan 24, 2018

ehuss commented Jan 24, 2018

Add unit test checking to cargo check #4592

Add unit test checking to cargo check #4592

Conversation

ehuss commented Oct 7, 2017 • edited Loading

rust-highfive commented Oct 7, 2017

ehuss commented Oct 8, 2017 • edited Loading

ehuss commented Oct 8, 2017 • edited Loading

bors commented Oct 14, 2017

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton Oct 19, 2017

Choose a reason for hiding this comment

alexcrichton Oct 19, 2017

Choose a reason for hiding this comment

ehuss Oct 19, 2017

Choose a reason for hiding this comment

alexcrichton Oct 19, 2017

Choose a reason for hiding this comment

ehuss commented Oct 21, 2017

alexcrichton commented Oct 30, 2017

bors commented Oct 30, 2017

bors commented Oct 30, 2017

bors commented Oct 30, 2017

ehuss commented Oct 30, 2017

alexcrichton commented Oct 30, 2017

ehuss commented Oct 30, 2017

jonhoo commented Jan 24, 2018

ehuss commented Jan 24, 2018

jonhoo commented Jan 24, 2018

ehuss commented Jan 24, 2018

Add unit test checking to `cargo check` #4592

Add unit test checking to `cargo check` #4592

ehuss commented Oct 7, 2017 •

edited

Loading

ehuss commented Oct 8, 2017 •

edited

Loading

ehuss commented Oct 8, 2017 •

edited

Loading