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

Add tidy check to ensure rustdoc ui tests parity #87845

Closed

Conversation

GuillaumeGomez
Copy link
Member

In #87728, I discovered that it wasn't known that tests present in src/test/ui/rustdoc should also be present in src/test/rustdoc-ui (because we need to ensure that the rustdoc attributes and errors are thrown by both rustc and rustdoc).

cc @jyn514 @camelid
r? @Mark-Simulacrum

@GuillaumeGomez GuillaumeGomez added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Aug 7, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 7, 2021
@rust-log-analyzer

This comment has been minimized.

@Mark-Simulacrum
Copy link
Member

Hm, this seems like an odd fix - if we expect that one of these directories is a subset of the other, I would expect us to not have two directories but instead annotate files with the compiletest revision support or a new dedicated feature to this effect. The current check just checks for same file name in both directories, which seems quite error prone - either file can get edited easily.

I think either we should move to revisions or a similar style feature. But I'm not sure the premise of this is quite right. If the code to validate attributes is in rustc's attribute validation, then rustdoc simply must run that - I don't think in that case we need to duplicate this for all tests. We don't rerun regular UI tests with rustdoc to make sure it's not using a different rustc parser or something; this seems like a similar case.

@GuillaumeGomez
Copy link
Member Author

Yes, it's not great as is. For context: for a long time, rustdoc hid most attributes checks, preventing the warnings from rustc to be deniable in rustdoc. The idea was then to ensure this behaviour doesn't happen again.

@jyn514
Copy link
Member

jyn514 commented Aug 9, 2021

Yes, it's not great as is. For context: for a long time, rustdoc hid most attributes checks, preventing the warnings from rustc to be deniable in rustdoc. The idea was then to ensure this behaviour doesn't happen again.

This can happen anyway though: because of #73566, the lints aren't ever emitted in the first place. This misleads people working on rustdoc because they could think it's a bug on their end that the lint shows up in rustc and not rustdoc, when in fact that's intentional. The tests only pass now because we happen to only test lints that run in rustdoc.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Aug 15, 2021
@Mark-Simulacrum
Copy link
Member

I think it makes sense for T-rustdoc to decide whether/how this check should look.

@jyn514
Copy link
Member

jyn514 commented Aug 15, 2021

Like I said above, I don't think we should add this check.

@GuillaumeGomez
Copy link
Member Author

Closing then.

@GuillaumeGomez GuillaumeGomez deleted the ui-rustdoc-test-parity branch August 16, 2021 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). 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.

None yet

5 participants