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

Diffs from UI tests are noisy when line numbers change #46643

Closed
petrochenkov opened this issue Dec 10, 2017 · 6 comments
Closed

Diffs from UI tests are noisy when line numbers change #46643

petrochenkov opened this issue Dec 10, 2017 · 6 comments
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-feature-request Category: A feature request, i.e: not implemented / a PR.

Comments

@petrochenkov
Copy link
Contributor

Here's a typical example: #46640

One line added at the start of the test source (// must-compile-successfully) causes changes in all line numbers in the test's output.

 warning: unreachable pattern
-  --> $DIR/issue-43253.rs:37:9
+  --> $DIR/issue-43253.rs:39:9
    |
-37 |         9 => {},
+39 |         9 => {},
    |         ^
    |
 note: lint level defined here
-  --> $DIR/issue-43253.rs:12:9
+  --> $DIR/issue-43253.rs:14:9
    |
-12 | #![warn(unreachable_patterns)]
+14 | #![warn(unreachable_patterns)]
    |         ^^^^^^^^^^^^^^^^^^^^
 
 warning: unreachable pattern
-  --> $DIR/issue-43253.rs:43:9
+  --> $DIR/issue-43253.rs:45:9
    |
-43 |         8...9 => {},
+45 |         8...9 => {},
    |         ^^^^^
 
 warning: unreachable pattern
-  --> $DIR/issue-43253.rs:49:9
+  --> $DIR/issue-43253.rs:51:9
    |
-49 |         9...9 => {},
+51 |         9...9 => {},
    |         ^^^^^

There are probably tests in which specific values of line numbers are important, but maybe they can be replaced with some placeholder by default?

@petrochenkov petrochenkov added the A-testsuite Area: The testsuite used to check the correctness of rustc label Dec 10, 2017
@oli-obk
Copy link
Contributor

oli-obk commented Dec 11, 2017

We could filter out line numbers, but then moving something across the line 9/10 border will still produce noise because of the additional indent

To make sure that tests where line numbers are important still can be tested we could have a flag that skips the filter step

@oli-obk
Copy link
Contributor

oli-obk commented Dec 11, 2017

Alternatively we could replace line numbers by line offsets to the previous diagnostic's line number.

@petrochenkov
Copy link
Contributor Author

moving something across the line 9/10 border will still produce noise because of the additional indent

Any line number can be replaced with a constant-width placeholder: 1 => LL, 12 => LL, 123 => LL, $DIR/issue-43253.rs:39:9 => $DIR/issue-43253.rs:LL:CC and the replacement can be performed by rustc before (or during) diagnostics formatting under a debug option.

To make sure that tests where line numbers are important still can be tested we could have a flag that skips the filter step

Yes, that's what I meant by "by default".

Alternatively we could replace line numbers by line offsets to the previous diagnostic's line number.

This mostly solves the noise problem, but I suspect the resulting numbers will be more misleading than actually useful.

@kennytm kennytm added the C-feature-request Category: A feature request, i.e: not implemented / a PR. label Dec 11, 2017
@kennytm
Copy link
Member

kennytm commented Dec 11, 2017

We could introduce a flag (or the opposite of it)

// redact-line-numbers

then during the normalization step, it will replace all \.rs:\d+:\d+ by .rs:$LL:$CC and all ^\d+\s+\| by $LL |.

However, the line 9/10 problem still happens due to alignment:

error[E0322]: explicit impls for the `Sized` trait are not permitted
 --> src/main.rs:7:1
  |
7 | / impl Sized for u32 {
8 | | }
  | |_^ impl of 'Sized' not allowed

error[E0322]: explicit impls for the `Sized` trait are not permitted
  --> src/main.rs:9:1
   |
9  | / impl Sized for u64 {
10 | | }
   | |_^ impl of 'Sized' not allowed

Note the column position of the arrow --> and vertical bar for the error happening on line 7 and line 9. This can't be avoided unless

  • we can instruct rustc to always reserve 3 digits in the gutter (or find some way to tell rustc to start counting from line 100).
  • or we additionally replace all ^\s{2,}\| by ␣␣␣␣| and \s+--> by ␣␣␣-->. I'm not sure if the former will introduce any false replacement.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 11, 2017

I think the best solution would be the unstable compiler flag that does the replacement upfront (as suggested by @petrochenkov ), because of the large number of hacks needed to make the other solutions work.

the replacement can be performed by rustc before (or during) diagnostics formatting under a debug option.

@arielb1
Copy link
Contributor

arielb1 commented Dec 11, 2017

Don't we want to keep line numbers for at least the header lines ( --> $DIR/issue-43253.rs:49:9), to prevent confusion if error messages are reordered?

Manishearth added a commit to Manishearth/rust that referenced this issue Feb 25, 2018
Anonymize some line numbers in UI test output

New unstable flag `-Z ui-testing` is introduced. This flag changes diagnostic output of the compiler *in some way* making it more suitable for UI testing (this is intentionally vague).
At the moment this flag anonymizes line numbers at line starts thus solving the largest issue with UI test diffs. If diffs continue to be too noisy, some other tweaks could be applied (e.g. anonymizing lines/columns in `--> $DIR/file.rs:line:column`), but this needs some time and experience (we shouldn't diverge too much from the actual output in general).

If comment `// disable-ui-testing-normalization` is added to an UI test, then `-Z ui-testing` is not passed.

Closes rust-lang#46643
bors added a commit that referenced this issue Feb 27, 2018
Anonymize some line numbers in UI test output

New unstable flag `-Z ui-testing` is introduced. This flag changes diagnostic output of the compiler *in some way* making it more suitable for UI testing (this is intentionally vague).
At the moment this flag anonymizes line numbers at line starts thus solving the largest issue with UI test diffs. If diffs continue to be too noisy, some other tweaks could be applied (e.g. anonymizing lines/columns in `--> $DIR/file.rs:line:column`), but this needs some time and experience (we shouldn't diverge too much from the actual output in general).

If comment `// disable-ui-testing-normalization` is added to an UI test, then `-Z ui-testing` is not passed.

Closes #46643
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-feature-request Category: A feature request, i.e: not implemented / a PR.
Projects
None yet
Development

No branches or pull requests

4 participants