Skip to content

Use Layer.toImage for golden tests on CanvasKit #135249

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.

Sign up for GitHub

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

Jump to bottom
Merged
harryterkelsen merged 2 commits into from
Oct 18, 2023
Merged

Use Layer.toImage for golden tests on CanvasKit #135249

harryterkelsen merged 2 commits into from
Oct 18, 2023

Conversation

@harryterkelsen
Copy link
Contributor

@harryterkelsen harryterkelsen commented Sep 21, 2023

Changes golden tests on CanvasKit to use Layer.toImage instead of browser APIs for screenshots. This brings it more in line with other platforms and should also fix some async timing bugs with tests.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard
Copy link

flutter-dashboard bot commented Sep 21, 2023

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 or stuartmorgan on 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.

@github-actions github-actions bot added a: tests "flutter test", flutter_test, or one of our tests tool Affects the "flutter" command-line tool. See also t: labels. framework flutter/packages/flutter repository. See also f: labels. labels Sep 21, 2023
christopherfujino
christopherfujino reviewed Sep 21, 2023
View reviewed changes
Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tool change LGTM (not sure the right person to review the flutter_test changes)

@harryterkelsen
Copy link
Contributor Author

harryterkelsen commented Sep 27, 2023

Coming back to this now that the drawShadow fix for CanvasKit has landed

@flutter-dashboard
Copy link

flutter-dashboard bot commented Sep 27, 2023

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #135249 at sha 401aeb1

@flutter-dashboard flutter-dashboard bot added the will affect goldens Changes to golden files label Sep 27, 2023
yjbanov
yjbanov approved these changes Sep 28, 2023
View reviewed changes
Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@flutter-dashboard
Copy link

flutter-dashboard bot commented Sep 28, 2023

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #135249 at sha ba271f1

This was referenced Sep 28, 2023
Closed
Closed
@goderbauer
Copy link
Member

goderbauer commented Oct 10, 2023

(triage): Can this one be closed in favor of #135711 or vice versa?

@github-actions github-actions bot added a: text input Entering text in a text field or keyboard related problems engine flutter/engine related. See also e: labels. a: animation Animation APIs f: material design flutter/packages/flutter/material repository. a: internationalization Supporting other languages or locales. (aka i18n) f: scrolling Viewports, list views, slivers, etc. f: cupertino flutter/packages/flutter/cupertino repository d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: routes Navigator, Router, and related APIs. f: gestures flutter/packages/flutter/gestures repository. a: desktop Running on desktop f: focus Focus traversal, gaining or losing focus f: integration_test The flutter/packages/integration_test plugin labels Oct 18, 2023
@flutter-dashboard
Copy link

flutter-dashboard bot commented Oct 18, 2023

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #135249 at sha d93413b9655b19eafb643df3fbb25f49191bbb47

@harryterkelsen harryterkelsen force-pushed the canvaskit-golden-screenshot branch from 9b60d8f to 5298b5b Compare October 18, 2023 21:01
@flutter-dashboard
Copy link

flutter-dashboard bot commented Oct 18, 2023

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #135249 at sha e053568

@harryterkelsen
Copy link
Contributor Author

harryterkelsen commented Oct 18, 2023

Landing because the google testing seems broken. The only failures are expected Scuba failures.

@harryterkelsen harryterkelsen merged commit 747128f into flutter:master Oct 18, 2023
@zanderso zanderso added the revert Autorevert PR (with "Reason for revert:" comment) label Oct 19, 2023
@auto-submit auto-submit bot mentioned this pull request Oct 19, 2023
Merged
8 tasks
@auto-submit auto-submit bot removed the revert Autorevert PR (with "Reason for revert:" comment) label Oct 19, 2023
auto-submit bot pushed a commit that referenced this pull request Oct 19, 2023
Revert "Use Layer.toImage for golden tests on CanvasKit (#135249)"
b5d9ad2 
This reverts commit 747128f.
auto-submit bot added a commit that referenced this pull request Oct 19, 2023
@auto-submit
Reverts "Use Layer.toImage for golden tests on CanvasKit" (#136860)
c881808 
Reverts #135249
Initiated by: zanderso
This change reverts the following previous change:
Original Description:
Changes golden tests on CanvasKit to use Layer.toImage instead of browser APIs for screenshots. This brings it more in line with other platforms and should also fix some async timing bugs with tests.
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 19, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 19, 2023
@engine-flutter-autoroll
747128f Use Layer.toImage for golden tests on CanvasKit (flutter/flut…
74b0362 
…ter#135249)
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Oct 19, 2023
@engine-flutter-autoroll
Roll Flutter from 189196d to c2bd2c1 (11 revisions) (#5183)
ed1b0a3 
flutter/flutter@189196d...c2bd2c1

2023-10-19 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Fixes ability to call nextFocus() on a node to focus its descendant" (flutter/flutter#136894)
2023-10-19 [email protected] Roll Flutter Engine from d0d7b4d61265 to 28cb2508b8e0 (1 revision) (flutter/flutter#136878)
2023-10-19 [email protected] Roll Flutter Engine from 8d51b64e9440 to d0d7b4d61265 (13 revisions) (flutter/flutter#136872)
2023-10-19 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Add code for updating `focusedChild` when removing grandchildren from scope" (flutter/flutter#136862)
2023-10-19 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Use Layer.toImage for golden tests on CanvasKit" (flutter/flutter#136860)
2023-10-18 [email protected] Bump flutter_lints to 3.0 (flutter/flutter#136841)
2023-10-18 [email protected] Add code for updating `focusedChild` when removing grandchildren from scope (flutter/flutter#136771)
2023-10-18 [email protected] Use Layer.toImage for golden tests on CanvasKit (flutter/flutter#135249)
2023-10-18 [email protected] Roll Flutter Engine from b67edb05d3f7 to 8d51b64e9440 (4 revisions) (flutter/flutter#136846)
2023-10-18 [email protected] Roll Flutter Engine from 6caee3236d37 to b67edb05d3f7 (5 revisions) (flutter/flutter#136844)
2023-10-18 [email protected] Null-assert the value given to a Completer expecting a non-null value (flutter/flutter#136776)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
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 Packages: 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
@harryterkelsen harryterkelsen mentioned this pull request Oct 19, 2023
Merged
harryterkelsen added a commit that referenced this pull request Oct 20, 2023
@harryterkelsen
Reland "Use Layer.toImage for golden tests on CanvasKit" (#136918)
464e751 
Relands #135249

A golden test was failing in post submit in the previous PR
@yjbanov yjbanov mentioned this pull request Oct 24, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yjbanov yjbanov yjbanov approved these changes

+1 more reviewer

@christopherfujino christopherfujino christopherfujino left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

a: animation Animation APIs a: desktop Running on desktop a: internationalization Supporting other languages or locales. (aka i18n) a: tests "flutter test", flutter_test, or one of our tests a: text input Entering text in a text field or keyboard related problems d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos engine flutter/engine related. See also e: labels. f: cupertino flutter/packages/flutter/cupertino repository f: focus Focus traversal, gaining or losing focus f: gestures flutter/packages/flutter/gestures repository. f: integration_test The flutter/packages/integration_test plugin f: material design flutter/packages/flutter/material repository. f: routes Navigator, Router, and related APIs. f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. tool Affects the "flutter" command-line tool. See also t: labels. will affect goldens Changes to golden files

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@harryterkelsen @goderbauer @yjbanov @christopherfujino @zanderso