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

Most of UI tests are now compiled twice, once with --error-format json and once with --error-format human #48550

Closed
petrochenkov opened this issue Feb 26, 2018 · 7 comments
Labels
T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.

Comments

@petrochenkov
Copy link
Contributor

petrochenkov commented Feb 26, 2018

The change was made in #48337
cc @GuillaumeGomez

This is not helpful for our testing times, especially given that number of UI tests will grow due to other test categories being converted to UI.

@petrochenkov petrochenkov added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Feb 26, 2018
@GuillaumeGomez
Copy link
Member

Yep, sorry about that. However, if I didn't do this, the output was broken (because we'd have multiple useless "rustc --explain" sentences). If anyone has a better idea, it'd be very appreciated!

@GuillaumeGomez
Copy link
Member

Actually, for the moment we only rely on the json error output. Add a new check over the "normal" error output would allow us to greatly reduce the ui tests duration.

@zackmdavis
Copy link
Member

@GuillaumeGomez

if I didn't do this, the output was broken (because we'd have multiple useless "rustc --explain" sentences).

Can you be more specific about what the problem was exactly? Intuitively, I wouldn't expect printing one or two extra lines after the errors to require any changes to the UI test framework itself.

@zackmdavis
Copy link
Member

zackmdavis commented Feb 28, 2018

I have a branch which reverts 1dc2015. And indeed, when outputting for humans, we see the message about --explain just once, but in JSON mode, it appears once per error. It looks like this is because the message is being printed inside of EmitterWriter's .drop() method (!), and the JSON emitter itself instantiates an EmitterWriter when creating a Diagnostic from a DiagnosticBuilder.

@zackmdavis
Copy link
Member

I need to sleep now, but I'd like to put together a PR tomorrow

@zackmdavis
Copy link
Member

I'm pretty confident that we want to revert #48337: printing the --explain usage message inside of EmitterWriter.drop was not an adequate solution (because it's counterintuitive to be doing non-cleanup work in .drop, the unintended additional messages for every diagnostic in the JSON output were undesirable, and changing the test framework in response to that unintended undesirable thing is also undesirable).

I was hoping that, in order to avoid another enormous UI test output diff (which could potentially slow down the rest of the queue), it would be easy to reimplement the --explain usage message near where we issue the "aborting after n errors" message, without changing the output: then we would only have one more enormous UI test output diff when we edit the wording (#48559). But outputting the --explain usage message along with the "aborting after n errors" message in the Handler is looking somewhat less trivial that it sounds, because Handler doesn't seem to have direct access to an output stream where we can write some arbitrarily lines; it just has an Emitter trait object whose sole method (emit) can only take a DiagnosticBuilder. No doubt we could formulate the --explain message that we want in the form of a diagnostic (or come up with some alternative solution), but I wouldn't want to block on that.

zackmdavis added a commit to zackmdavis/rust that referenced this issue 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 issue 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
zackmdavis added a commit to zackmdavis/rust that referenced this issue Mar 1, 2018
@petrochenkov
Copy link
Contributor Author

Fixed in #48684

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants