[web] Start support for Skia Gold #29139

mdebbar · 2021-10-12T20:11:49Z

This PR provides initial support for Skia Gold. It uploads all screenshots to the Skia Gold dashboard.

At this point, we still use the goldens repo for the actual comparison.

mdebbar · 2021-10-19T16:58:53Z

@CaseyHillers I'm still not getting the GOLDCTL env variable. See this log file towards the end it shows:

_setupSkiaGoldClient:::GOLDCTL null

Other environment variables are working fine (e.g. SWARMING_TASK_ID, LUCI_CONTEXT, ENGINE_PATH, etc).

CaseyHillers · 2021-10-19T17:42:29Z

@mdebbar the issue is the goldctl is being run on a drone. I'm not sure if we'd support this. Are the gold tests something we could run on the original bot? An alternative is the web engine recipe can be updated to pass its properties to the drone builds

skia-gold · 2021-11-03T17:01:24Z

Gold has detected about 292 new digest(s) on patchset 6.
View them at https://flutter-engine-gold.skia.org/cl/github/29139

skia-gold · 2021-11-09T15:39:21Z

Gold has detected about 86 new digest(s) on patchset 13.
View them at https://flutter-engine-gold.skia.org/cl/github/29139

skia-gold · 2021-11-09T16:39:21Z

Gold has detected about 507 new digest(s) on patchset 14.
View them at https://flutter-engine-gold.skia.org/cl/github/29139

flutter-dashboard · 2021-11-09T18:46:01Z

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.

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.

skia-gold · 2021-11-09T19:34:21Z

Gold has detected about 217 new digest(s) on patchset 15.
View them at https://flutter-engine-gold.skia.org/cl/github/29139

Piinks · 2021-11-11T00:03:54Z

I am just back in office today, this look exciting! Will be able to do a full review tomorrow. 🎉
@mdebbar do you have a design doc or plans for migrating off of flutter/goldens for the comparison as you mentioned above?

lib/web_ui/dev/steps/run_tests_step.dart

yjbanov · 2021-11-11T02:24:43Z

lib/web_ui/dev/steps/run_tests_step.dart

+      } on io.OSError catch (_) {
+        print('OSError occurred, could not reach Gold.');
+      } on io.SocketException catch (_) {
+        print('SocketException occurred, could not reach Gold.');


These should do io.stderr.writeln, but as mentioned above, I think I'd prefer a hard failure in this case. Instead, if whether goldens are compared should be controlled by a command-line flag.

I don't think this line of code is the right place to do it, but I agree that we should fail if Skia Gold isn't available and we expect golden tests to run.

Right now, I don't want to introduce any failures caused by Skia Gold. In a future step, I'm going to start relying on Skia Gold to block PRs (until images are triaged). That's when I'm going to add the check to make sure Skia Gold is actually running when we expect it to.

SGTM. Let's file an issue and leave a TODO pointing to it.

Was there an issue filed & TODO added?

I updated the issue to list all steps required to finish this work.

web_sdk/web_test_utils/lib/image_compare.dart

web_sdk/web_test_utils/lib/skia_client.dart

lib/web_ui/dev/environment.dart

skia-gold · 2021-11-11T15:59:21Z

Gold has detected about 38 new digest(s) on patchset 17.
View them at https://flutter-engine-gold.skia.org/cl/github/29139

lib/web_ui/dev/steps/run_tests_step.dart

Piinks · 2021-11-11T19:47:55Z

web_sdk/web_test_utils/lib/image_compare.dart

+    // Skia Gold. The next step would be to actually use Skia Gold for
+    // comparison.
+
+    // TODO(mdebbar): Use Skia Gold for comparison, not only for uploading.


Might be good to add to this comment that when this is being used for comparison, it should account for any auto rollers, they will break if it fails in presubmit. We should be able to really easily add support of the flutter-gold check to manage checking in images when it's ready, just a small adjustment in cocoon. :)

1. Autorollers:

We plan to rely on the gold check for failures in pre-submit (we won't fail any tests even if the golden match fails). This comment/TODO was meant for local testing. Sorry that wasn't clear.

2. gold-check & cocoon:

Out of the box, we got those comments by the gold bot that provide triage links. I looked at the cocoon repo and found a couple places that need to be adjusted to work for the engine. Is there anything else we need to do (besides the cocoon change) to get the extra gold check to appear on engine PRs?

web_sdk/web_test_utils/lib/skia_client.dart

skia-gold · 2021-11-12T16:35:21Z

Gold has detected about 108 new digest(s) on patchset 20.
View them at https://flutter-engine-gold.skia.org/cl/github/29139

yjbanov

Piinks · 2021-11-15T20:07:48Z

@CaseyHillers I see you switched this PR to the main branch, but it looks like the bot switched it back to master, can you confirm this is ok to land and on which branch? Thanks very much! :)

mdebbar · 2021-11-15T20:10:44Z

@Piinks Thanks for noticing that! The merge button was actually disabled because the "master" branch is now protected (?). So I had to switch the base back to main in order to re-enable the merge button.

Piinks

This LGTM! Verified the engine instance of Gold received and has been approved with 540 images! Awesome! Can you cc me on the follow-up issue? Thanks!

Piinks · 2021-11-15T20:09:32Z

lib/web_ui/dev/steps/run_tests_step.dart

+      } on io.OSError catch (_) {
+        print('OSError occurred, could not reach Gold.');
+      } on io.SocketException catch (_) {
+        print('SocketException occurred, could not reach Gold.');


Was there an issue filed & TODO added?

flutter-dashboard bot added the platform-web Code specifically for the web engine label Oct 12, 2021

google-cla bot added the cla: yes label Oct 12, 2021

mdebbar mentioned this pull request Oct 13, 2021

[web] Add goldctl as a dependency in LUCI #29168

Merged

mdebbar force-pushed the skia_gold branch 4 times, most recently from 0d90c7b to 6cf5b52 Compare October 19, 2021 16:26

mdebbar added 4 commits November 2, 2021 13:44

[web] Wire Skia Gold for screenshot tests

5ead923

add print

3fb1b3f

print in bash file

804ace6

print more env variables

cd87aab

mdebbar force-pushed the skia_gold branch from 6cf5b52 to cd87aab Compare November 2, 2021 18:29

mdebbar added 2 commits November 3, 2021 11:29

more printing

c672cc3

fix working directory

7be523a

mdebbar added 7 commits November 3, 2021 13:50

Merge branch 'master' into skia_gold

5e44874

Merge branch 'master' into skia_gold

205e850

skip skia gold if it's not available (e.g. on windows)

c93a8e1

force post-submit mode so that goldens are committed

6746378

record browser name with goldens

2fe3d48

Merge branch 'master' into skia_gold

a0c0735

fix condition

81306b1

assert that goldctl is available

42f6a7b

mdebbar changed the title ~~Skia gold~~ [web] Start support for Skia Gold Nov 9, 2021

mdebbar marked this pull request as ready for review November 9, 2021 18:45

mdebbar requested a review from Piinks November 10, 2021 17:52

yjbanov reviewed Nov 11, 2021

View reviewed changes

mdebbar added 2 commits November 11, 2021 10:38

remove package:file and package:platform

264bd64

cleanup docs

3db0551

Piinks reviewed Nov 11, 2021

View reviewed changes

mdebbar added 3 commits November 11, 2021 16:35

formatting

824549f

rework initialization & authorization checks

66b39b0

Merge branch 'master' into skia_gold

0761583

mdebbar requested review from yjbanov and Piinks November 12, 2021 21:32

yjbanov approved these changes Nov 12, 2021

View reviewed changes

CaseyHillers changed the base branch from master to main November 15, 2021 18:14

fluttergithubbot force-pushed the master branch from b1a8771 to 2962099 Compare November 15, 2021 18:30

mdebbar changed the base branch from master to main November 15, 2021 20:09

mdebbar added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Nov 15, 2021

Piinks approved these changes Nov 15, 2021

View reviewed changes

fluttergithubbot merged commit cdcb49b into flutter:main Nov 16, 2021

engine-flutter-autoroll mentioned this pull request Nov 17, 2021

Roll Engine from 2962099077b3 to bba38c0b8312 (11 revisions) flutter/flutter#93759

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 17, 2021

cdcb49b [web] Start support for Skia Gold (flutter/engine#29139)

5645534

engine-flutter-autoroll mentioned this pull request Nov 17, 2021

Roll Engine from 2962099077b3 to ace1f5f2997b (13 revisions) flutter/flutter#93769

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 17, 2021

cdcb49b [web] Start support for Skia Gold (flutter/engine#29139)

71ff90c

This was referenced Nov 17, 2021

[web] Fail if Skia Gold is required but unavailable #29792

Merged

[web] Use Flutter Gold instead of flutter/goldens flutter/flutter#62706

Closed

mdebbar deleted the skia_gold branch January 17, 2023 18:32

[web] Start support for Skia Gold #29139

[web] Start support for Skia Gold #29139

Uh oh!

Conversation

mdebbar commented Oct 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdebbar commented Oct 19, 2021

Uh oh!

CaseyHillers commented Oct 19, 2021

Uh oh!

skia-gold commented Nov 3, 2021

Uh oh!

skia-gold commented Nov 9, 2021

Uh oh!

skia-gold commented Nov 9, 2021

Uh oh!

flutter-dashboard bot commented Nov 9, 2021

Uh oh!

skia-gold commented Nov 9, 2021

Uh oh!

Piinks commented Nov 11, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skia-gold commented Nov 11, 2021

Uh oh!

Uh oh!

Piinks Nov 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

1. Autorollers:

2. gold-check & cocoon:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skia-gold commented Nov 12, 2021

Uh oh!

yjbanov left a comment

Choose a reason for hiding this comment

Uh oh!

Piinks commented Nov 15, 2021

Uh oh!

mdebbar commented Nov 15, 2021

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mdebbar commented Oct 12, 2021 •

edited

Loading

Piinks Nov 11, 2021 •

edited

Loading