Re-land "Part 1: Skia Gold Testing" by Piinks · Pull Request #36103 · flutter/flutter

Piinks · 2019-07-12T21:49:36Z

Description

New PR to re-land reverted changes from #33688

Current State

Ready for review, changes from #33688 have been notated.

In a previous state of this PR, Cirrus behavior was validated by mocking post-submit conditions.

Issues resolved from original PR:

Windows test shards were not executing, which has been resolved by Fix windows, exclude widgets from others #36197 and Fix test_widgets-windows not running tests #36468
The encrypted service account was returning null in the Cirrus environment for some shards
Local changes were introduced to the goldens repo on Cirrus, which blocked further update, fixed by Updating cirrus fingerprint script to include goldens version #36690
The mac instances on Cirrus appear to have a time issue that is breaking logic for authenticating with Google Cloud - this can be fixed by ensuring that the Gold related shards use the mac image that has xcode 10.2
The powershell script failed to install the goldctl tool for the windows instances on Cirrus, it has been fixed here.

Notes:

Results from Gold have been sporadically returning exceptions from the service. It appears these flakes have been resolved, but will require validation. As such, exceptions have been disabled for the skia gold client.

Related Issues

Addresses:

Tests

Added/Relocated/Updated:

flutter_test/test/matchers_test.dart
- Comparator succeeds in incorporating version number
- Comparator succeeds with null version number
- Comparator failure incorporates version number
- Comparator failure with null version number
flutter_test/test/goldens_test.dart
- getTestUri updates file name with version number
- getTestUri does nothing for null version number
flutter_goldens/test/flutter_goldens_test.dart
- group: GoldensClient
- group: SkiaGoldClient
  - auth performs minimal work if already authorized
- group: FlutterGoldensRepositoryFileComparator
  - fromDefaultComparator calculates the base directory correctly
  - group: getTestUri
    - incorporates version number
    - ignores null version number
- group: FlutterSkiaGoldFileComparator
  - fromDefaultComparator calculates the base directory correctly
  - group: getTestUri
    - ignores version number
Any and all tests that have invoked matchesGoldenFile with a skip in place for platforms other than Linux.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

No, this is not a breaking change.

…tion for Skia Gold test names.

… may need to move to xcode10.1 shard

Piinks

I've notated the lines that have changed (and why) from the original change that was reverted, in the hopes that it will make for easier review.
cc/ @goderbauer @dnfield @tvolkert

Piinks · 2019-07-25T20:01:54Z

.cirrus.yml

  use_compute_credits: $CIRRUS_USER_COLLABORATOR == 'true'
  osx_instance:
-    image: mojave-xcode-10.1
+    image: mojave-xcode-10.2


Fixes time drift on Cirrus.

Piinks · 2019-07-25T20:02:44Z

.cirrus.yml

      test_all_script:
        - ulimit -S -n 2048 # https://github.com/flutter/flutter/issues/2976
        - dart --enable-asserts dev/bots/test.dart
+      on_failure:


Added to help identify time drift if it should happen again. Time issues are referenced at the top of this shard.

Piinks · 2019-07-25T20:02:50Z

.cirrus.yml

      test_all_script:
        - ulimit -S -n 2048 # https://github.com/flutter/flutter/issues/2976
        - dart --enable-asserts dev/bots/test.dart
+      on_failure:


Added to help identify time drift if it should happen again. Time issues are referenced at the top of this shard.

Piinks · 2019-07-25T20:04:00Z

dev/bots/download_goldctl.ps1

@@ -0,0 +1,14 @@
+$url= "https://storage.googleapis.com/chrome-infra/depot_tools.zip"


Updated to work on Cirrus. Verified in earlier commit by using a mock-post-submit state in pre-submit to run Skia tests.

Piinks · 2019-07-25T20:11:18Z

packages/flutter_goldens/lib/flutter_goldens.dart

+    final String cirrusCI = platform.environment['CIRRUS_CI'] ?? '';
+    final String cirrusPR = platform.environment['CIRRUS_PR'] ?? '';
+    final String cirrusBranch = platform.environment['CIRRUS_BRANCH'] ?? '';
+    final String goldServiceAccount = platform.environment['GOLD_SERVICE_ACCOUNT'] ?? '';


Added the availability of the service account as a condition for using the FlutterSkiaGoldFileComparator. There are some shards that meet the first three conditions, but are not meant for executing skia gold testing.

Piinks · 2019-07-25T20:12:29Z

packages/flutter_goldens_client/lib/skia_client.dart

+  String get _serviceAccount => platform.environment[_kServiceAccountKey];
+
+  @override
+  Directory get comparisonRoot => flutterRoot.childDirectory(fs.path.join('bin', 'cache', 'pkg', 'skia_goldens'));


Rather than putting the generated images in the same folder 'goldens', created a separate one for skia files.

Piinks · 2019-07-25T20:13:36Z

packages/flutter_goldens_client/lib/skia_client.dart

+      _goldctl,
+      authArguments,
+    );
+    // TODO(Piinks): Re-enable after Gold flakes are resolved, https://github.com/flutter/flutter/pull/36103


Flaky behavior from the skia gold service needs to be resolved before these exceptions can be enabled.

Piinks · 2019-07-25T20:13:51Z

packages/flutter_goldens_client/lib/skia_client.dart

+      imgtestInitArguments,
+    );
+
+    // TODO(Piinks): Re-enable after Gold flakes are resolved, https://github.com/flutter/flutter/pull/36103


Flaky behavior from the skia gold service needs to be resolved before these exceptions can be enabled.

tvolkert

Thanks for pointing out the differences. LGTM!

dnfield

LGTM.

goderbauer

LGTM

Piinks · 2019-07-28T19:25:50Z

The linux integration test states it is queued, but going into Cirrus it is all green.

Piinks added 30 commits May 31, 2019 16:19

Fresh PR for Gold integration.

02af1e7

Nits

0fe9fed

WIP

7e64978

Merge branch 'master' of github.com:flutter/flutter into goldIntegration

082c587

Artifacts from merge

7823af7

Changed some platform dependencies for web, added library prefix nota…

4f54ac8

…tion for Skia Gold test names.

Updating for CI implementation

45753ac

Write out service account

e94efb1

Writing to skip out

a0a83ed

WIP

5df4bf6

++

11ec09d

Fixing depot tools deps

0a67433

Windows depot_tools

783cc95

Fixing setup scripts

14c2d0e

++

9d10b57

depot tools

ff07c1a

++

59f1738

WIP

f447a9b

Tracing depot_tools clone

cdc8d1a

WIP

58a38ba

++

ab5fd88

analyzer

3c6d409

Merge branch 'master' of github.com:flutter/flutter into goldIntegration

9a7ca23

WIP

a860418

chrome typo

2afdad4

copy artifact

e569151

Merge branch 'master' of github.com:flutter/flutter into goldIntegration

cb9c115

Working on tests

7bd02fc

Code cleanup

0e18e7e

++

0c662a4

Piinks added 12 commits July 24, 2019 10:22

Changing default encoding for Out-File in windows script

847488f

++

0d2f58a

Checking ensure file encoding, getting output from passed tests

5484965

Fixing windows ensure file

48b86df

++

6c6519d

Code cleanup from windows script.

0650440

Final mock post-submit check

c32872e

Disabling exception due to gold flakes.

ffb3926

Re-enabling other shards with mock-post-submit in place. Add2App test…

d69be59

… may need to move to xcode10.1 shard

Merge branch 'master' of github.com:flutter/flutter into relandGold

76b32ba

Analyzer enforces TODO // location.

4e5ca82

Removing mock-post-submit state. Code clean-up.

221efd3

Piinks commented Jul 25, 2019

View reviewed changes

Piinks removed the work in progress; do not review label Jul 25, 2019

Piinks changed the title ~~WIP Re-land "Part 1: Skia Gold Testing"~~ Re-land "Part 1: Skia Gold Testing" Jul 25, 2019

Piinks requested review from dnfield, goderbauer and tvolkert July 25, 2019 20:15

tvolkert approved these changes Jul 25, 2019

View reviewed changes

dnfield approved these changes Jul 25, 2019

View reviewed changes

Piinks added the waiting for tree to go green label Jul 25, 2019

goderbauer approved these changes Jul 26, 2019

View reviewed changes

Piinks merged commit 616794f into flutter:master Jul 28, 2019

Piinks mentioned this pull request Jul 30, 2019

Removing Leftover Golden Test Skips #37250

Merged

8 tasks

johnsonmh pushed a commit to johnsonmh/flutter that referenced this pull request Jul 30, 2019

Re-land "Part 1: Skia Gold Testing" (flutter#36103)

297daef

Piinks mentioned this pull request Aug 13, 2019

Comparing pixels instead of bytes with GoldenFileComparator #38473

Merged

8 tasks

Piinks mentioned this pull request Sep 19, 2019

Skia Gold Support for Local & PreSubmit Testing in package:flutter #40710

Merged

11 tasks

Piinks deleted the relandGold branch November 22, 2019 00:52

github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2021

		@@ -0,0 +1,14 @@
		$url= "https://storage.googleapis.com/chrome-infra/depot_tools.zip"

Conversation

Piinks commented Jul 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Current State

Issues resolved from original PR:

Notes:

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

Piinks left a comment

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!

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!

tvolkert left a comment

Choose a reason for hiding this comment

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Piinks commented Jul 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Piinks commented Jul 12, 2019 •

edited

Loading