Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include doc tests in cargo test --all-targets #6671

Closed
wants to merge 1 commit into from

Conversation

dwijnand
Copy link
Member

Fixes #6669

@rust-highfive
Copy link

r? @Eh2406

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

@ehuss
Copy link
Contributor

ehuss commented Feb 15, 2019

I'm not necessarily opposed to this, but I think we should be careful with how to move forward with this. The issue is that --doc is more of a mode than a target. There is some desire to support the ability to doctest other targets, but it is not clear how the UI for cargo test would support that. I haven't been able to think of any good ideas for how that might work. For example, --doc --all-targets could mean doctest all doctestable targets, and do nothing else. That would make this change proposed here a little confusing.

I'd like to see a plan for how cargo test should handle doc tests long term before making changes to how it works.

@Eh2406
Copy link
Contributor

Eh2406 commented Feb 15, 2019

I was just going to cc you, and you have opinions. so:
@bors: r? @ehuss

@rust-highfive rust-highfive assigned ehuss and unassigned Eh2406 Feb 15, 2019
@dwijnand
Copy link
Member Author

I'd like to see a plan for how cargo test should handle doc tests long term before making changes to how it works.

I think that's heavy-handed. The fact that doc-testing and testing are different "modes" is (IMHO) implementation detail of cargo/rustc. Do you agree cargo test --all-targets should run doctests (aka #6669 is a valid bug)? If so, do you think this implementation is valid?

@ehuss
Copy link
Contributor

ehuss commented Feb 15, 2019

Do you agree cargo test --all-targets should run doctests (aka #6669 is a valid bug)?

I don't currently agree or disagree. Running doctests is different enough that I think they should be treated separately. For example, should --bin foo do both tests and doctests? I would say "no". Same with --lib. So why would --all-targets be special? What about other plurality flags like --bins or --examples? Why would --all-targets be the only way to do both tests and doctests?

I'll just dump some ideas here, but like I said I'm not happy with them. I'd just like to see a cohesive model for how it works without too many special cases.

Method A: --doc is a mode flag.

  • --doc will doctest all doctestable targets.
  • --doc <target flags> will only doctest the given targets.
  • --doc --all-targets would only doctest all targets.
  • --all-targets would 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 --mode means rustc test only.

Drawbacks is that this adds another flag, which is very undesirable.

Method C: --doc could take targets as a parameter

  • --doc --lib doctests all doctestable targets, and rustc tests just the library.
  • --doc=lib doctests just the lib.
  • --doc=lib,example=foo,bins doctests lib, example foo, and all bins.
  • --doc=all-targets --all-targets does everything

Drawbacks is that I think the first case is confusing, and the last one is awkward.

If you have any ideas how it should work long-term, I'd like to hear them (or if any of these don't sound horrible). I'm not completely opposed to making --all-targets special, but special cases can be harder to explain, internalize, and remember. I'd also like to avoid making --all-targets special now if it conflicts with how it eventually should behave.

FWIW, from a use-case perspective, the "I want to test everything" use case would likely be served well by this change. However, I worry that if we enable doctesting for bins or examples, that --all-targets could suddenly break things for code that wasn't tested before. That might not be such a bad thing, though.

@dwijnand
Copy link
Member Author

How about --doc should filters down from "all kinds of tests" to doc tests, for the given targets. We'd then need another one for "just libtest tests". Though I'm not sure what the --all-targets flag does (i.e what's the point of including it).

@ehuss
Copy link
Contributor

ehuss commented Feb 16, 2019

Though I'm not sure what the --all-targets flag does (i.e what's the point of including it).

It's a relatively obscure case right now. It adds running unit tests of examples (instead of just compiling them) and builds and runs benchmarks once. Both can be added to the defaults by setting test=true in Cargo.toml. Benchmarks would require nightly if you are using the default harness.

@dwijnand
Copy link
Member Author

Well #6669 is tagged for Command-test, so we can consider it when we fix the multiple test-related issues cargo has.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants