Validate error patterns and error annotation in ui tests when present #65759
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (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. |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
| @@ -3137,21 +3137,24 @@ impl<'test> TestCx<'test> { | |||
| self.fatal_proc_rec("test run succeeded!", &proc_res); | |||
| } | |||
| } | |||
| if !self.props.error_patterns.is_empty() { | |||
| // "// error-pattern" comments | |||
| self.check_error_patterns(&proc_res.stderr, &proc_res); | |||
There was a problem hiding this comment.
It's a bit weird that this is checked in two places - first in should_run and then in !should_run below.
I think, we should run check_expected_errors and check_error_patterns together under the same conditions.
There was a problem hiding this comment.
Also, could you try to remove the !explicit && self.config.compare_mode.is_none() condition as well?
I'm not sure what will happen, but perhaps we can check annotations in those modes as well?
There was a problem hiding this comment.
After fixing the first bug, i.e., no validation being performed when
compilation succeeds, some test cases started to fail because they expected
error patterns to be matched against program output.
To address that issue, in the should_run case the error patterns are matched
against the program output, and otherwise the errors patterns are matched
against compiler output as before. This is why they are checked in two places.
There was a problem hiding this comment.
The explicit tests are those that use --error-format flag. They are
generally incompatible with error annotations, since there is only json error
parser. I also got two failures after removing this check, on missing-type.rs
and unused_parens_remove_json_suggestion.rs.
I am not familiar with compare mode, so I would rather not touch
self.config.compare_mode.is_none() condition either.
There was a problem hiding this comment.
some test cases started to fail because they expected
error patterns to be matched against program output.
I see.
I'm pretty sure that was a misuse of the // error-pattern annotation.
We have a separate annotation for checking the runtime output - // check-run-results.
There was a problem hiding this comment.
Indeed, right now there are only handful of test in ui that use error-pattern
for matching executable results, so they could be presumably migrated to use
check-run-results.
At the same time, the changes are party motivated as help with #65506 which
moves run-fails tests to ui. For those tests such use of error-pattern is quite
typical.
Which is why I think we should allow for such use of error-pattern, at least
until all those tests are migrated if check-run-results is preferred variant.
There was a problem hiding this comment.
@tmiasko
Could you make an issue for migrating run-fail tests to // check-run-results and restoring the compile-time meaning of // error-pattern in them?
petrochenkov
added
S-waiting-on-author
| // "// error-pattern" comments | ||
| self.check_error_patterns(&proc_res.stderr, &proc_res); | ||
| } | ||
| if !expected_errors.is_empty() { |
There was a problem hiding this comment.
Looks like the removal of this condition should fix #55596.
There was a problem hiding this comment.
The fallout from making annotations for errors and warnings mandatory is
quite substantial, so I would rather not fix that issue in this PR.
|
@bors r+ |
|
📌 Commit 07283544bf5423940cf89108a0858f0eabf3896f has been approved by |
bors
added
S-waiting-on-bors
3 tasks
|
☔ The latest upstream changes (presumably #65885) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
S-waiting-on-author
|
@bors r+ |
|
📌 Commit 88bfc2f has been approved by |
bors
added
S-waiting-on-bors
Centril
added a commit
to Centril/rust
that referenced
this pull request
Oct 29, 2019
Validate error patterns and error annotation in ui tests when present Previously, when compilation succeeded, neither error patterns nor error annotation would be validated. Additionally, when compilation failed, only error patterns would be validated if both error patterns and error annotation were present. Now both error patterns and error annotation are validated when present, regardless of compilation status. Furthermore, for test that should run, the error patterns are matched against executable output, which is what some of tests already expect to happen, and when rust-lang#65506 is merged even more ui tests will. Fixes rust-lang#56277
bors
added
S-waiting-on-bors
|
⌛ Testing commit 17e7cbdbdb9601b5717fdf5bc51180f4ef78dbbf with merge 8bfc5668b6f2524abfeb9f1a66ad6b0ba05c5432... |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
💔 Test failed - checks-azure |
bors
added
S-waiting-on-review
|
NLL versions of tests can be run by adding |
petrochenkov
added
S-waiting-on-author
When both error patterns and error annotations are present in an ui test, only error patterns are validated against the output. Replace the error pattern with an error annotation to avoid silently ignoring the other error annotation.
Since 8ec9d72, in the case of a local macro expansion, the errors are now matched to macro definition location. Update test cases accordingly.
Previously, when compilation succeeded, neither error patterns nor error annotation would be validated. Additionally, when compilation failed, only error patterns would be validated if both error patterns and error annotation were present. Now both error patterns and error annotation are validated when present, regardless of compilation status. Furthermore, for test that should run, the error patterns are matched against executable output, which is what some of tests already expect to happen, and when rust-lang#65506 is merged even more ui tests will.
The concrete type that will be too big is target dependent. Avoid matching it in error annotation to make test work correctly across different targets.
|
Amended the commit changing |
|
@bors r+ |
|
📌 Commit cfa2a26 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Nov 3, 2019
Validate error patterns and error annotation in ui tests when present Previously, when compilation succeeded, neither error patterns nor error annotation would be validated. Additionally, when compilation failed, only error patterns would be validated if both error patterns and error annotation were present. Now both error patterns and error annotation are validated when present, regardless of compilation status. Furthermore, for test that should run, the error patterns are matched against executable output, which is what some of tests already expect to happen, and when #65506 is merged even more ui tests will. Fixes #56277
|
☀️ Test successful - checks-azure |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, when compilation succeeded, neither error patterns nor error
annotation would be validated. Additionally, when compilation failed,
only error patterns would be validated if both error patterns and error
annotation were present.
Now both error patterns and error annotation are validated when present,
regardless of compilation status. Furthermore, for test that should run,
the error patterns are matched against executable output, which is what
some of tests already expect to happen, and when #65506 is merged even
more ui tests will.
Fixes #56277