-
Notifications
You must be signed in to change notification settings - Fork 6k
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
[Impeller] started asserting golden test runner has fatal impeller validations #51357
Conversation
ac7848a
to
8400d93
Compare
8400d93
to
1c636fd
Compare
8c83828
to
d5eb464
Compare
@@ -24,7 +25,14 @@ void print_usage() { | |||
} | |||
} // namespace | |||
|
|||
namespace impeller { | |||
TEST(ValidationTest, IsFatal) { | |||
EXPECT_TRUE(ImpellerValidationErrorsAreFatal()); |
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 tried to use EXPECT_EXIT
but I couldn't get it to stop killing the process.
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.
You want to use EXPECT_DEATH?
That seems to work for me
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 "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use 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. |
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.
LGTM, but agree with expect_death
if able.
I'll give that another try today. |
Yea looks like EXPECT_DEATH is unreliable in processes with more than one thread:
I looked to see if I could make sure the test happens first, presumably before the other threads are spawned, and it doesn't look like there is a way to make that the case reliably with googletest. |
…mpeller validations (flutter/engine#51357)
…145164) flutter/engine@41fce3f...cc1e7d1 2024-03-14 [email protected] [Impeller] started asserting golden test runner has fatal impeller validations (flutter/engine#51357) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
fixes flutter/flutter#145017
This works by removing the conditional compilation for validation and turning them to be fatal in the test runner's main.
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.