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

Anonymize some line numbers in UI test output #48449

Merged
merged 6 commits into from
Feb 27, 2018

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Feb 23, 2018

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

@rust-highfive
Copy link
Collaborator

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

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

r? @nikomatsakis

LL | &self[x]
| ------- recursive call site
|
= help: a `loop` may express intention better if this is on purpose
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example of a test with "long" actual numbers that become "short" anonymized numbers.

@estebank
Copy link
Contributor

This doesn't deal with the line number in the file:line:col section, but should be fine. LGTM.

LL | / fn foo<'a,'b,T>(x: &'a T, y: &'b T) //~ ERROR type annotations required
LL | | where &'a T : Foo,
LL | | &'b T : Foo
LL | | {
19 | | x.foo();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a line number left here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, noticed these too late, will fix today or tomorrow.
I haven't figured out yet where they come from.

@petrochenkov
Copy link
Contributor Author

This doesn't deal with the line number in the file:line:col section

cc #46643 (comment)

@bors
Copy link
Contributor

bors commented Feb 23, 2018

☔ The latest upstream changes (presumably #47799) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors 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 Feb 23, 2018
@petrochenkov
Copy link
Contributor Author

Updated.

@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 Feb 23, 2018
@nikomatsakis
Copy link
Contributor

Now that we have the //~ ERROR lines, sgtm.

@bors r+

@bors
Copy link
Contributor

bors commented Feb 23, 2018

📌 Commit a1065b4 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Feb 23, 2018

🌲 The tree is currently closed for pull requests below priority 99, this pull request will be tested once the tree is reopened

@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 Feb 23, 2018
@petrochenkov
Copy link
Contributor Author

@bors p=1
The patch is conflict-prone + other PRs in the queue modifying UI tests will benefit from it.

@bors
Copy link
Contributor

bors commented Feb 24, 2018

☔ The latest upstream changes (presumably #48476) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 24, 2018
@petrochenkov
Copy link
Contributor Author

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Feb 24, 2018

📌 Commit b94e8a4 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Feb 24, 2018

🌲 The tree is currently closed for pull requests below priority 100, this pull request will be tested once the tree is reopened

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Feb 26, 2018
@Mark-Simulacrum
Copy link
Member

Looks like now 64-bit AppVeyor is starting to hit the timeout as well :/

@kennytm
Copy link
Member

kennytm commented Feb 26, 2018

check-aux is timing out quite a lot actually. Perhaps doing cargo-test + pretty test together is too long for Windows. I’m thinking of moving one of the check-aux tests to the tools builder, but we need to figure out which ones can balance the time.

@bors
Copy link
Contributor

bors commented Feb 27, 2018

⌛ Testing commit 9f9183d with merge 29f5c69...

bors added a commit that referenced this pull request 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
@bors
Copy link
Contributor

bors commented Feb 27, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 29f5c69 to master...

@bors bors merged commit 9f9183d into rust-lang:master Feb 27, 2018
zackmdavis added a commit to zackmdavis/rust that referenced this pull request Feb 28, 2018
This reverts commit 1dc2015, which
switched to compiling the UI tests twice rather than extracting the
humanized output from a field in the JSON output.

A conflict in this revert commit had to be fixed manually where changes
introduced in rust-lang#48449 collide with the change we're trying to revert
(from rust-lang#48337).

This is in the matter of rust-lang#48550.

Conflicts:
	src/tools/compiletest/src/runtest.rs
zackmdavis added a commit to zackmdavis/rust that referenced this pull request Feb 28, 2018
This reverts commit 9b597a1. That
commit added a message informing users about the `rustc --explain`
functionality inside of a `Drop` implementation for `EmitterWriter`. In
addition to exhibiting questionable semantics (printing a
hopefully-helpful message for the user does not seem like a "cleanup"
action), this resulted in divergent behavior between humanized output
and JSON output, because the latter actually instantiates an
`EmitterWriter` for every diagnostic.

Several conflicts in this revert commit had to be fixed manually, again
owing to code-area collision between rust-lang#48449 and rust-lang#48337.

This is in the matter of rust-lang#48550.

Conflicts:
	src/librustc_errors/emitter.rs
flip1995 added a commit to flip1995/rust that referenced this pull request Mar 1, 2018
@petrochenkov petrochenkov deleted the uidiff branch June 5, 2019 16:02
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.

8 participants