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

Set cfg(test) when rustdoc is running with --test option #59940

Merged
merged 2 commits into from
Apr 26, 2019

Conversation

GuillaumeGomez
Copy link
Member

Following a discussion on twitter, I proposed this change. What do you think about it?

r? @QuietMisdreavus

cc @BurntSushi

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 13, 2019
@jonas-schievink jonas-schievink added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Apr 13, 2019
@BurntSushi
Copy link
Member

I'm not sure what the rustdoc team procedures are here, but you might want to consider this as a normal public API addition. That is, if you accept this change and it makes it to stable Rust, then removing this would be a breaking change AIUI. For example, one thing I think this would allow you to do is to write doc tests using code that is gated behind a cfg(test).

@ehuss
Copy link
Contributor

ehuss commented Apr 13, 2019

There is some discussion about this in #45599.

@QuietMisdreavus
Copy link
Member

(Sorry about closing this, i misclicked the button when typing my initial comment. 😓)

This looks like it's not the same as #45599 - this is about setting cfg(test) when collecting doctests, not when compiling them. This is probably fine, since there's been a slight pattern of having non-public doctests to test when something should not compile, or (in the case of the linked twitter thread) to check the doctests of the README without directly including it in the crate's docs. I originally learned about this when trying to block doctests on private items (#54438). It looks like this is a way to properly support that.

@ehuss
Copy link
Contributor

ehuss commented Apr 13, 2019

Ah, sorry for misinterpreting.

@GuillaumeGomez
Copy link
Member Author

So is it ok to merge it?

@QuietMisdreavus QuietMisdreavus added the relnotes Marks issues that should be documented in the release notes of the next release. label Apr 24, 2019
@QuietMisdreavus QuietMisdreavus changed the title Set test flag when rustdoc is running with --test option Set cfg(test) when rustdoc is running with --test option Apr 24, 2019
Copy link
Member

@QuietMisdreavus QuietMisdreavus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine to me, though i'm a little worried that there will be some subtle ramifications that we haven't thought about. I'd like someone from @rust-lang/docs or @rust-lang/rustdoc to see whether i'm overthinking this, but in the meantime there doesn't seem to be much harm from landing it into nightly for now - we have plenty of time to revert it if things go awry.

@QuietMisdreavus
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 24, 2019

📌 Commit 1ce17e6392a0bcb4d99cd2e8cd6249d015f43e6d has been approved by QuietMisdreavus

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 24, 2019
@ollie27
Copy link
Member

ollie27 commented Apr 24, 2019

I'm a bit worried that doctests on items marked #[cfg(not(test))] will no longer be run but I don't know how common that would be in the wild.

Also can we add a rustdoc-ui test like the following to test this behavior?

// compile-pass
// compile-flags:--test
// normalize-stdout-test: "src/test/rustdoc-ui" -> "$$DIR"

/// this doctest will be ignored:
///
/// ```
/// assert!(false);
/// ```
#[cfg(not(test))]
pub struct Foo;

/// this doctest will be tested:
///
/// ```
/// assert!(true);
/// ```
#[cfg(test)]
pub struct Foo;

@GuillaumeGomez
Copy link
Member Author

@ollie27: Hum good point, they won't be tested indeed. I don't see it as an issue since there are workarounds though. I'll add the test shortly.

@bors: r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 24, 2019
@GuillaumeGomez
Copy link
Member Author

Updated!

@GuillaumeGomez
Copy link
Member Author

r? @ollie27 (since @QuietMisdreavus was ok with the changes, I guess it's up to you to validate the tests :p)

@ollie27
Copy link
Member

ollie27 commented Apr 25, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Apr 25, 2019

📌 Commit 459d677 has been approved by ollie27

@Centril
Copy link
Contributor

Centril commented Apr 25, 2019

@bors retry

yielding to r0llup

@bors
Copy link
Contributor

bors commented Apr 26, 2019

⌛ Testing commit 459d677 with merge 3ae1afdb0a6df45f29f9829968419ff7fde0cc3a...

Centril added a commit to Centril/rust that referenced this pull request Apr 26, 2019
Set cfg(test) when rustdoc is running with --test option

Following a [discussion on twitter](https://twitter.com/burntsushi5/status/1117091914199785473), I proposed this change. What do you think about it?

r? @QuietMisdreavus

cc @BurntSushi
@Centril
Copy link
Contributor

Centril commented Apr 26, 2019

@bors retry

@Centril Centril added this to the 1.36 milestone Apr 26, 2019
bors added a commit that referenced this pull request Apr 26, 2019
Rollup of 12 pull requests

Successful merges:

 - #59734 (Prevent failure in case no space left on device in rustdoc)
 - #59940 (Set cfg(test) when rustdoc is running with --test option)
 - #60134 (Fix index-page generation)
 - #60165 (Add Pin::{into_inner,into_inner_unchecked})
 - #60183 (Chalkify: Add builtin Copy/Clone)
 - #60225 (Introduce hir::ExprKind::Use and employ in for loop desugaring.)
 - #60247 (Implement Debug for Place using Place::iterate)
 - #60259 (Derive Default instead of new in applicable lint)
 - #60267 (Add feature-gate for f16c target feature)
 - #60284 (Do not allow const generics to depend on type parameters)
 - #60285 (Do not ICE when checking types against foreign fn)
 - #60289 (Make `-Z allow-features` work for stdlib features)

Failed merges:

r? @ghost
@bors
Copy link
Contributor

bors commented Apr 26, 2019

⌛ Testing commit 459d677 with merge 180edc2...

@bors bors merged commit 459d677 into rust-lang:master Apr 26, 2019
@GuillaumeGomez GuillaumeGomez deleted the rustdoc-test branch April 26, 2019 08:04
dvdhrm added a commit to r-efi/r-efi-string that referenced this pull request May 6, 2019
The rustdoc program now sets `cfg(test)` if run with `--test`. Hence,
the r-efi-string crate will now compile properly under all
circumstances. Therefore, re-enable the doctests for better test
coverage. Note that this will be released with rust-1.36.

For details, see:

    rust-lang/rust#59940

Signed-off-by: David Rheinsberg <david.rheinsberg@gmail.com>
@ollie27
Copy link
Member

ollie27 commented May 25, 2019

I should have checked before merging this but there are doctests in this repository that are behind #[cfg(not(test))] which we haven't been testing since this was merged. For example the entire of core:

rust/src/libcore/lib.rs

Lines 50 to 51 in f492693

// This cfg won't affect doc tests.
#![cfg(not(test))]

I've submitted a revert: #61199

bors added a commit that referenced this pull request Jun 28, 2019
Revert "Set test flag when rustdoc is running with --test option"

Reverts #59940.

It caused doctests in this repository to no longer be tested including all of the core crate.
Centril added a commit to Centril/rust that referenced this pull request Jun 28, 2019
…sdreavus

Revert "Set test flag when rustdoc is running with --test option"

Reverts rust-lang#59940.

It caused doctests in this repository to no longer be tested including all of the core crate.
Centril added a commit to Centril/rust that referenced this pull request Jun 28, 2019
…sdreavus

Revert "Set test flag when rustdoc is running with --test option"

Reverts rust-lang#59940.

It caused doctests in this repository to no longer be tested including all of the core crate.
Centril added a commit to Centril/rust that referenced this pull request Jun 28, 2019
…sdreavus

Revert "Set test flag when rustdoc is running with --test option"

Reverts rust-lang#59940.

It caused doctests in this repository to no longer be tested including all of the core crate.
Centril added a commit to Centril/rust that referenced this pull request Jun 29, 2019
…sdreavus

Revert "Set test flag when rustdoc is running with --test option"

Reverts rust-lang#59940.

It caused doctests in this repository to no longer be tested including all of the core crate.
@abonander abonander mentioned this pull request Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants