Skip to content

Tidy on UI tests is annoying #75987

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
kornelski opened this issue Aug 27, 2020 · 8 comments
Closed

Tidy on UI tests is annoying #75987

kornelski opened this issue Aug 27, 2020 · 8 comments
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@kornelski
Copy link
Contributor

I've added a single word to an error message, and it broke tests for a silly reason:

tidy error: /checkout/src/test/ui/export-fully-qualified.rs:6: line longer than 100 chars
tidy error: /checkout/src/test/ui/export2.rs:2: line longer than 100 chars
tidy error: /checkout/src/test/ui/imports/extern-prelude-extern-crate-fail.rs:10: line longer than 100 chars

The line has to be that long, otherwise it's not testing the message. I don't know what to do about this.

@kornelski
Copy link
Contributor Author

Ok, I've found // ignore-tidy-line-length. But I think it should be the default for UI tests.

@petrochenkov
Copy link
Contributor

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.

@kornelski kornelski changed the title tidy is being run on UI tests Tidy on UI tests is annoying Aug 28, 2020
@kornelski kornelski reopened this Aug 28, 2020
@kornelski
Copy link
Contributor Author

kornelski commented Aug 28, 2020

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".

@petrochenkov
Copy link
Contributor

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.

@pickfire
Copy link
Contributor

pickfire commented Sep 9, 2020

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`
}

@camelid camelid added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) A-testsuite Area: The testsuite used to check the correctness of rustc labels Oct 20, 2020
@jyn514
Copy link
Member

jyn514 commented Apr 30, 2021

@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.

@kornelski
Copy link
Contributor Author

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

@kornelski kornelski reopened this Apr 30, 2021
@jyn514
Copy link
Member

jyn514 commented Apr 30, 2021

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

@jyn514 jyn514 closed this as completed Apr 30, 2021
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 T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

No branches or pull requests

5 participants