Tidy check for test revisions that are mentioned but not declared #124706
Conversation
rustbot
added
A-testsuite
Zalathar
force-pushed
the
revision-checker
branch
2 times, most recently
from
1ec1f55 to
c341740
Compare
|
It would be nice to have a variant of this for Filecheck directives as well, but that's probably trickier because they're more freeform and thus harder to identify reliably. |
|
☔ The latest upstream changes (presumably #124345) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this comment.
I feel like the long-term solution to revision-related funny business is to rework how compiletest directive collection, parsing and validation is done. This would include adding validation for revisions that are used but not declared (i.e. the problem this PR is trying to solve), duplicate, invalid (incl. invalid characters and such), etc.
I think having a tidy check in the meantime (before compiletest directive rework, which I plan to start work on towards the end of this month) is reasonable though. It just feels like it should be the job of compiletest rather than a tidy check.
|
I'll roll a bootstrap reviewer as well because this mostly concerns tidy which falls under the purview of bootstrap. I think this PR is definitely rightfully addressing a "wtf moment" when writing and reading tests, I'm just not sure if it should be tidy's responsibility. r? bootstrap |
|
Yeah, my original implementation was integrated into compiletest, but it felt like I was adding more mess that would make future cleanups even trickier. So I decided to try a more self-contained tidy-based version instead. So I’m fine with this being seen as a relatively temporary solution, to eventually be replaced with better checks inside compiletest itself. |
|
Also see more silliness for compiletest directive/revision handling: |
|
r? jieyouxu -- tidy seems like a reasonable place |
Zalathar
force-pushed
the
revision-checker
branch
2 times, most recently
from
436b20f to
f8b392b
Compare
There was a problem hiding this comment.
Should we leave a FIXME here (feel free to FIXME(jieyouxu)) to remind people (well, mostly me) that ideally we would have these checks as part of compiletest directive and revision handling?
|
Feel free to r=me with or without the FIXME. @bors delegate=Zalathar |
If a `[revision]` name appears in a test header directive or error annotation, but isn't declared in the `//@ revisions:` header, that is almost always a mistake. In cases where a revision needs to be temporarily disabled, adding it to an `//@ unused-revision-names:` header will suppress these checks for that name. Adding the wildcard name `*` to the unused list will suppress these checks for the entire file.
Most of these changes either add revision names that were apparently missing, or explicitly mark a revision name as currently unused.
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (8c7c151): comparison URL. Overall result: ❌✅ regressions and improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 673.446s -> 674.785s (0.20%) |
If a
[revision]name appears in a test header directive or error annotation, but isn't declared in the//@ revisions:header, that is almost always a mistake.In cases where a revision needs to be temporarily disabled, adding it to an
//@ unused-revision-names:header will suppress these checks for that name.Adding the wildcard name
*to the unused list will suppress these checks for the entire file.(None of the tests actually use
*; it's just there because it was easy to add and could be handy as an escape hatch when dealing with other problems.)Most of the existing problems discovered by this check were fairly straightforward to fix (or ignore); the trickiest cases are in
borrowcktests.