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

[thomaseizinger]

As per title cargo test --all-targets does not run the doc tests on a crate. This is very surprising behaviour. I found this and it looks like running doc tests is done by rustdoc which is apparently not invoked when doing --all-targets.

[dwijnand]

An attempt to fix this was made in #6671, but there's a bit of a mismatch between cargo test and doc tests that makes the issue complicated, so the PR didn't land.

[thomaseizinger]

Maybe it is worth to add some context to this issue.

We ran into this because we use cargo test --all-targets on Travis and usually cargo test locally.
A not-compiling doc test sneaked into a last-minute update of a PR but the build was fine since we use --all-targets on Travis.
Later on, someone complained that we have a broken master because of a non-compiling doc test.

I read some of the discussion in the #6671 and TBH, the whole --doc is a mode would be SUPER confusing.
As a user, I expect cargo test to test LESS than cargo test --all-targets since the latter has all in the name.
In addition, the documentation (--help) lists doc tests next to all the other targets.

[glts]

This means the command to run absolutely all tests everywhere is cargo test --workspace, not cargo test --workspace --all-targets, right?

[ehuss]

This means the command to run absolutely all tests everywhere is cargo test --workspace, not cargo test --workspace --all-targets, right?

It depends on the structure of your packages, but in general there is currently no single command to "test everything". cargo test is intended to be the usual command. If you also want to test benchmarks and examples, you can either mark them with test=true in Cargo.toml, or run two commands (cargo test --all-targets and cargo test --doc).

[thomaseizinger]

Just for some clarification, cargo has a model where there is a distinction between a target and a mode. Doctests are a mode, and that is why --all-targets by itself doesn't run them. I understand that can be confusing, isn't obvious from the documentation, and not the mental model some people have. We have talked in the past about making that more explicit (which in part would solve this issue by offering new switches to do both "run tests" and "run doctests"). I can't find that discussion offhand, but I don't think it was very deep anyway (just acknowledging that the CLI is limited and there were some ideas around a --mode flag). Perhaps that's something the testing team could tackle 😉 .

I don't know anything about the internals of cargo and I've been using Rust for close to 6 years now in a full-time setting. This is the first time I am hearing about cargo having a mode (other than the "release mode" toggled by --release).

Can we just remove the --doc flag and always include doc tests in every target that is selected, i.e. --lib, --tests, --examples etc. The usecase of only running doc tests could be served by a new flag of cargo doc: cargo doc --tests although I'd probably play it conservative and not add that one to begin with. It seems odd wanting to run only the doc tests. If doc tests are just more tests that get compiled into the crate, they could be selected via the regular filtering of test names. Perhaps they should all be under a dedicated doc module? Something like cargo test mycrate::doc::my_function_name_that_has_doc_test.

[est31]

Can we just remove the --doc flag and always include doc tests in every target that is selected

doctests are not unit tests, they are integration tests. In other words, they require a separate rustc process to be spawned for each snippet. On top of that, you have the rustdoc process (which isn't that neccessary, one could move the logic to extract the snippets to a place shared by rustc and rustdoc).

edit: there is the third issue, one that all integration tests face, that they can depend on the crate as an external dependency. In other words you need to compile the lib crate with #[cfg(test)] disabled.

[joshtriplett]

In today's @rust-lang/cargo meeting, we agreed to try the experiment of adding an --all-targets-including-doc unstable option, initially to cargo test and then to other commands like cargo build and cargo check. We don't yet know what we should call it, but we can experiment with implementation of it.

[Veetaha]

Ah, I know that feel. I had the same problem at work with a similar option of all that we couldn't scale to include more options.

A naming suggestion (we used a similar one as well): --all-targets-and-doc

[epage]

Also, when we do build, check, and fix (and suggest for clippy to do similar), we might want to add --doc flags to them (#6424).

As for naming, one idea is to explore disambiguating the term "target" in cargo. If we rename "build target", then the --all-<something> could just have the new meaning.

We lean towards a new flag because people likely have broken doctests (as evidenced by this thread). While they would want to discover and fix them, they likely want to do it on their own timetable and not be forced to by cargo.

[joshtriplett]

Note that in a future edition we could potentially make --all-targets default to this, which would make this an interim option for current editions.

[djc]

Hmm, I wonder if we can have a project-level switch (maybe in the .cargo/config) for this? cargo c --all-features --all-targets is already pretty verbose and I'm not sure making it more onerous to do the right thing is the best way forward. So for projects that are aware of this and want to eagerly fix the doctests breakage, it would be great if we can opt-in to making --all-targets do (arguably) the right thing without waiting until 2027.

[epage]

I think we're leaning towards renaming "build target" and having the new name for --all-targets include doctests. We would then not need to wait for a new edition.

[weihanglo]

Just note that there are designs needing to figure out, for the interaction between doctesting everything and a subset of doctest-able lib/example/bench/test/bin. See #6669 (comment) and the original comment #6671 (comment).

[thomaseizinger]

Hmm, I wonder if we can have a project-level switch (maybe in the .cargo/config) for this?

You could maybe solve that with an alias that "does the right thing" whatever that means for you?

[orium]

I think we're leaning towards renaming "build target" and having the new name for --all-targets include doctests. We would then not need to wait for a new edition.

@epage The idea would still be to default to the "--all-targets includes docs" in edition 2024, is that right? The new edition is just a few months away, so I would be happy just waiting for that instead of having an --all-targets-with-doc.

[epage]

It is too late to introduce 2024 Edition changes. If a solution needs an Edition, it'd be 2027.

[orium]

Ah, well, it that case it makes sense to have a new flag for "all targets and docs" 😄 .