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

Move calls to test_main into one function #87372

Merged
merged 1 commit into from
Jul 23, 2021

Conversation

GuillaumeGomez
Copy link
Member

Fixes #86254.

cc @jyn514
r? @camelid

@GuillaumeGomez GuillaumeGomez added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jul 22, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 22, 2021
@jyn514
Copy link
Member

jyn514 commented Jul 22, 2021

@bors r+ rollup

@GuillaumeGomez is there a reason you cc me on all these PRs?

@bors
Copy link
Contributor

bors commented Jul 22, 2021

📌 Commit ef953b0 has been approved by jyn514

@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 Jul 22, 2021
@GuillaumeGomez
Copy link
Member Author

On this one it's simply because I know you like to keep an eye on rustdoc "backend" changes. But I can stop if the notifications annoy you, as you prefer. :)

@jyn514
Copy link
Member

jyn514 commented Jul 22, 2021

ahh, that makes sense :) no worries, the pings are fine

@camelid
Copy link
Member

camelid commented Jul 22, 2021

Thank you, this is definitely an improvement! It still seems surprising to me that there are two separate doctest-running functions (markdown::test and doctest::run), with significantly different contents. Why are they different, and should this PR still be marked as closing #86254?

@jyn514
Copy link
Member

jyn514 commented Jul 22, 2021

@camelid they're different entry points: doctest is used by rustdoc --test and markdown is used as a library by error_index_generator. I want to get rid of error index's dependence on rustdoc at some point so it should go away when that happens.

@camelid
Copy link
Member

camelid commented Jul 22, 2021

@camelid they're different entry points: doctest is used by rustdoc --test and markdown is used as a library by error_index_generator. I want to get rid of error index's dependence on rustdoc at some point so it should go away when that happens.

Hmm, Guillaume mentioned previously that markdown is also used by rustdoc foo.md (i.e., calling rustdoc on a Markdown file instead of a Rust file), so I'm not sure if we'd be able to get rid of it.

@Mark-Simulacrum
Copy link
Member

Both of them are invoked from main rustdoc (rustdoc supports rustdoc --test foo.md on the CLI); it seems like the difference is whether the input source text is Rust code (and so needs to be parsed & expanded to get at the doc tests) or Markdown (so needs to be parsed, but in a completely different way).

I don't think either could go away, but it does seem a little suspicious that this PR is not unifying the (seemingly overlooked?) support for unused externs in markdown tests. I at least would expect that to work just as well regardless of the input type...

@GuillaumeGomez
Copy link
Member Author

Just like @camelid said: one is run from rust code (we iterate through hir items and then check into their documentation if they have tests) whereas the second directly check from a markdown file (which is used by the error index as @jyn514 said). So we need to keep both, the goal here is to simply to merge the parts which can be merged.

@GuillaumeGomez
Copy link
Member Author

Why are they different, and should this PR still be marked as closing #86254?

And yes, this is the part of the code which I referred to when I opened this issue. ;)

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jul 23, 2021
…, r=jyn514

Move calls to test_main into one function

Fixes rust-lang#86254.

cc `@jyn514`
r? `@camelid`
This was referenced Jul 23, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 23, 2021
Rollup of 8 pull requests

Successful merges:

 - rust-lang#87034 (DOC: fix hypothetical Rust code in `step_by()` docstring)
 - rust-lang#87298 (memorialize Anna Harren in the bastion of the turbofish)
 - rust-lang#87332 (Don't hide fields of enum struct variants)
 - rust-lang#87362 (Make `x.py d` an alias for `x.py doc`)
 - rust-lang#87372 (Move calls to test_main into one function)
 - rust-lang#87373 (Extend HIR WF checking to fields)
 - rust-lang#87376 (Change rustdoc logo to use the full container size)
 - rust-lang#87383 (Add regression tests for the impl_trait_in_bindings ICEs)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 7c2436a into rust-lang:master Jul 23, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jul 23, 2021
@GuillaumeGomez GuillaumeGomez deleted the move-test_main-calls branch July 23, 2021 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. 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.

Deduplicate doc tests logic
7 participants