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.

Sign up for GitHub

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

Jump to bottom

Add --scrape-tests option to rustdoc to scrape functions marked #[test] #93497

Merged
merged 1 commit into from
Feb 19, 2022
Merged

Add --scrape-tests option to rustdoc to scrape functions marked #[test] #93497

merged 1 commit into from
Feb 19, 2022

Conversation

willcrichton
Copy link
Contributor

As a part of stabilizing the scrape examples extension in Cargo, I uncovered a bug where examples cannot be scraped from tests. See this test: https://github.com/rust-lang/cargo/pull/10343/files#diff-27aa4f012ebfebaaee61498d91d2370de460628405d136b05e77efe61e044679R2496

The issue is that when rustdoc is run on a test file, because --test is not passed as a rustc option, then functions annotated with #[test] are ignored by the compiler. So this PR changes rustdoc so when --test is passed in conjunction with a --scrape-example-<suffix> flag, then the test field of rustc_interface::Config is true.

r? @camelid

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jan 31, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 31, 2022
This was referenced Jan 31, 2022
Merged
Merged
@GuillaumeGomez
Copy link
Member

In my opinion it's not necessarily the "right" usage. In tests you use items in order to make sure they work under any circumstance. Not sure if it's good to be used as an example.

@willcrichton
Copy link
Contributor Author

In my opinion it's not necessarily the "right" usage. In tests you use items in order to make sure they work under any circumstance. Not sure if it's good to be used as an example.

Note that this PR makes it possible for Rustdoc to scrape examples from tests. The proposed default is still that only examples and library code are used. But this way if someone wants to include tests as examples, then they can. That decision is left to the library author.

@GuillaumeGomez
Copy link
Member

I stand on my position by if it's not enabled by default, then let's say I'm neutral.

@willcrichton willcrichton mentioned this pull request Feb 2, 2022
Open
4 tasks
@camelid
Copy link
Member

camelid commented Feb 3, 2022

I thought scrape-examples only works on examples/? Aren't #[test] functions usually in the crate itself?

@GuillaumeGomez
Copy link
Member

They can be inside the crate source code and in their own folder.

@camelid camelid assigned GuillaumeGomez and unassigned camelid Feb 3, 2022
@willcrichton
Copy link
Contributor Author

Examples can theoretically be scraped from any source file in your Cargo workspace. The default is just the examples/ directory.

@camelid
Copy link
Member

camelid commented Feb 3, 2022

Oh, so scrape-examples already supports scraping e.g. library source code if you tell it to?

@willcrichton
Copy link
Contributor Author

That's right. And rust-lang/cargo#10343 makes that configurable at the target level.

@willcrichton
Copy link
Contributor Author

If there are no other objections to this PR, it would be good to get it merged so I can get tests passing on rust-lang/cargo#10343.

@GuillaumeGomez
Copy link
Member

This is quite an extension so I'd prefer to have everyone on the team agreeing with it first.

cc @rust-lang/rustdoc

@notriddle
Copy link
Contributor

I support this PR. A crate might be intended for use in test cases, so it makes sense to have example code that uses #[test], and you ought to be able up scrape it.

If other words, I think being example code and being test code are orthogonal.

@GuillaumeGomez
Copy link
Member

I don't agree with your point of view but I agree on the fact that users should be able to include their tests into the "scraped examples". For me, tests are meant to check that your code works whereas examples are meant to show how to use your API.

@camelid
Copy link
Member

camelid commented Feb 9, 2022

Maybe this should be a different flag, e.g. --include-tests? --test is used to run tests, but AFAICT this PR makes it so it's also used when building docs? That seems wrong.

@willcrichton
Copy link
Contributor Author

willcrichton commented Feb 9, 2022 •
edited
Loading

This PR only affects the case where the user is scraping examples, not the general documentation code paths. But I can create a new flag if you like.

@camelid
Copy link
Member

camelid commented Feb 10, 2022

This PR only affects the case where the user is scraping examples, not the general documentation code paths. But I can create a new flag if you like.

Yeah, using the flag seems confusing to me.

@camelid
Copy link
Member

camelid commented Feb 10, 2022

Could you squash your commits btw?

@willcrichton
Copy link
Contributor Author

Done.

@camelid camelid changed the title Pass --test flag through rustdoc to rustc so #[test] functions can be scraped Pass --test flag through rustdoc to rustc so #[test] functions can be scraped Feb 11, 2022
@camelid
Copy link
Member

camelid commented Feb 11, 2022

Just noting that now that it's a new unstable flag, an FCP shouldn't be necessary.

@willcrichton
Copy link
Contributor Author

I just added some documentation for the flag as well.

@GuillaumeGomez
Copy link
Member

Looks good to me now, thanks!

@willcrichton
Copy link
Contributor Author

@GuillaumeGomez can you @bors (or should I do it?)

@GuillaumeGomez
Copy link
Member

I'm waiting for one last review from @camelid. If they don't see anything else, then let's r+. :)

@camelid
Copy link
Member

camelid commented Feb 17, 2022

As long as this PR only adds behavior behind a new unstable flag, I have no objections.

@GuillaumeGomez
Copy link
Member

Then let's go!

@bors: r+

@bors
Copy link
Contributor

bors commented Feb 18, 2022

📌 Commit fbbcb08 has been approved by GuillaumeGomez

@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 Feb 18, 2022
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 18, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 19, 2022
@bors
Auto merge of rust-lang#94134 - matthiaskrgr:rollup-b132kjz, r=matthi…
1882597 
…askrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#89892 (Suggest `impl Trait` return type when incorrectly using a generic return type)
 - rust-lang#91675 (Add MemTagSanitizer Support)
 - rust-lang#92806 (Add more information to `impl Trait` error)
 - rust-lang#93497 (Pass `--test` flag through rustdoc to rustc so `#[test]` functions can be scraped)
 - rust-lang#93814 (mips64-openwrt-linux-musl: correct soft-foat)
 - rust-lang#93847 (kmc-solid: Use the filesystem thread-safety wrapper)
 - rust-lang#93877 (asm: Allow the use of r8-r14 as clobbers on Thumb1)
 - rust-lang#93892 (Only mark projection as ambiguous if GAT substs are constrained)
 - rust-lang#93915 (Implement --check-cfg option (RFC 3013), take 2)
 - rust-lang#93953 (Add the `known-bug` test directive, use it, and do some cleanup)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e3a1e19 into rust-lang:master Feb 19, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 19, 2022
@camelid
Copy link
Member

camelid commented Feb 19, 2022 •
edited
Loading

Note

This PR's title is actually inaccurate since the contents changed after creation. It actually adds a new unstable flag and doesn't change the existing --test flag's behavior.

@GuillaumeGomez GuillaumeGomez changed the title Pass --test flag through rustdoc to rustc so #[test] functions can be scraped Add --scrape-tests option to rustdoc to scrape functions marked #[test] Feb 20, 2022
@GuillaumeGomez
Copy link
Member

Good point. I updated the title.

@huang12zheng huang12zheng mentioned this pull request Jul 14, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

Assignees

@GuillaumeGomez GuillaumeGomez

Labels
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
Milestone
1.60.0
Development

Successfully merging this pull request may close these issues.
7 participants
@willcrichton @GuillaumeGomez @camelid @notriddle @bors @rust-highfive @rustbot