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

tidy: Add a check for inline unit tests #62996

Merged
merged 3 commits into from
Jul 28, 2019
Merged

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Jul 26, 2019

As described in #61097.

There's a large whitelist right now, because in many crates the tests are not outlined yet.
This PR only outlines tests in one crate (rustc_lexer) as an example.

r? @Mark-Simulacrum

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 26, 2019
@petrochenkov
Copy link
Contributor Author

This also needs to check for #[bench] + false positives like " ... #[test] ... " in diagnostic strings need to be worked around somehow + liballoc seems to have the same restrictions as libcore so it would be nice to enforce them in tidy.

@Mark-Simulacrum
Copy link
Member

I think false-positives are presumably infrequent enough that we could work around them via // ignore-tidy- infrastructure (similar to length, etc.). Not sure how easy it is to thread that into a sub-pass of tidy, though.

Approach looks good to me. It might be nicer to an extent to use the same ignore-tidy- annotations for all files that we're ignoring instead of the custom skip list (at least outside of src/test, perhaps?) but that doesn't seem all that important.

@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. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 26, 2019
@petrochenkov
Copy link
Contributor Author

Updated.

  • #[bench] attributes are checked as well
  • restrictions on tests and benchmarks in liballoc are enforced (identical to libcore)
  • `#[test] and `#[bench] (with a back quote) are ignored during checking, this is a good heuristic for allowing the attributes in diagnostic messages, so I didn't need to add an additional ignore-tidy-... annotation.
  • the changes in librustc_lexer are removed for now to avoid possible conflicts.

It might be nicer to an extent to use the same ignore-tidy- annotations for all files that we're ignoring instead of the custom skip list (at least outside of src/test, perhaps?) but that doesn't seem all that important.

Yeah, this is all temporary anyway, once this PR lands, I'll prepare another big PR moving the code, so it doesn't matter much.

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 27, 2019
@Mark-Simulacrum
Copy link
Member

@bors r+

Looks good to me. I'm not entirely certain that the skip rules are correct, but since this is simply tidy we can correct that over time if we discover they're inaccurate.

@bors
Copy link
Contributor

bors commented Jul 27, 2019

📌 Commit aecaa03 has been approved by Mark-Simulacrum

@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 Jul 27, 2019
Centril added a commit to Centril/rust that referenced this pull request Jul 27, 2019
tidy: Add a check for inline unit tests

As described in rust-lang#61097.

There's a large whitelist right now, because in many crates the tests are not outlined yet.
~This PR only outlines tests in one crate (`rustc_lexer`) as an example.~

r? @Mark-Simulacrum
Centril added a commit to Centril/rust that referenced this pull request Jul 27, 2019
tidy: Add a check for inline unit tests

As described in rust-lang#61097.

There's a large whitelist right now, because in many crates the tests are not outlined yet.
~This PR only outlines tests in one crate (`rustc_lexer`) as an example.~

r? @Mark-Simulacrum
Centril added a commit to Centril/rust that referenced this pull request Jul 28, 2019
tidy: Add a check for inline unit tests

As described in rust-lang#61097.

There's a large whitelist right now, because in many crates the tests are not outlined yet.
~This PR only outlines tests in one crate (`rustc_lexer`) as an example.~

r? @Mark-Simulacrum
bors added a commit that referenced this pull request Jul 28, 2019
Rollup of 8 pull requests

Successful merges:

 - #61207 (Allow lifetime elision in `Pin<&(mut) Self>`)
 - #62074 (squash of all commits for nth_back on ChunksMut)
 - #62771 (Break dependencies between `syntax_ext` and other crates)
 - #62883 (Refactoring use common code between option, result and accum)
 - #62949 (Re-enable assertions in PPC dist builder)
 - #62996 (tidy: Add a check for inline unit tests)
 - #63038 (Make more informative error on outer attribute after inner)
 - #63050 (ci: download awscli from our mirror)

Failed merges:

r? @ghost
@bors bors merged commit aecaa03 into rust-lang:master Jul 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants