-
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
Add a throw statement for imgtestAdd non 0 exit codes. #50829
Add a throw statement for imgtestAdd non 0 exit codes. #50829
Conversation
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 and thanks @Piinks for catching this!
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.
Still unclear why this is untested?
print('goldctl imgtest add stdout: ${result.stdout}'); | ||
print('goldctl imgtest add stderr: ${result.stderr}'); | ||
final StringBuffer buf = StringBuffer() | ||
..writeln('Skia Gold imgtest add failed.') |
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.
Gonna nit this error message to try to encourage consistency across flutter/flutter and flutter/engine. In the framework we provide links to the dashboard for triage etc.
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 is where we handle this case in the framework: https://github.com/flutter/flutter/blob/b09a015ff5677da6cd9bb631ca44b801953cb2ad/packages/flutter_goldens/lib/skia_client.dart#L237
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.
So here it would be (I think the result-state.json file is not necessary):
final StringBuffer buf = StringBuffer()
..writeln('Skia Gold received an unapproved image in post-submit ')
..writeln('testing. Golden file images in flutter/engine are triaged ')
..writeln('in pre-submit during code review for the given PR.')
..writeln()
..writeln('Visit https://flutter-engine-gold.skia.org/ to view and approve ')
..writeln('the image(s), or revert the associated change. For more ')
..writeln('information, visit the wiki: ')
..writeln('https://github.com/flutter/flutter/wiki/Writing-a-golden-file-test-for-package:flutter')
..writeln()
..writeln('Debug information for Gold --------------------------------')
..writeln('stdout: ${result.stdout}')
..writeln('stderr: ${result.stderr}');
throw SkiaException(buf.toString());
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.
Nit appreciated and updated.
It's untested because it was allowed to be submitted without tests 😅 . I can volunteer to write some tests, but it won't be in the same PR. |
…ent` (#50844) @Piinks correctly pointed out we had to make a number of changes to the engine's `SkiaGoldClient` and we had no test coverage (#50829, #50826). This corrects the problem. I am not in _love_ with these tests (they could be less snapshot-y), but the mechanisms I need to write better tests is beyond what I can spend ~1-2 hours on (flutter/flutter#133569). /cc @dnfield @mdebbar
…143911) flutter/engine@bf5c003...7eeb697 2024-02-22 [email protected] Add view focus direction detection to flutter web. (flutter/engine#50843) 2024-02-22 [email protected] Add a similar `runIf` guard to `web_engine` as web framework. (flutter/engine#50846) 2024-02-22 [email protected] Add scheduleWarmUpFrame (flutter/engine#50570) 2024-02-22 [email protected] Remove/reduce unused or private methods and add tests to `SkiaGoldClient` (flutter/engine#50844) 2024-02-22 [email protected] Clean up contributing formatting, add a Skia gold callout (flutter/engine#50828) 2024-02-21 [email protected] Make Android Studio be able to run android embedding unit tests out of the box (flutter/engine#50840) 2024-02-21 [email protected] Shift some deps to //flutter/third_party (flutter/engine#50830) 2024-02-21 [email protected] Make the view focus binding report focus transitions across elements. (flutter/engine#50610) 2024-02-21 [email protected] [macOS] Wrap FlutterEngineTest in autoreleasepool (flutter/engine#50832) 2024-02-21 [email protected] Update the vulkan_glfw sample for the latest roll of vulkan-deps (flutter/engine#50839) 2024-02-21 [email protected] Roll Skia from 8fa858855820 to 57490c8d257e (6 revisions) (flutter/engine#50833) 2024-02-21 [email protected] Starts a .ci.yaml parser (flutter/engine#50783) 2024-02-21 [email protected] Roll Dart SDK from f344e2266468 to 0f0f7400c38a (6 revisions) (flutter/engine#50837) 2024-02-21 [email protected] [Windows] Fix top-level message procedure order (flutter/engine#50797) 2024-02-21 [email protected] Tweak verbose log messages in ImageReaderSurfaceProducer (flutter/engine#50831) 2024-02-21 [email protected] Add a throw statement for imgtestAdd non 0 exit codes. (flutter/engine#50829) 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
Thanks @Piinks for noticing.