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

Ignored doctests are run with cargo test -- --ignored. #87586

Open
thomcc opened this issue Jul 29, 2021 · 8 comments
Open

Ignored doctests are run with cargo test -- --ignored. #87586

thomcc opened this issue Jul 29, 2021 · 8 comments
Labels
A-doctests Area: Documentation tests, run by rustdoc T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@thomcc
Copy link
Member

thomcc commented Jul 29, 2021

If you use rust,ignore to ignore doctests which don't compile (often because you want to include example code that's entirely expository in your documentation — something that doesn't compile at all, and should merely syntax highlight as Rust), they still are attempted to compile and run when you pass --ignored to the test runner.

This is unfortunate, as this is what you use to run #[ignore]d tests, which are good (intended?) for tests that are too slow to complete in normal execution, often ones which are exhaustive. Having doctests that fail to compile be mixed in here means this use of #[ignore] is effectively broken.

The workaround I know for this is rather gross, e.g. using a const _: &str = stringify!{ /* code that doesn't compile here */ } wrapper (for example — even if using ignore wouldn't have been an alternative here since it's large enough that you'd rather ensure it compiles in practice, this is the pattern I'm referring to).

I think it's honestly a bug that this is the effect of rust,ignore, and think that instead it should be to just not emit the test.

(This is related to, but distinct from #30032)

I bugged @jyn514 about this once and he indicated he thought it might require an RFC to fix, but when I was complaining about it elsewhere recently, someone pointed out that there was no bug on file for it already.

@jyn514
Copy link
Member

jyn514 commented Jul 29, 2021

This is also very similar to #82715.

@jyn514 jyn514 added A-doctests Area: Documentation tests, run by rustdoc T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jul 29, 2021
@ocstl
Copy link

ocstl commented Nov 13, 2021

It also seems similar to #63193.

@CAD97
Copy link
Contributor

CAD97 commented Apr 30, 2022

I think ```rust,ignore should behave identically to #[ignore]; that is, they should actually be checked to compile, but not run unless you pass --ignored. That this isn't checked to compile I personally think is a bug, though one I don't think we can fix.

My ideal state of the world:

doc codeblock tag test attribute compile run
ignore ignore yes1 with --ignored
no_run no_run2 yes no
compile_fail fail
no_compile3 cfg(any()) no

Given current usage of ```ignore, though, I think the best we can reasonably ask for is something along the lines of

doc codeblock tag test attribute compile run
maybe_ignore4 ignore yes with --ignored
no_run no_run yes no
compile_fail n/a fail
ignore cfg(any(()) no

Unless maybe this can be cleaned up over an edition change?

Footnotes

  1. currently doctests only compile check with --ignored

  2. polyfill is #[cfg(test)] const MY_TEST: fn() = || { .. } instead of #[test] fn my_test() { .. }

  3. polyfill is #[cfg_attr(doctest, doc = " ````text"]

  4. modulo bikeshed: picking a name here is hard since it can't be #[ignore]

@CAD97
Copy link
Contributor

CAD97 commented Oct 7, 2022

As of some point between cargo 1.65.0-beta.2 (082503982 2022-09-13) and cargo 1.66.0-nightly (0b84a35c2 2022-10-03), the behavior here has changed, and cargo test -- --include-ignored no longer runs documentation tests.

example pwsh session
PS D:\git\cad97\playground> cargo +beta test -- --include-ignored > $null
    Finished test [unoptimized + debuginfo] target(s) in 0.00s
     Running unittests src\lib.rs (D:\.rust\target\debug\deps\playground-917c13ed19ceedcc.exe)
     Running unittests src\main.rs (D:\.rust\target\debug\deps\playground-3a8e8a709e4658bf.exe)
   Doc-tests playground
PS D:\git\cad97\playground> cargo +nightly test -- --include-ignored > $null
   Compiling playground v0.0.0 (D:\git\cad97\playground)
    Finished test [unoptimized + debuginfo] target(s) in 0.22s
     Running unittests src\lib.rs (D:\.rust\target\debug\deps\playground-f1421a85aae244d2.exe)
     Running unittests src\main.rs (D:\.rust\target\debug\deps\playground-cc3e84b5b419fbbd.exe)

Now in order to run ```ignore documentation tests, you have to use cargo test --doc -- --ignored. (Even --all-targets doesn't run documentation tests -- this surprised me, at least.)

It looks like this may have been a bugfix

cc @epage -- the only change in cargo's git history I saw in a quick skim that looked like it could've changed this behavior is the clap4 upgrade changing how -- behaves; see this comment for my suspicions. I'm now fairly certain that clap4's changes to trailing varargs are the culprit here -- though this is I think mostly a beneficial change.

@CAD97
Copy link
Contributor

CAD97 commented Oct 7, 2022

To be clear, this change makes the nightly state

compile run
```ignore with test --doc -- --ignored with test --doc -- --ignored
#[ignore] with cfg(test) build with test -- --ignored

Personally I think this is a completely reasonable state and nothing needs to be added.

@RalfJung
Copy link
Member

RalfJung commented Oct 7, 2022

I would say running ignored (doc)tests when --ignored is set is exactly expected behavior. After all you asked for ignored tests to be executed. Not running them would be a bug.

cargo test --tests -- --ignored can be used to run ignored tests but not doctests.

@schneems
Copy link
Contributor

schneems commented Feb 2, 2023

I wanted to leave a comment in support of the nightly change.

My main use case of wanting this addressed is writing mixed markdown files (like README.md) where I want to include non-rust-code (CLI usage with output, for example) but also make sure that the example code I DO provide is tested via my doc tests (using the include_str trick).

I need a way to tell rust "hey, seriously, honestly this really isn't something you should even consider running." For right now the only way to do that is via ignore.

Now in order to run ```ignore documentation tests, you have to use cargo test --doc -- --ignored.

For my use case of testing my READMEs this change would work well. I'm relatively new to the ecosystem. What's the path off of nightly and onto stable look like?

@thomcc
Copy link
Member Author

thomcc commented Jan 6, 2024

I would say running ignored (doc)tests when --ignored is set is exactly expected behavior. After all you asked for ignored tests to be executed. Not running them would be a bug.

In my experience, nearly 100% of the time ```ignore is used on doctests it's because the API is not public, or because it's exposition-only code. I've never seen it used for the reasons you'd use #[ignore] on tests (they're too slow to complete, require other setup, etc).

Even if we decide that --ignored should run ```ignored doctests (I concede that it's hard to argue against consistency here), we still should have no_compile, since otherwise there's no way to add an ignored doctest (that wants to use ignore for one of the above reasons) to a project that is using ```ignore to have non-compiled code in it's doctests.

That said, if the status quo on nightly is to require test --doc -- --ignored to run ignored doctests, that is also fine by me.

cargo test --tests -- --ignored can be used to run ignored tests but not doctests.

Thanks, this is useful, I hadn't thought of it.

tedil added a commit to varfish-org/hgvs-rs that referenced this issue Jun 10, 2024
* fix: use array instead of Vec for Codon representation

* don't allocate until necessary

* reduce usage of lazy_static, use consts instead where possible, copy instead of reference lookup tables

* cache calls to alignment::Mapper::new

* rely on seqrepo-rs with cloneable Error

* fmt

* implement data_version and schema_version for mock Provider in tests

* update seqrepo to v0.10.2

* shorter Arc::new error wrapping

* clippy lints

* clippy lints

* do not run doctests for sequence.rs generators

* workaround ignore in doctest by using stringify, see rust-lang/rust/issues/87586
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-doctests Area: Documentation tests, run by rustdoc T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
Status: No status
Development

No branches or pull requests

6 participants