Skip to content

Clean up rustdoc tests #76036

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

Closed
GuillaumeGomez opened this issue Aug 28, 2020 · 3 comments
Closed

Clean up rustdoc tests #76036

GuillaumeGomez opened this issue Aug 28, 2020 · 3 comments
Assignees
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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@GuillaumeGomez
Copy link
Member

A lot of rustdoc tests are in the middle of the code. I think we should centralize them in one place (like in librustdoc/test/) and then organize the tests in there directly (grouping them by what they actually test).

@GuillaumeGomez GuillaumeGomez added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Aug 28, 2020
@GuillaumeGomez GuillaumeGomez self-assigned this Aug 28, 2020
@petrochenkov
Copy link
Contributor

@GuillaumeGomez
What do you mean?
We have a tidy check that forces tests to be in separate files named tests.rs rather than in the middle of the code, so all rustdoc tests look like this:

src\librustdoc\clean\cfg\tests.rs
src\librustdoc\html\highlight\tests.rs
src\librustdoc\html\markdown\tests.rs
src\librustdoc\html\render\tests.rs
src\librustdoc\html\toc\tests.rs
src\librustdoc\passes\unindent_comments\tests.rs
src\librustdoc\test\tests.rs
src\librustdoc\theme\tests.rs

@GuillaumeGomez
Copy link
Member Author

I guess it's up to debate but this issue came up recently because a contributor (cc @matklad) needed to add a fixture file, making the whole very messy in my point of view. So what I had in mind was to move the files "out of tree" into something looking like this:

src/librustdoc/tests/clean.rs
src/librustdoc/tests/html_highlight.rs
src/librustdoc/tests/html_markdown.rs
src/librustdoc/tests/html_render.rs
src/librustdoc/tests/html_toc.rs
...

Or something like this:

src/librustdoc/tests/clean/cfg.rs
src/librustdoc/tests/html/highlight.rs
src/librustdoc/tests/html/markdown.rs
src/librustdoc/tests/html/render.rs
src/librustdoc/tests/html/toc.rs
...

(I have to admit that I prefer the second one.)

@jyn514
Copy link
Member

jyn514 commented Sep 17, 2020

I have the same comment as on #76223 - I don't think making things public just for testing is helpful or necessary. It's possible to have a tests/fixtures directory if you think that would be less 'messy', but I don't think the tests themselves should be moved.

@jyn514 jyn514 added A-testsuite Area: The testsuite used to check the correctness of rustc C-cleanup Category: PRs that clean code up or issues documenting cleanup. labels Sep 17, 2020
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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants