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

rustdoc: rename issue-\d+.rs tests to have meaningful names (part 11) #136258

Merged
merged 4 commits into from
Jan 31, 2025
Merged

rustdoc: rename issue-\d+.rs tests to have meaningful names (part 11) #136258

merged 4 commits into from
Jan 31, 2025

Conversation

notriddle
Copy link
Contributor

Follow up

et al

As always, it's easier to review the commits one at a time. Don't use the Files Changed tab. It's confusing.

@rustbot
Copy link
Collaborator

rustbot commented Jan 29, 2025

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jan 29, 2025
@notriddle notriddle changed the title rustdoc: rename issue-\d+.rs tests to have meaningful names (part 10) rustdoc: rename issue-\d+.rs tests to have meaningful names (part 11) Jan 29, 2025
fmease
fmease approved these changes Jan 30, 2025
View reviewed changes
tests/rustdoc-ui/intra-doc/ice-103463.rs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Uh yeah, this is quite a specific regression test. Any more descriptive file name probably doesn't do it justice (like priv-use).

tests/rustdoc-ui/intra-doc/ice-104145.rs Outdated
Copy link
Member

@fmease fmease Jan 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I admit that all of these ICE regression tests are super niche and that likely nobody will ever look at them again but I think it's worth spending some words on these ^^'. What about sth. like extern-trait-local-impl or non-local-trait-local-impl (with or without the ice- prefix)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to have the ice prefix for test cases that have no assertions. It makes it clearer what's being tested.

tests/rustdoc/inline_cross/doc-hidden-module-impl-31948-1.rs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

doc-hidden-* is only half the truth, this also tests that trait impls of effectively private types aren't rendered. Maybe doc-hidden-or-priv-module-* or the like?

tests/rustdoc/inline_cross/doc-hidden-module-impl-31948.rs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The commit cfad7ad that created these files describes it as a "reachability" check. How about calling it "doc-reachability-impl"?

tests/rustdoc/inline_cross/doc-hidden-module-impl-31948-2.rs Outdated
Copy link
Member

@fmease fmease Jan 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This one is accurate. Well, except that the test also checks for the absence of impl Bar for Wobble (Bar relates to the effectively private test cases) which I think doesn't make that much sense as a test case but yea.

tests/rustdoc/intra-doc/link-same-name-different-disambiguator-108459.rs
Copy link
Member

Choose a reason for hiding this comment

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

(hah, [#]108459 isn't even an issue but a PR)

@fmease
Copy link
Member

fmease commented Jan 30, 2025

Thanks! r=me with nits addressed
@bors rollup

@fmease fmease 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 Jan 30, 2025
@notriddle
Copy link
Contributor Author

@bors r=fmease

@bors
Copy link
Contributor

bors commented Jan 30, 2025

📌 Commit c17d568 has been approved by fmease

It is now in the queue for this repository.

@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 Jan 30, 2025
@jhpratt jhpratt mentioned this pull request Jan 31, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 31, 2025
@bors
Auto merge of rust-lang#136331 - jhpratt:rollup-curo1f4, r=jhpratt
25a1657 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#135414 (Stabilize `const_black_box`)
 - rust-lang#136150 (ci: use windows 2025 for i686-mingw)
 - rust-lang#136258 (rustdoc: rename `issue-\d+.rs` tests to have meaningful names (part 11))
 - rust-lang#136270 (Remove `NamedVarMap`.)
 - rust-lang#136278 (add constraint graph to polonius MIR dump)
 - rust-lang#136287 (LLVM changed the nocapture attribute to captures(none))
 - rust-lang#136291 (some test suite cleanups)
 - rust-lang#136296 (float::min/max: mention the non-determinism around signed 0)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1a5ebc9 into rust-lang:master Jan 31, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 31, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 31, 2025
@rust-timer
Unrolled build for rust-lang#136258
c6383c2 
Rollup merge of rust-lang#136258 - notriddle:notriddle/issue-d, r=fmease

rustdoc: rename `issue-\d+.rs` tests to have meaningful names (part 11)

Follow up

* rust-lang#134053
* rust-lang#130287

et al

As always, it's easier to review the commits one at a time. Don't use the Files Changed tab. It's confusing.
@notriddle notriddle deleted the notriddle/issue-d branch January 31, 2025 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmease fmease fmease approved these changes

Assignees

@fmease fmease

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.86.0
Development

Successfully merging this pull request may close these issues.
4 participants
@notriddle @rustbot @fmease @bors