Stop ignoring expected note/help messages in compiletest suite. #32263
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
/cc @fhahn |
src/compiletest/runtest.rs
Outdated
There was a problem hiding this comment.
I have a separate branch that cleans up this "kind" logic by using enums. I plan to open it up as a follow-up PR if this merges.
There was a problem hiding this comment.
This should make the opt-in work for note and help as well, but it is still opt-in and notes and help messages are only checked in files with help and note annotations.
When making this changes, it seems like I overlooked note and help (and assumed there is more normalization going on). I think using enums there would make it clearer 👍
frewsxcv
force-pushed
the
compiletest-ignored-expected
branch
from
2991d5b to
fb96cc8
Compare
src/test/compile-fail/issue-25386.rs
Outdated
There was a problem hiding this comment.
I'm pretty sure I did this incorrectly. A couple errors in this testcase have errors that report on <std macros> and I'm not sure the best way to handle it.
There was a problem hiding this comment.
there really isn't a good way, actually. In this case though I'd just remove the NOTEs. the issue in question (#25386) was an ICE, not a test of diagnostics (I'd not include NOTEs unless the purpose is to test that they are emitted, personally)
There was a problem hiding this comment.
Cool, that's what I figured, I'll remove them now.
There was a problem hiding this comment.
Removed in the latest force push.
|
A little background on this PR: I did a bit of refactoring on compiletest and realized that some help/note messages were being ignored. I noticed this conditional and assumed it was a mistake to not also check |
|
On second thought, I think it is still opt-in, but my changes make the opt-in work for both |
src/compiletest/runtest.rs
Outdated
There was a problem hiding this comment.
Pre-existing, but can't we use any here instead of fold? Seems like it more clearly expresses the intent.
There was a problem hiding this comment.
Given this code block:
fn main() {
let i = 1; //~ NOTE: some note
let j = 2; //~ HELP: some help
}The current implementation checks for note and help in one pass through. I'm not sure how it'd be possible using any unless we were expecting both help and note on the same line. I agree it's not clean, and I'm willing to clean it up, but would rather do that in a different PR.
There was a problem hiding this comment.
Do it in two passes?
On Wed, Mar 16, 2016 at 12:25:23PM -0700, Corey Farwell wrote:
@@ -1013,8 +1013,8 @@ fn check_expected_errors(revision: Option<&str>,
expected_errors.iter()
.fold((false, false),Given this code block:
fn main() { let i = 1; //~ NOTE: some note let j = 2; //~ HELP: some help }The current implementation checks for
noteandhelpin one pass through. I'm not sure how it'd be possible usinganyunless we were expecting bothhelpandnoteon the same line. I agree it's not clean, and I'm willing to clean it up, but would rather do that in a different PR.
You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub:
https://github.com/rust-lang/rust/pull/32263/files/fb96cc8b5d83da8c32cdaf51a5f53edb74152024#r56398787
There was a problem hiding this comment.
Spoke with you on IRC. Going to do it in a follow-up PR.
|
left some nits, but seems good |
|
let me know when they're fixed :) |
frewsxcv
force-pushed
the
compiletest-ignored-expected
branch
from
fb96cc8 to
0f8fdba
Compare
Assuming your only nit was this one, then it's fixed |
Original issue: rust-lang#21195 Relevant PR: rust-lang#30778 Prior to this commit, if a compiletest testcase included the text "HELP:" or "NOTE:" (note the colons), then it would indicate to the compiletest suite that we should verify "help" and "note" expected messages. This commit updates this check to also check "HELP" and "NOTE" (not the absense of colons) so that we always verify "help" and "note" expected messages.
frewsxcv
force-pushed
the
compiletest-ignored-expected
branch
from
0f8fdba to
abd1cea
Compare
|
Merge conflicts addressed. |
|
@bors r+ |
|
📌 Commit abd1cea has been approved by |
…atsakis Stop ignoring expected note/help messages in compiletest suite. Original issue: #21195 Relevant PR: #30778 Prior to this commit, if a compiletest testcase included the text "HELP:" or "NOTE:" (note the colons), then it would indicate to the compiletest suite that we should verify "help" and "note" expected messages. This commit updates this check to also check "HELP" and "NOTE" (not the absense of colons) so that we always verify "help" and "note" expected messages.
6 participants
Original issue: #21195
Relevant PR: #30778
Prior to this commit, if a compiletest testcase included the text
"HELP:" or "NOTE:" (note the colons), then it would indicate to the
compiletest suite that we should verify "help" and "note" expected
messages.
This commit updates this check to also check "HELP" and "NOTE" (not the
absense of colons) so that we always verify "help" and "note" expected
messages.