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

Deny warnings in rustdoc non-UI tests #91275

Merged
merged 4 commits into from
Dec 1, 2021
Merged

Deny warnings in rustdoc non-UI tests #91275

merged 4 commits into from
Dec 1, 2021

Conversation

camelid
Copy link
Member

@camelid camelid commented Nov 26, 2021 •
edited
Loading

These warnings were silently ignored since they did not appear in a
.stderr file and did not fail the test. With this change, warnings in
tests are denied, causing the tests to fail if they have warnings.

This change has already led me to find a bug in rustdoc (#91274) and a
useless test (src/test/rustdoc/primitive/primitive-generic-impl.rs,
though its uselessness is unrelated to its warnings).

r? @jyn514

@camelid
Deny warnings in rustdoc non-UI tests
5fcae0a 
These warnings were silently ignored since they did not appear in a
`.stderr` file and did not fail the test. With this change, warnings in
tests are denied, causing the tests to fail if they have warnings.

I will fix all the warnings that are now test failures next.
@camelid camelid added A-testsuite Area: The testsuite used to check the correctness of rustc T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Nov 26, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 26, 2021
@camelid
Copy link
Member Author

camelid commented Nov 26, 2021

If there are any places where you are unclear what warning is being fixed, just ask and I will try to explain :)

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
camelid
camelid commented Nov 26, 2021
View reviewed changes
src/test/rustdoc/primitive/primitive-generic-impl.rs Outdated
Comment on lines 1 to 6
#![feature(rustdoc_internals)]

// FIXME: this test doesn't test anything

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this file is included as part of another test, so the feature gate is invalid and the note is incorrect. But this confusing include! behavior is removed in a later commit.

jyn514
jyn514 reviewed Nov 27, 2021
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

This is awesome, thank you for fixing it!

src/test/rustdoc/const-generics/const-impl.rs Show resolved Hide resolved
src/test/rustdoc/issue-43869.rs Show resolved Hide resolved
@jyn514 jyn514 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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 27, 2021
@jyn514
Copy link
Member

jyn514 commented Nov 27, 2021

Looks good once you fix the one failing test :)

@camelid
Copy link
Member Author

camelid commented Nov 27, 2021

@bors r=jyn514 rollup=iffy (because it may conflict with PRs adding or changing tests)

@bors
Copy link
Contributor

bors commented Nov 27, 2021

📌 Commit cdba47703e9be6161a09d803c8b7dd10d2f17bf2 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 27, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Nov 27, 2021

⌛ Testing commit cdba47703e9be6161a09d803c8b7dd10d2f17bf2 with merge ee203ec0d150905a211e75f74d85ef360ce54d72...

@camelid
Copy link
Member Author

camelid commented Nov 27, 2021

GitHub Actions incident appears to have caused problems for the build. @bors retry

@bors
Copy link
Contributor

bors commented Nov 28, 2021

⌛ Testing commit cdba47703e9be6161a09d803c8b7dd10d2f17bf2 with merge 259dce145ffdb4f8b6a6157c4cfa1ffaa6a42e0a...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Nov 28, 2021

💔 Test failed - checks-actions

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

camelid commented Nov 28, 2021

Added #![allow(rustdoc::broken_intra_doc_links)] to a test that was intentionally using broken links.

@bors r=jyn514

@camelid
Copy link
Member Author

camelid commented Nov 28, 2021

@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 Nov 28, 2021
@camelid
Fix warnings in rustdoc HTML tests
ee58c06 
Now that compiletest denies warnings in these tests, they need fixing!
@camelid
Remove unnecessary use of include! in rustdoc test
959dc77 
Using `include!` shouldn't affect the test. It was only added because:

> I replicated how it was performed in libstd. Since it's the main
> target of this fix, I thought it was the best way.

<https://github.com/rust-lang/rust/pull/52827/files#r207647331>

But it's unnecessary and adds unnecessary indirection.
@camelid
Fix warnings in rustdoc JS tests
ac88bb7
@camelid
Copy link
Member Author

camelid commented Nov 28, 2021

Updated line numbers in stderr file.

@bors r=jyn514

@bors
Copy link
Contributor

bors commented Nov 28, 2021

📌 Commit ac88bb7 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 28, 2021
@bors
Copy link
Contributor

bors commented Nov 30, 2021

⌛ Testing commit ac88bb7 with merge 2c07507b68c29937d70ee423f6766369fd3324a0...

@bors bors mentioned this pull request Nov 30, 2021
Merged
@bors
Copy link
Contributor

bors commented Nov 30, 2021

💥 Test timed out

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

jyn514 commented Nov 30, 2021

@bors retry

@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 Nov 30, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Dec 1, 2021

⌛ Testing commit ac88bb7 with merge 3c51718...

@bors
Copy link
Contributor

bors commented Dec 1, 2021

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing 3c51718 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 1, 2021
@bors bors merged commit 3c51718 into rust-lang:master Dec 1, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 1, 2021
@bors bors mentioned this pull request Dec 1, 2021
Closed
7 tasks
@camelid camelid deleted the deny-test-warnings branch December 1, 2021 21:03
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3c51718): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@matthiaskrgr matthiaskrgr mentioned this pull request Aug 17, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 left review comments

Assignees

@jyn514 jyn514

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. 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.59.0
Development

Successfully merging this pull request may close these issues.
7 participants
@camelid @rust-log-analyzer @jyn514 @bors @rust-timer @rust-highfive @rustbot