Make cargo check also check unit tests

[tbourvon]

This PR is a POC not to be merged immediately.

These first changes demonstrate basic cargo checking of unit tests by default, and with --tests (in conjunction with --lib or --bin).

There are a few points I would like to discuss and improve upon:

• Should unit tests be checked by default (current behavior), or should it only be opt-in via --tests?
• I am not entirely convinced that hijacking --tests is the best solution, as it binds integration tests checking and unit tests checking. Should we use a separate option like --unit-tests?
• For the moment, it's not possible to select specific unit tests using --test NAME. Do we want this to be possible? It doesn't seem very useful to me.
• It seems reasonable to me to also be able to cargo check unit benchmarks and doctests. Should we also add this possibility?
• Last but not least, is my code/implementation the right way to do this? I wanted the change to be as high level as possible, and the only way to do what I wanted without modifying cargo_rustc was to add another Profile. However, if we are also going to add check_benchmark and check_doctest, changing some code to abstract that away might be wiser.

(This PR is related to #4003 )

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[alexcrichton]

Thanks for the PR! This isn't 100% what I had in mind though per se, but is certainly a possibility! My thinking was that cargo check --tests would imply a command like cargo check --lib --profile test. That is, given just a bare --tests, I think we'd want to run cargo check over the library's unit tests in addition to the integration tests and such.

In theory it's possible to check any target through the standard filters that Cargo has, e.g. --bin foo, --bins, etc. I was thinking we'd just expand the definition of "all tests" here to including the library's unit tests.

Another possibility is to add a --profile flag to cargo check as well. This would be similar to the flag cargo rustc --profile where it says what profile to build a target in. For example cargo check --lib --profile test would compile the lib target in the test profile, e.g. for compiling unit tests.

[tbourvon]

@alexcrichton Seems like the --profile flag is the missing ingredient I wasn't aware of! I'm gonna start working towards what you described, it makes more sense to me now.

However, unless I'm missing something, wouldn't switching to the test profile imply that we aren't using the check profile anymore, and therefore that the binary generation will take place?

[alexcrichton]

Oh we can make sure that check --profile test does the right thing and selects an appropriate profile.

Although... I just ran cargo check --test concurrent -v in Cargo's own directory, and it didn't actually generate rmeta for... anything! I think check --test foo may be totally busted right now? (doing way too much work)

[alexcrichton]

I've opened #4059 for that issue

[tbourvon]

@alexcrichton I'm not sure I understand what you mean with the --profile argument. AFAIK, the current way things are done, you can only use one profile per build target, and the current Profile structs don't offer any way of having check = true && test = true, hence the new Profile I created in my commit. I really feel like I'm missing an obvious, alternate way of doing this from what you're telling me...

What we could do is modify the code in cargo_rustc to offer a completely separate way of using both the test harness and only generating metadata, but I'm not sure that's necessary.

[alexcrichton]

Ah yes so #4059 stems from the fact that we don't have a check_test profile today, so it's definitely right to add one. I think here in this commit the profile selection for each target goes a bit awry. Unfortunately that bit of code is a bit of hairball, but I'll try to explain with an example.

Let's say we've got a Cargo project with a binary, a library, and an integration tests. We should then have:

• cargo check - compile the binary and the library
• cargo check --tests - check the library's unit tests and the integration tests
• cargo check --test foo - just check the integration tests

I think that this PR covers the second case above, but not the other two? Currently today I think Cargo takes care of the first and third (but does too much work on the third), and doesn't include the library's unit tests in the second case.

[tbourvon]

@alexcrichton Alright, I think I got what you meant. I'll work on that soon and send a new commit.

[alexcrichton]

Sorry the complexity is growing here, but let me know if you've got any questions!

[tbourvon]

@alexcrichton Let me know how this looks to you, I think it behaves correctly for the 3 examples you mentioned.

EDIT: it does not fix the --test issue.

[alexcrichton]

This may be an existing issue, but I think that this may not be handled correctly?

cargo check --bin foo --test bar

I think with your PR it wouldn't compile the test bar in the check_test profile, right? I think for this the "global" let profile = match mode { ... }; may not suffice here, the profile selection may need to happen per target?

[tbourvon]

@alexcrichton Well since bar is an integration test it would be compiled with the test profile, just like before. If you're talking about the issue #4059 then I'll fix it in another commit. Or am I missing the point?

[jonhoo]

Ping @alexcrichton ?

[alexcrichton]

sorry for the delay, I haven't forgotten about this! I may have run out of time to get around to this this week but I can try to get to it next week. @matklad if you've got some spare cycles though you may be able to get to this sooner!

[alexcrichton]

@burningmind sorry for the delay with this! I think that the generate_targets function here needs to essentially be entirely reworked. Right now it attempts to select one profile to apply to all the targets that are generated, but that's not actually correct in this case because cargo check --test foo --bin bar would end up requiring two profiles.

I think we'll want to change this to instead generate a list of targets from the manifest and then afterwards match them to a profile to compile in.

[bors]

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

[thejefflarson]

Hi, just letting you all know that this is conflicting with flycheck/flycheck-rust#51 in today's release, because flycheck needs -Z no-trans to check unit tests. Otherwise, congrats on the release today!

[ehuss]

We have also switched over to cargo check in rust-lang/sublime-rust but I am concerned that this PR doesn't quite match our needs. We want to do a complete check for any errors/warnings in a particular file (whether or not the code is behind a cfg(test)). If I am reading this patch right, it looks like it only checks if you pass --tests to check all tests.

Previously with -Z no-trans we were able to pass --test on any target to enable checking the target and any of its tests.

[alexcrichton]

Sorry for taking awhile to get back to this! I think this still suffers the problem though where it's not quite solving all the use cases here. What we really need is to make these lines and the profile calculation a function per target, not one variable which is used for all targets.

[tbourvon]

@alexcrichton I don't have a lot of time to take care of that right now. Is it okay if this is momentarily put on hold for a 2 or 3 weeks?

[alexcrichton]

Sure yeah, no worries!

[jonhoo]

@tbourvon gentle ping about this. I also just noticed that --tests --all will currently not check binaries in subcrates, whereas --all will. Will this PR also fix that behavior?

[tbourvon]

@jonhoo Thanks for the ping. Unfortunately, I don't have the time nor the motivation to take care of this anymore. If someone else wants to look into it, please be my guest!

As for your question, I believe that this is because --all has a specific code path for including absolutely everything, and adding --tests instead goes into a particular build profile, either for building tests or checking them... I'm not certain of this however. Even though I submitted this PR, I don't have a full grasp of the inner workings of the command picker.

[jonhoo]

Ah, I see. I wonder if @alexcrichton might have some more insight on this that he can share?

[ehuss]

I'm willing to take this on.

I've been trying to sort through the different issues (#4003, #3431, #4059, #4330), and I'd like to ensure that there is a way to check the unittests of a particular target. The only way I've seen proposed to do that is to add a --profile test flag.

I'd like to be absolutely clear about what the different flags should do. Here's a summary of how I think it could work:

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 --tests	check,check_test†	-†	check	-	-
check --all-targets	check,check_test†	check	check	check	check

† = different behavior from today

Does this behavior sound good?

Potentially in the future I imagine cargo check could add doctests to its repertoire, possibly with --profile doctest? That is outside the changes here, but a possibility to keep in mind.

EDIT: A big caveat with doing both check and check_test on --tests and --all-targets is that errors/warnings outside of #[cfg(test)] will show up twice. It will also take twice as long to run. My reasoning for suggesting that both should run is that #[cfg(test)] can fundamentally change what is compiled, so it is possible for something to pass check_test and not pass check. However, that should be extremely rare. Another concern is the two issues I posted earlier when passing --test to rustc: missing_docs lints are disabled, and main not found error is disabled. If these issues are not severe enough, then I think it would be reasonable to just run check_test on the lib.

[alexcrichton]

Thanks so much for the table there @ehuss! That looks great to me! Would you be interested in starting a PR for that?

[ehuss]

Yea, I'll try to post a PR soon.

[alexcrichton]

Awesome, thanks!

[alexcrichton]

I believe this mostly landed with #4592, so closing.