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

Clean up rustdoc tests by moving them into one place #76223

Closed

Conversation

GuillaumeGomez
Copy link
Member

Fixes #76035

The idea behind this move is to have tests in one place. It became very important for me because we started to have fixtures, making the code "more messy" than it should in my opinion. Like this, all tests are in one place and they're not mixed with the code anymore.

r? @jyn514

I think it'll also interest @ollie27 so pinging them!

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 1, 2020
@jyn514 jyn514 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 Sep 1, 2020
mod html_toc;
#[cfg(test)]
mod passes_unindent_comments;
#[cfg(test)]
Copy link
Member

Choose a reason for hiding this comment

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

Should we just cfg(test) the parent module instead of duplicating the cfgs?

@petrochenkov
Copy link
Contributor

The whole point of Rust unit tests is that they can be placed in modules which they are testing, preventing things that don't need to be public from being public.
No other rustc crate uses the proposed scheme.

@bors
Copy link
Contributor

bors commented Sep 3, 2020

☔ The latest upstream changes (presumably #73819) made this pull request unmergeable. Please resolve the merge conflicts.

@jyn514
Copy link
Member

jyn514 commented Sep 3, 2020

I agree with petrochenkov here ... this makes a lot of functions public just for testing.

It became very important for me because we started to have fixtures, making the code "more messy" than it should in my opinion.

Can you expand on this? What fixtures does rustdoc use for unit tests?

@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 Sep 3, 2020
@jyn514
Copy link
Member

jyn514 commented Sep 3, 2020

Also, you linked to the wrong issue in the PR description, I think you meant #76036.

@jyn514 jyn514 added the A-testsuite Area: The testsuite used to check the correctness of rustc label Sep 3, 2020
@jyn514 jyn514 added the I-needs-decision Issue: In need of a decision. label Sep 17, 2020
@jyn514
Copy link
Member

jyn514 commented Sep 17, 2020

I'm going to close this since it's not clear it's a useful change.

@jyn514 jyn514 closed this Sep 17, 2020
@jyn514 jyn514 mentioned this pull request Sep 17, 2020
@GuillaumeGomez GuillaumeGomez deleted the rustdoc-test-cleanup branch August 19, 2024 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-cleanup Category: PRs that clean code up or issues documenting cleanup. I-needs-decision Issue: In need of a decision. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

6 participants