Part 1: Skia Gold Testing #33688

Piinks · 2019-05-31T23:45:29Z

Description

Integrating Flutter golden file testing with Skia Gold
Re-worked into a phased transition from #31630

This PR keeps in place the current structure of Gold file testing via the flutter/goldens repository for local and pre-submit testing, while Skia Gold is used for post-submit testing.

Golden tests in this implementation will not turn the tree red. Instead, once this lands and is validated by CI on post-submit, commenting on associated PR's will be added as a method of notification.

Once Pre-submit testing is enabled by the Skia Gold team, the former testing structure used through flutter/goldens will be removed, with Skia Gold taking its place.

Related Issues

Dependent on:

~~Add web safe indirection to Platform.isPlatform getters #33406~~
~~Splitting golden file versioning out as an argument of matchesGoldenFile #33880~~
~~Correct version name for BottomNavigationBar golden test #33865~~
~~Check for duplicate Golden File names #33806~~ - will implement later through use of the analyzer. Testing in Skia Gold will incorporate a pre-fix naming convention for all tests based on library so as to reduce the chance of a naming conflict.

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?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

Piinks · 2019-06-04T22:35:12Z

Waiting on #33880 and #33865 to land and update this PR.

…tion for Skia Gold test names.

Piinks

Ready for another go.

.cirrus.yml

packages/flutter_goldens/lib/flutter_goldens.dart

Piinks · 2019-07-10T19:21:26Z

packages/flutter_goldens_client/lib/client.dart

-/// repository, nested within the `bin/cache` directory of the caller's Flutter
-/// repository.
+/// A base class that provides shared information to the [SkiaGoldClient] and
+/// [GoldensRepositoryClient].


Can do & done! :)

packages/flutter_goldens_client/lib/client.dart

packages/flutter_goldens_client/lib/skia_client.dart

packages/flutter_goldens/lib/flutter_goldens.dart

dev/bots/download_goldctl.ps1

packages/flutter_goldens/lib/flutter_goldens.dart

goderbauer · 2019-07-10T22:57:57Z

packages/flutter_goldens_client/lib/client.dart

-/// repository, nested within the `bin/cache` directory of the caller's Flutter
-/// repository.
+/// A base class that provides shared information to the [SkiaGoldClient] and
+/// [GoldensRepositoryClient].


GoldensClient is still not abstract.

packages/flutter_test/lib/src/goldens.dart

goderbauer · 2019-07-10T23:00:05Z

packages/flutter_test/lib/src/goldens.dart


+  @override
+  Uri getTestUri(Uri key, int version) {
+    if (version == null)


Any way we can avoid the code duplication of this method?

I think I found a way. :)

tvolkert

LGTM.

@goderbauer will probably want to review and approve as well.

packages/flutter_goldens/lib/flutter_goldens.dart

tvolkert · 2019-07-10T23:00:47Z

packages/flutter_goldens/lib/flutter_goldens.dart

+    fs: fs,
+    platform: platform,
+  ) {
+    _skiaClient = SkiaGoldClient(fs.directory(basedir));


This could be set in the initializer as well and made final

I don't know that I can do that since the SkiaGoldClient is not a const constructor. cc/ @goderbauer for offline chat.

tvolkert · 2019-07-10T23:04:17Z

packages/flutter_goldens/lib/flutter_goldens.dart

-    await goldenFile.writeAsBytes(imageBytes, flush: true);
+  Future<bool> compare(Uint8List imageBytes, Uri golden) async {
+    golden = _addPrefix(golden);
+    await update(golden, imageBytes);


How come we update() during compare()?

Is it that we update the version in Gold that's associated with this test run whenever we compare, and that's separate from approving which is the new golden?

We update to create the golden file that will be uploaded to Skia.

The master golden file is never downloaded or stored locally for comparison like in the flutter/goldens implementation, so the only file that exists is the current golden that needs to be compared. So when compare() is called, is calls update() to write out the bytes to file for the test.

Piinks

Addressed comments, will run another round of testing across all environments tomorrow.

dev/bots/download_goldctl.ps1

Piinks · 2019-07-10T23:36:25Z

packages/flutter_goldens/lib/flutter_goldens.dart

-    await goldenFile.writeAsBytes(imageBytes, flush: true);
+  Future<bool> compare(Uint8List imageBytes, Uri golden) async {
+    golden = _addPrefix(golden);
+    await update(golden, imageBytes);


We update to create the golden file that will be uploaded to Skia.

The master golden file is never downloaded or stored locally for comparison like in the flutter/goldens implementation, so the only file that exists is the current golden that needs to be compared. So when compare() is called, is calls update() to write out the bytes to file for the test.

Piinks · 2019-07-10T23:40:30Z

packages/flutter_goldens_client/lib/client.dart

-/// repository, nested within the `bin/cache` directory of the caller's Flutter
-/// repository.
+/// A base class that provides shared information to the [SkiaGoldClient] and
+/// [GoldensRepositoryClient].


It can't be abstract since it is called in fromDefaultComparator by the FlutterSkiaGoldFileComparator to access the base information.

Piinks · 2019-07-10T23:55:04Z

packages/flutter_test/lib/src/goldens.dart


+  @override
+  Uri getTestUri(Uri key, int version) {
+    if (version == null)


I think I found a way. :)

Piinks · 2019-07-11T00:01:16Z

packages/flutter_goldens/lib/flutter_goldens.dart

+    fs: fs,
+    platform: platform,
+  ) {
+    _skiaClient = SkiaGoldClient(fs.directory(basedir));


I don't know that I can do that since the SkiaGoldClient is not a const constructor. cc/ @goderbauer for offline chat.

goderbauer

LGTM

packages/flutter_goldens/lib/flutter_goldens.dart

goderbauer · 2019-07-11T01:29:51Z

packages/flutter_goldens_client/lib/client.dart

-/// repository, nested within the `bin/cache` directory of the caller's Flutter
-/// repository.
+/// A base class that provides shared information to the [SkiaGoldClient] and
+/// [GoldensRepositoryClient].


You never seem to instantiate a GoldensClient, though. Only its subclasses get instantiated, so you should be able to still mark this class as abstract.

goderbauer

LGTM

goderbauer · 2019-07-11T20:49:33Z

packages/flutter_goldens_client/lib/skia_client.dart

  /// informed by the basedir of the [FlutterSkiaGoldFileComparator].
-  Future<void> auth() async {
+  Future<void> auth(Directory workDirectory) async {
+    assert(workDirectory != null);


Maybe add an error message here indicating that auth can only be called once?

I don't know that I would want to throw an error if it is already authorized, I just made it a no-op in that case.

Piinks · 2019-07-12T16:30:57Z

tool_test_integration-{platform} presubmit tests have been timing out. I'm going to update the branch again and see if it resolves this. Just waiting on these tests to land this change.

tvolkert · 2019-07-12T20:01:48Z

FYI for future reference: https://github.com/flutter/flutter/wiki/Tree-hygiene#squashing-commits

(as seen in eb0b179)

This reverts commit eb0b179.

This reverts commit eb0b179. Skia Gold post-submit test were failing due to the service account not being found on Cirrus.

* Fresh PR for Gold integration. * Nits * WIP * Artifacts from merge * Changed some platform dependencies for web, added library prefix notation for Skia Gold test names. * Updating for CI implementation * Write out service account * Writing to skip out * WIP * ++ * Fixing depot tools deps * Windows depot_tools * Fixing setup scripts * ++ * depot tools * ++ * WIP * Tracing depot_tools clone * WIP * ++ * analyzer * WIP * chrome typo * copy artifact * Working on tests * Code cleanup * ++ * Code cleanup, updated tests * ++ review feedback * Review * Analyzer * Review feedback * Nits from review * PRogress * ++ * Fixing tests * ++ * Testing repo route * Just needing documention around new structures. * cleanup * Analyzer * Documentation updates * Documentation updates * Cirrus updates * cirrus nit * Review feedback * Review feedback * Fixing skip comparator * Fix base directory for Skia Gold case * ++ * Feedback * ++ * Fixed uri assertion * Made GoldensClient abstract, altered SkiaGoldClient constructor * Analyzer

This reverts commit eb0b179. Skia Gold post-submit test were failing due to the service account not being found on Cirrus.

Piinks added 2 commits May 31, 2019 16:19

Fresh PR for Gold integration.

02af1e7

Nits

0fe9fed

Piinks added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. labels May 31, 2019

googlebot added the cla: yes label May 31, 2019

Piinks changed the title ~~WIP - Part1: Skia Gold Testing~~ WIP - Part 1: Skia Gold Testing May 31, 2019

This comment has been minimized.

Sign in to view

WIP

7e64978

goderbauer added the work in progress; do not review label Jun 3, 2019

Piinks mentioned this pull request Jun 7, 2019

Prevent duplicate golden file test names #34064

Closed

9 tasks

Piinks added the waiting for PR to land (fixed) A fix is in flight label Jun 7, 2019

Piinks mentioned this pull request Jun 10, 2019

[WIP] Golden testing for flutter web #34117

Closed

Piinks self-assigned this Jun 12, 2019

Piinks added 10 commits June 14, 2019 14:17

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

Piinks added 4 commits July 10, 2019 12:55

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

ebed961

Fixing skip comparator

9e5a850

Fix base directory for Skia Gold case

afa0fd4

++

45a2403

Piinks commented Jul 10, 2019

View reviewed changes

goderbauer reviewed Jul 10, 2019

View reviewed changes

tvolkert approved these changes Jul 10, 2019

View reviewed changes

Feedback

1c599c4

Piinks commented Jul 11, 2019

View reviewed changes

goderbauer approved these changes Jul 11, 2019

View reviewed changes

Piinks added 4 commits July 11, 2019 09:42

++

2c6a288

Fixed uri assertion

ad3f007

Made GoldensClient abstract, altered SkiaGoldClient constructor

91bca34

Analyzer

5c12876

goderbauer approved these changes Jul 11, 2019

View reviewed changes

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

e1aa133

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

f6b5fdf

Piinks merged commit eb0b179 into flutter:master Jul 12, 2019

Piinks added a commit that referenced this pull request Jul 12, 2019

Revert "Part 1: Skia Gold Testing (#33688)"

8144e4f

This reverts commit eb0b179.

Piinks mentioned this pull request Jul 12, 2019

Revert "Part 1: Skia Gold Testing" #36094

Merged

Piinks added a commit that referenced this pull request Jul 12, 2019

Revert "Part 1: Skia Gold Testing (#33688)" (#36094)

8809f69

This reverts commit eb0b179. Skia Gold post-submit test were failing due to the service account not being found on Cirrus.

Piinks mentioned this pull request Jul 12, 2019

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

Merged

8 tasks

This was referenced Jul 19, 2019

Do Not Merge - Testing CI Goldens Issue #36573

Closed

Updating cirrus fingerprint script to include goldens version #36690

Merged

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

Revert "Part 1: Skia Gold Testing (flutter#33688)" (flutter#36094)

9d19e7d

This reverts commit eb0b179. Skia Gold post-submit test were failing due to the service account not being found on Cirrus.

Piinks deleted the goldIntegration branch November 22, 2019 00:53

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

Part 1: Skia Gold Testing #33688

Part 1: Skia Gold Testing #33688

Uh oh!

Conversation

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

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

This comment has been minimized.

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

Uh oh!

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

Piinks left a comment

Choose a reason for hiding this comment

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!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer 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!

Piinks commented May 31, 2019 •

edited

Loading

Piinks commented Jun 4, 2019 •

edited

Loading