cargo
cargo copied to clipboard
rust-lang
`cargo test --all-targets` does not run doc tests
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.
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.
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.
This means the command to run absolutely all tests everywhere is cargo test --workspace, not cargo test --workspace --all-targets, right?
This means the command to run absolutely all tests everywhere is
cargo test --workspace, notcargo 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).
Oh god, I think we've been not running doc tests on our CI for a couple of years due to this...
This just bit my projects — apparently we haven't been running doctests in CI for months.
I find it very confusing that a bare cargo test includes doctests and excludes examples, but then adding --all-targets adds examples while excluding doctests. While I find the non-additive behavior unintuitive, I don't claim to understand the nuances involved so perhaps it is my intuition that is flawed.
I'd like to propose amending the --help entry for --all-targets to explicitly mention the "doctests get explicitly excluded" caveat. Right now, that entry merely reads --all-targets Test all targets which I feel encourages users to use that flag as a footgun, just as I did.
If I were to create a PR to add the doctests caveat to the --help entry for --all-targets, is that something that the maintainers would consider merging?
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 😉 .
Perhaps that's something the testing team could tackle 😉 .
The whole situation around doctests is something I want to improve but have not dug into enough of the details to create a plan (yet).
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
Documentation is hard
- In Documentation tests, we say "Different from normal test targets", implying it is a target
- In Target selection, we talk abut doctests being a target
The --all-targets portion of Target selection also needs an edit. I attempted to do that in #12423, but the current structure of the docs makes it difficult to make a carve-out that adds the additional --all-targets clarification for cargo test only, without applying it to --all-targets for other cargo commands that have nothing to do with doctests.
I'd like to copy over some proposals from ehuss that was buried in a closed PR. Fundamentally this is more a UX issue than performance or technical issue.
Method A: --doc is a mode flag.
--docwill doctest all doctestable targets.--doc <target flags>will only doctest the given targets.--doc --all-targetswould only doctest all targets.--all-targetswould only rustc test all targets. (Or, maybe it could do both tests and doctests?)<target flags>will only rustc test the given targets.
Drawbacks here: I'm not sure if this is clear. If --all-targets only does rustc tests, then you would need two commands (cargo test --all-targets and cargo test --doc --all-targets) to do everything. If it does both, then it is a special case that people need to learn about. You wouldn't be able to say "test and doctest all examples", unless all plurality flags like --examples are special, in which case I think it would be confusing if --example foo didn't run doctests (but that seems undesirable to me).
Method B: --mode=... can specify the types of tests that are run.
--mode=test,doctest <target flags>will test and doctest the given targets.- No
--modemeans rustc test only.
Drawbacks is that this adds another flag, which is very undesirable.
Method C: --doc could take targets as a parameter
--doc --libdoctests all doctestable targets, and rustc tests just the library.--doc=libdoctests just the lib.--doc=lib,example=foo,binsdoctests lib, example foo, and all bins.--doc=all-targets --all-targetsdoes everything
Drawbacks is that I think the first case is confusing, and the last one is awkward.
Yeah it would be great if there was a single command to run all tests, doctests, everything.
having a unified command is a blocker for re-attempting #8437
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-targetsby 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--modeflag). 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.
Can we just remove the
--docflag 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.