Use skia golden files in driver test #49750

blasten · 2020-01-30T00:17:06Z

Description

Use Skia golden files in driver test, so we don't need to check in binaries into this repo. To make this happen, I removed the dependency on dart:ui from flutter_test/lib/src/goldens.dart and flutter_test/lib/src/_goldens_io.dart since dart:ui isn't available in the vm.

I filed #49749 to track access to the Skia gold service from LUCI.

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 read the Tree Hygiene wiki page, which explains my responsibilities.
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

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: Replace with a link to your migration guide

jonahwilliams · 2020-01-30T00:51:03Z

packages/flutter_test/lib/src/goldens.dart

  /// may load files from the local file system, whereas others may load files
  /// over the network or from a remote repository.
-  Future<bool> compare(Element element, Size size, Uri golden);
+  Future<bool> compare(dynamic element, dynamic size, Uri golden);


Though its useful for debugging, I don't think this is a good long-term change since we lose type safety on the APIs. I would recommend exploring how you can expose the same logic without changing the existing APIs

maybe instead of having conditional import in this file, it's really up to the caller to either import web or io. That will remove this problem too. WDYT?

Piinks · 2020-01-30T18:44:03Z

Taking a look now, I've updated #49749 regarding authenticating Gold in the Luci environment, goldctl has a luci auth method that does not require a service account.

fyi @kjlubick :)

Piinks · 2020-01-30T19:04:14Z

This is going to be great to have tests running on device. Thank you! 🙂 These driver tests are executed in post-submit, right?

I am curious about a few things here. First, I'd like to check which comparator is being used when these run. Right now there are couple of different comparators that may or may not be using Gold based on where/when the test is running. See

flutter/packages/flutter_goldens/lib/flutter_goldens.dart

Line 31 in 9375377

if (FlutterSkiaGoldFileComparator.isAvailableForEnvironment(platform)) {

Second, right now the skia gold/post-submit comparator will not return a negative test result, since we don't want to turn the tree red just for an intentional change.

flutter/packages/flutter_goldens_client/lib/skia_client.dart

Line 257 in 9375377

return true;

(I think this can actually be changed now since we are checking in golden files in pre-submit with tryjobs.)

If a change lands on master for these driver tests, what do we want to have happen? We won't know about it until it lands, because we aren't running pre-submit checks, so do we want the tree to turn red? Do we want to leave it green, but receive a comment on the PR that landed the change?

Before we decided to check in files in pre-submit, I had considered adding a benchmark for un-triaged results on master to bring them to our attention when they land. WDYT?

blasten · 2020-01-30T19:36:44Z

Thanks for the summary @Piinks!

My understanding is that we will start running presubmit tests on LUCI. (Some tests currently run on LUCI if someone adds the tag CQ+1 to a PR)

That said, I think it would make sense to follow the same procedure as any other golden test.

In the meanwhile, (for testing only) I can manually update the golden files.

wdyt?

Piinks · 2020-01-30T19:40:46Z

That SGTM. :) If we are getting ready to make the switch to luci for pre-submit, we'll just need to make a few changes to the cirrus-specific aspects of the goldenFileComparator. IIRC, right now for luci it just defers to the local comparator. I'm looking into some of that now, maybe we can touch base when I am back in-office next week? :)

blasten · 2020-01-30T19:44:42Z

Perfect. In the meanwhile, would you mind reviewing this PR, so once the changes land we can start testing the LUCI flow? I can manually update the golden files from a LUCI machine.

Piinks · 2020-01-30T19:57:26Z

packages/flutter_test/lib/src/buffer_matcher.dart

+      return null;
+    }
+    try {
+      final bool success = await goldenFileComparator.compare(buffer, testNameUri);


You mentioned handling this manually until the client is ready for luci testing. It looks like in post-submit on luci this comparison is a no-op:

flutter/packages/flutter_goldens/lib/flutter_goldens.dart

Line 489 in 0be9127

return platform.environment.containsKey('SWARMING_TASK_ID')

Yep. I can definitely follow up with that based on the discussion on #49749.

Ok. Is this an already existing test? If so, idk if changing it to a no-op test in the interim is a good idea.

Piinks · 2020-01-30T19:58:44Z

Also, is goldctl available on luci? Currently we download the binary for Cirrus testing on mac and windows, and it is in the docker file for linux tests.

kjlubick · 2020-01-30T20:04:52Z

Yes, goldctl is available from CIPD https://chrome-infra-packages.appspot.com/p/skia/tools/goldctl

Piinks

The larger re-factoring LGTM, but I don't know if we want to temporarily skip the actual test that this is being refactored for while we circle back to update the skia client/comparator.

Piinks · 2020-01-30T20:39:20Z

packages/flutter_test/lib/src/buffer_matcher.dart

+      return null;
+    }
+    try {
+      final bool success = await goldenFileComparator.compare(buffer, testNameUri);


Ok. Is this an already existing test? If so, idk if changing it to a no-op test in the interim is a good idea.

Piinks

Touched base with @blasten offline regarding the no-op tests. The pre-existing one will be restored and the new one will be no-op while we stand up the luci support to connect it to.

dev/integration_tests/flutter_driver_screenshot_test/test_driver/main_test.dart

blasten · 2020-01-30T22:49:08Z

@Piinks PTAL

Piinks

LGTM! :)

Piinks · 2020-01-31T17:06:57Z

dev/integration_tests/flutter_driver_screenshot_test/README.md


-The test currently only runs on device lab ["mac/ios"] which runs the app on iPhone 6s.
+* Device Lab which runs the app on iPhone 6s.
+* LUCI which runs the app on a Fuchsia NUC device.


Just a note to remember as we make the follow-up changes to this, we should see if adding device and environment parameters from these test environments will be beneficial for indexing device lab tests on Gold.
e.g.

flutter/packages/flutter_goldens_client/lib/skia_client.dart

Line 584 in effc019

/// Returns a JSON String with keys value pairs used to uniquely identify the

I may incorporate it into #49815

This reverts commit a50743f.

Use skia golden files in driver test

a8c239c

blasten requested review from Piinks and jonahwilliams January 30, 2020 00:17

fluttergithubbot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Jan 30, 2020

googlebot added the cla: yes label Jan 30, 2020

jonahwilliams reviewed Jan 30, 2020

View reviewed changes

blasten force-pushed the skia_gold_driver_test branch from badc669 to 60b2455 Compare January 30, 2020 01:52

blasten requested a review from jonahwilliams January 30, 2020 01:53

Refactor code and put types back in WebGoldenComparator

4223e89

blasten force-pushed the skia_gold_driver_test branch from 60b2455 to 4223e89 Compare January 30, 2020 01:54

Hide TypeMatcher and isInstanceOf from test/test.dart

64288fa

Piinks mentioned this pull request Jan 30, 2020

Refactoring Gold to enable both Luci & Cirrus support #49815

Merged

9 tasks

Piinks reviewed Jan 30, 2020

View reviewed changes

blasten requested a review from Piinks January 30, 2020 20:31

Piinks reviewed Jan 30, 2020

View reviewed changes

dev/integration_tests/flutter_driver_screenshot_test/test_driver/main_test.dart Outdated Show resolved Hide resolved

Emmanuel Garcia added 3 commits January 30, 2020 14:17

Continue running iOS test on the device lab

e9b78c8

Add TODO

de6fd69

Enable Flutter Gold in Fuchsia driver test

cb1b6ee

blasten requested a review from Piinks January 30, 2020 22:49

Piinks approved these changes Jan 31, 2020

View reviewed changes

blasten merged commit a50743f into flutter:master Jan 31, 2020

blasten deleted the skia_gold_driver_test branch January 31, 2020 19:05

blasten restored the skia_gold_driver_test branch January 31, 2020 19:28

blasten deleted the skia_gold_driver_test branch January 31, 2020 19:28

blasten pushed a commit that referenced this pull request Jan 31, 2020

Revert "Use skia golden files in driver test (#49750)"

7dc68ab

This reverts commit a50743f.

blasten mentioned this pull request Jan 31, 2020

Revert "Use skia golden files in driver test" #49900

Merged

blasten pushed a commit that referenced this pull request Jan 31, 2020

Revert "Use skia golden files in driver test (#49750)" (#49900)

1eb5bb6

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

Use skia golden files in driver test #49750

Use skia golden files in driver test #49750

Uh oh!

Conversation

blasten commented Jan 30, 2020

Description

Checklist

Breaking Change

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 commented Jan 30, 2020

Uh oh!

Piinks commented Jan 30, 2020

Uh oh!

blasten commented Jan 30, 2020

Uh oh!

Piinks commented Jan 30, 2020

Uh oh!

blasten commented Jan 30, 2020

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 commented Jan 30, 2020

Uh oh!

kjlubick commented Jan 30, 2020

Uh oh!

Piinks left a comment

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!

blasten commented Jan 30, 2020

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants