[flutter_tools] Update error matching regex in build_windows #98137
Conversation
flutter-dashboard
bot
added
the
tool
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
I wasn't sure where to write tests for this so I didn't add any, if they are needed let me know where to add them. |
miltosdoul
changed the title
|
@stuartmorgan can you take a look at this one? |
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
Yes, this needs tests. Unit tests for this file are in https://github.com/flutter/flutter/blob/master/packages/flutter_tools/test/commands.shard/hermetic/build_windows_test.dart
| // MSBuild sends all output to stdout, including build errors. This surfaces | ||
| // known error patterns. | ||
| final RegExp errorMatcher = RegExp(r':\s*(?:warning|(?:fatal )?error).*?:'); | ||
| final RegExp errorMatcher = RegExp(r':\s*(?:warning|(?:fatal )?error).*?:|.*Error detected in pubspec.yaml:.*|.*No file or variants found for asset: .*'); |
There was a problem hiding this comment.
Having to encode the specific error message is very unfortunate; any other pubspec error is going to be missing all of the actual details until we add each one as it comes up. But without fundamentally reworking stream I don't have a better suggestion.
We should at least make this more maintainable and clearly documented though. Something like:
final List<String> errorMessages = <String>[
r':\s*(?:warning|(?:fatal )?error).*?:',
r'Error detected in pubspec.yaml:',
];
// Known secondary error lines for pubspec.yaml errors:
final List<String> pubspecErrorDetails = <String>[
r'No file or variants found for asset:',
];
final RegExp errorMatcher = RegExp(<String>[
...errorMessages,
...pubspecErrorDetails,
].join('|'));
(Note that I have remove the leading and trailing .* from your strings. As you can see from the existing regex, this is not a full-line match.)
christopherfujino
added
the
waiting for customer response
|
Closing this as stale. Feel free to re-open if you are ready to respond to code review. Thanks! |
Added Regex to match error message from verbos build as suggested by @stuartmorgan [here](#98137 (comment)). Modified Windows Build Test Fixes #97065
Added Regex to match error message from verbos build as suggested by @stuartmorgan [here](flutter#98137 (comment)). Modified Windows Build Test Fixes flutter#97065
Added Regex to match error message from verbos build as suggested by @stuartmorgan [here](flutter#98137 (comment)). Modified Windows Build Test Fixes flutter#97065
3 participants
Updates Windows build error-matching regular expression to match error regarding invalid asset files added in pubspec.yaml project file.
Fixes #97065.
Pre-launch Checklist
///).