Tidy on UI tests is annoying #75987

Activity

1. kornelski commented on Aug 27, 2020

   

   kornelski

   on Aug 27, 2020

   ContributorAuthor

   More actions

   Ok, I've found // ignore-tidy-line-length. But I think it should be the default for UI tests.
2. petrochenkov commented on Aug 27, 2020

   

   petrochenkov

   on Aug 27, 2020

   Contributor

   More actions

   I disagree that this is an issue, various tidy checks are still very much useful for test code.

   //~ ERROR annotations in *.rs files are not required to contain full messages and can be trimmed to their "essential part" if necessary, *.stderr files will contain full messages anyway.
   // ignore-tidy-line-length and other // ignore-* directives are tools of last resort and should only be used in exceptional cases.
3. kornelski

   closed this as completedon Aug 27, 2020
4. kornelski

   changed the title [-]tidy is being run on UI tests[/-] [+]Tidy on UI tests is annoying[/+] on Aug 28, 2020
5. kornelski

   reopened this on Aug 28, 2020
6. kornelski commented on Aug 28, 2020

   

   kornelski

   on Aug 28, 2020

   Last edited by kornelski

   ContributorAuthor

   More actions

   Tidy has been a nightmare.

   1. The build failed. Tidy didn't like line lengths that were exceeded by a few characters.
   2. So I've had to find and add the // ignore-tidy-line-length (if I truncated the message instead, it wouldn't be testing the word I added)
   3. The build failed, because the ignore lines have changed line numbers in the tests.
   4. I've had to rebuild and bless tests again (it's a slow process)
   5. The build failed. I've added one ignore too many, and it failed because there wasn't a long line any more.
   6. I've removed the ignore line, but replaced it with a blank one, because I didn't want the pain of re-blessing the build (I've rebased my branch, so it'd cost me a full compiler recompile)
   7. The build failed. Tidy didn't allow having a blank line.
   8. Now I'm removing that build-breaking line. It wasn't even a big whitespace gap. Just literally one blank line at the top of the file. The file looks fine with it.

   I just wanted to add one word to an error message. It's taking me hours of tinkering and recompiling and the PR is taking two days to just get past the tidy.

   My enthusiasm went from "I've found a poorly worded error, I can fix it!" to "screw the petty gatekeeping tool, I don't have time to fight this".
7. petrochenkov commented on Aug 28, 2020

   

   petrochenkov

   on Aug 28, 2020

   Contributor

   More actions

   Perhaps it's annoying for the first time, but updating failing tests doesn't require any rebuilds and take ~5 minutes at most after the initial build.

   • You run x.py test --bless src/test/ui tidy for the first time, this is the long part.
   • A number of tests fail due to wrong or overlong //~ ERROR annotations, they are fixed by updating or trimming the //~ ERROR annotations.
   • x.py test --bless src/test/ui tidy is run again, it won't build anything and only re-test the changed tests. Fix remaining errors if something was missing, repeat as necessary.
8. pickfire commented on Sep 9, 2020

   

   pickfire

   on Sep 9, 2020

   Contributor

   More actions

   they are fixed by updating or trimming the //~ ERROR annotations.

   But what do I need to do? I got this

   tidy error: /checkout/src/test/ui/parser/default.rs:24: line longer than 100 chars
   tidy error: /checkout/src/test/ui/parser/duplicate-visibility.rs:5: line longer than 100 chars
   

   No action was mention on how to solve it so the first one that came into my mind is to add tidy ignore. Maybe it should have some actionable steps?

   The line looks like this and I don't know how to continue other than the mentioned // ignore tidy-line-length, I don't quite know how should I trim it.

   impl Foo for u32 { //~ ERROR not all trait items implemented, missing: `foo`
       default pub fn foo<T: Default>() -> T { T::default() }
       //~^ ERROR expected one of `async`, `const`, `extern`, `fn`, `pub`, `unsafe`, or `use`, found keyword `pub`
   }
9. camelid

   added

   T-bootstrapRelevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

   A-testsuiteArea: The testsuite used to check the correctness of rustcArea: The testsuite used to check the correctness of rustc

   on Oct 20, 2020
10. jyn514 commented on Apr 30, 2021

    

    jyn514

    on Apr 30, 2021

    Member

    More actions

    @kornelski do you think disabling the line length check is enough? #77675
    Or do you think other checks should be disabled too?

    I do think tidy (and all tests really) should suggest how to replicate exactly what went wrong without running a full x.py test, I'll open a separate issue for that.
11. kornelski commented on Apr 30, 2021

    

    kornelski

    on Apr 30, 2021

    ContributorAuthor

    More actions

    Line length has been the major issue, so perhaps just this is enough.

    👍React with 👍1Reacted by jyn
12. kornelski

    closed this as completedon Apr 30, 2021
13. kornelski

    reopened this on Apr 30, 2021
14. jyn514 commented on Apr 30, 2021

    

    jyn514

    on Apr 30, 2021

    Member

    More actions

    Ok. I'm sorry you had a frustrating experience. I'll see if I can implement #84742 today or this weekend.

    👍React with 👍1Reacted by Kornel
15. jyn514

    closed this as completedon Apr 30, 2021