test = false is newly treated inconsistently (and was already not behaving as documented)

[kpreid]

The current stable behavior of the test = false option for targets is:

The test field indicates whether or not the target is tested by default by cargo test.

Passing any target selection option that matches the target, including --all-targets, or even a test filter, overrides test = false and causes the target to be tested. (This is somewhat inconsistent with the documentation, which implies that this override occurs particularly when the target is specifically named on the command line.)

However, PR #15007 was written with the assumption that test = false should be understood to mean “this target contains no tests”, and will cause tests and cfg(test) code inside test = false targets to issue warnings as if test code is useless inside such targets. This behavior is inconsistent and misleading, because it is possible to run such test code, even unintentionally via --all-targets, yet a user encountering those warnings would reasonably conclude that test = false means the crate is never compiled with cfg(test).

It would be valuable to have a way to actually mark targets as containing no test code (to reduce wasted build effort and to eliminate compilation errors from crates that can’t be compiled as tests, when --all-targets is used), but test = false is not, in fact, such a way, and so should not be treated as if it is. Therefore, I believe #15007 should be reverted, and some thought should be given to adding a true “this target contains no tests” option.

@rustbot label +C-bug +regression-from-stable-to-nightly

[weihanglo]

See also the discussion on Zulip: https://rust-lang.zulipchat.com/#narrow/channel/246057-t-cargo/topic/.60test.60.20well.20known.20cfg.20error.

[hanna-kruppe]

It's true that #15007 does not address "this code should never ever be built with --test" use cases, it was never intended to. The behavior of test = false before #15007 also didn't help with that -- but it did make it easy to unintentionally write tests that are never run in CI or by most developers! In particular, there's never been any reason to use cargo test --all-targets if you don't explicitly set test = false and don't add #[test]s to examples (which have test = false by default). Quite the opposite: if you have doctests, --all-targets is currently a trap!

Adding a new "don't try to build this in test mode even with --all-targets or a filter" opt-in may be a good idea as well. But it would have to be a new setting alongside test = false for backwards compatibility. So I think #15007 would still be useful just to prompt people who have cfg(test) code in a test = false build target (especially in examples where it's set implicitly) to review whether this is intentional or not, similar to unexpected_cfgs warnings in many other places. They can make the intent explicit by adding cfg(test) to the expected cfgs, or switch to the "really no tests ever" mode once it exists, or fix a bug if they actually intended to run those tests by default.

Of the two crates discussed in the Zulip thread:

• core really wants the "never compile me in test mode" meaning and is working around the lack of it by cfg-ing out the entire crate's contents in test mode. If that cfg hack wasn't needed, then core would actually benefit from the newly introduced warning to prevent anyone from mistakenly adding a #[test] to core instead of coretests.
• stdarch is only affected because it's a standalone crate that generally has test = true but is also pasted into core (which has test = false) via #[path] instead of being used as a normal dependency. Ignoring this oddity specific to core, stdarch conforms to the expectation that crates with unit tests have test = true, not test = false.

Do we know of any other crates where #15007 has triggered a new warning? May be a bit early, but that would be more illuminating than the above special cases.

This behavior is inconsistent and misleading, because it is possible to run such test code, even unintentionally via --all-targets, yet a user encountering those warnings would reasonably conclude that test = false means the crate is never compiled with cfg(test).

Yes, such a misunderstanding is possible, but people have already been treating test = false as the "this doesn't have any tests" opt-out, either unaware or not interested in this nuance of its behavior. So users were already confused and/or inconsistent about the meaning of this setting. I don't think the new behavior is inconsistent any more than an unexpected_cfg warning about cfg(feature="blah") is: yes, it's technically possible to {run such tests, set this --cfg} but it's very likely to be a subtle mistake, so you should be more explicit about it if you intend to do it.

[hanna-kruppe]

Adding a new "don't try to build this in test mode even with --all-targets or a filter" opt-in may be a good idea as well. But it would have to be a new setting alongside test = false for backwards compatibility.

To make this more explicit: I don't think it's realistic at this stage to fix #8338 as stated, i.e., change cargo test --all-targets to not build/run anything that has test = false. Such a change would break any project that currently relies on --all-targets to override test = false, for whatever reason. Giving an error in those cases would already be backwards incompatible, but silently not running a subset of tests would be even worse even if it has less immediate fallout. It could be tied to an edition, but that's not until 2027, and ideally the edition would just enable by default something that older editions can also opt into. (Like resolver version or edition migration lints.)

In contrast, #15007 doesn't change any behavior (modulo deny(warnings)). Instead, it could be viewed as a sort of soft-deprecation of the test = false + --all-targets combination: existing usages keep working, but the new path of least resistance is to treat test = false as "no tests here" and --all-targets as pointless. I'd consider this a good change because the existing behavior of either feature (let alone their combination) doesn't make anyone happy but changing it abruptly is risky. However, intentionally going down this path definitely deserves at least an FCP, not just a normal PR approval.

[kpreid]

To make this more explicit: I don't think it's realistic at this stage to fix #8338 as stated, i.e., change cargo test --all-targets to not build/run anything that has test = false. Such a change would break any project that currently relies on --all-targets to override test = false, for whatever reason.

Yes, I agree that this would be a breaking change. #8338 is, in my current opinion, more “missing feature; lack of expressiveness” than a bug. Except that the documentation is factually incorrect about when test = false targets are run; that is a bug.

the new path of least resistance is to treat test = false as "no tests here" and --all-targets as pointless.

It’s not pointless. cargo test by default does not test benches. There's no way to run a cargo test that includes benches as well as other targets, without explicitly listing all wanted targets, except by specifying either --all-targets or an equivalent set of other flags. (That’s why the #8338 behavior caused trouble for me and came to my attention in the first place.)

[hanna-kruppe]

Ah, good point, I forgot about benches. If it was up to me I'd like that to change as well, but (1) some harness = false benchmarks may not be well equipped to handle that gracefully, and (2) it's a more intrusive change because benches are probably more common than test = false build targets that actually have tests.