-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Check //~ERROR comments in ui tests #46116
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
I'm very excited to see this PR! One minor thing is that I think there was some minor advantage in trying to capture the exact output we gave to the user (versus relying on JSON). But I think having just a test or two that checks that would be good enough -- and really we weren't checking colors anyhow. |
let id_span_message = (lint_id, span, message.to_owned()); | ||
let fresh = self.one_time_diagnostics.borrow_mut().insert(id_span_message); | ||
if fresh { | ||
do_method() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The motivation for defining the do_method
closure was to avoid repeating the match method
code for the JSON and human-friendly cases: if we're going to start treating these the same, we can delete lines 362–371 and put the match here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
||
warning: unreachable pattern | ||
--> $DIR/issue-43253.rs:49:9 | ||
| | ||
49 | 9...9 => {}, | ||
| ^^^^^ | ||
| | ||
note: lint level defined here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a regression in one-time diagnostics (the motivation for which was avoiding too many "lint level defined here"s), but it's not immediately clear why that would happen ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is just a mixup that happened due to the update-all script not actually updating everything after it produced the expected output once. I need to touch
all files in order to have them regenerate their stderr files.
I'm glad someone's taken over for me. Thanks. |
☔ The latest upstream changes (presumably #45545) made this pull request unmergeable. Please resolve the merge conflicts. |
I added one. It's just a matter of adding |
a30ef3f
to
f40b805
Compare
Travis should pass now. At least both |
☔ The latest upstream changes (presumably #46166) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me
I'm sure this is prone to bitrot, so p=1 as well.
src/libsyntax/json.rs
Outdated
@@ -170,6 +171,27 @@ impl Diagnostic { | |||
rendered: None, | |||
} | |||
}); | |||
|
|||
// generate regular command line output and store it in the json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an aside, one thing I have long wanted is precisely this: to have the full formatted output in the JSON. The only thing missing here are the colors, but I guess we could add that in a side "attribute" table saying like "the text from this column to that column is this color".
cc @estebank
Travis result:
|
f4cc0f6
to
5a6da19
Compare
tests pass locally now |
@bors r=nikomatsakis p=1 |
📌 Commit 5a6da19 has been approved by |
⌛ Testing commit 5a6da1969c0e5f74866ca6d0cec62aeca8948c95 with merge 8b0d1bb4012f71191363fdf3d024eb72454ab84c... |
💔 Test failed - status-travis |
The UI tests need to be updated to account for the recent merges. Please rebase on latest master.
|
updated |
☔ The latest upstream changes (presumably #46024) made this pull request unmergeable. Please resolve the merge conflicts. |
rebased again |
I have not been able to fix it in time (gotta run, and rebase + test is taking too long). Will try again tomorrow. Can I have bors delegation rights so I can immediately put it in the queue when I fix it? |
@bors delegate+ |
✌️ @oli-obk can now approve this pull request |
@bors r=nikomatsakis p=1 |
📌 Commit 8c1f9ef has been approved by |
Check //~ERROR comments in ui tests r? @nikomatsakis cc #44844 @Phlosioneer @estebank @petrochenkov this depends on #46052 getting merged first (the commits are included in here) The relevant changes of this PR are c2f0af7 and 979269b
💔 Test failed - status-appveyor |
legit:
|
I wasn't able to test on windows, but I undid the change that lead to the problem (replacing @bors r=nikomatsakis p=1 |
📌 Commit 8937d6a has been approved by |
Check //~ERROR comments in ui tests r? @nikomatsakis cc #44844 @Phlosioneer @estebank @petrochenkov this depends on #46052 getting merged first (the commits are included in here) The relevant changes of this PR are c2f0af7 and 979269b
☀️ Test successful - status-appveyor, status-travis |
r? @nikomatsakis
cc #44844 @Phlosioneer @estebank @petrochenkov
this depends on #46052 getting merged first (the commits are included in here)
The relevant changes of this PR are c2f0af7 and 979269b