Skia Gold Support for Local & PreSubmit Testing in package:flutter by Piinks · Pull Request #40710 · flutter/flutter

Piinks · 2019-09-17T21:14:44Z

Description

This PR adds Local and PreSubmit testing support for Skia Gold, which was first landed as a post-submit comparator in #36103.

Original PR #39853
Design Doc: flutter.dev/go/golden-workflow.
Feedback is welcome here and in the design doc.

Needs to land first:

Backend support : Skia Gold support cocoon#409
- Landed, ~~needs deployment~~
Update wiki
- Written, waiting to land to make live :)
Add benchmark for un-triaged digests on Flutter Gold
- ~~Opened PR here: Benchmark for un-triaged image results on Flutter Gold #40634~~

Tests

Many.

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.

.cirrus.yml

bin/internal/goldens.version

Piinks · 2019-09-19T20:10:57Z

Marking this WIP again for some changes after some offline feedback from @kjlubick. 👍

goderbauer

I am missing an overview somewhere in the docs that says: These are the three cooperators we have and this is how they differ.

packages/flutter_goldens/lib/flutter_goldens.dart

packages/flutter_goldens_client/lib/skia_client.dart

goderbauer

LGTM

Piinks · 2019-10-17T16:56:02Z

FYI @kjlubick this is ready to land. :)

Piinks · 2019-10-18T20:21:24Z

@goderbauer I have made 2 changes here.

due to API behavior I had not accounted for when outputting test failures. It incorporates the image hash in the output so if there have been a lot of recent changes to a particular file, they can be differentiated.
I have also removed the output clean-up, as I have found it does not work as intended. Since multiple processes are running tests, one process can end up deleting the output of another. I have not yet found a way to safely delete the output without exposing such a nuclear option publicly to other developers running golden file tests. Since output is only generated when making a change, which is not often, I think this is ok for now.

I would like to follow-up on these in later (smaller) PRs. I will already be revisiting to incorporate new features from Skia Gold as they become available. WDYT?

goderbauer

Those changes sound good.

Piinks · 2019-10-21T19:51:57Z

I will land this once the tree is green. I already have the wiki update queued for when it lands. :)

…utter (#40710)" This reverts commit 8df0d65.

…utter (#40710)" (#43227) This reverts commit 8df0d65.

Piinks · 2019-10-22T15:41:01Z

packages/flutter_goldens_client/lib/skia_client.dart

+    String rawResponse;
+    await io.HttpOverrides.runWithHttpOverrides<Future<void>>(() async {
+      final Uri requestForDigest = Uri.parse(
+        'https://flutter-gold.skia.org/json/details?test=$testName&digest=$expectation'


FYI @kjlubick This appears to be the request that was timing out.

Alright, that also is prone to the 5 QPS limit atm. I'll dig in a bit and see what we can do about it before our discussion tomorrow.

Actually, that one is on a rate limit that is way under 5QPS, but measurements show that it might not need to be.

https://bugs.chromium.org/p/skia/issues/detail?id=9557 for tracking Gold side changes

…lutter#40710)

…utter (flutter#40710)" (flutter#43227) This reverts commit 8df0d65.

Piinks added 4 commits September 17, 2019 12:56

Merged original PR with changes from master

a9afca5

++

fabfd1c

Ignored test integration

ee47e61

++

f711147

fluttergithubbot added a: tests "flutter test", flutter_test, or one of our tests f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Sep 17, 2019

googlebot added the cla: yes label Sep 17, 2019

Updating tests

dc5cb46

Piinks removed f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. labels Sep 17, 2019

Piinks added 3 commits September 17, 2019 15:21

Added testing templates for mocks

915f7e8

++ on tests

b68213c

Tests

a271107

Piinks added the work in progress; do not review label Sep 18, 2019

Piinks added 2 commits September 18, 2019 16:22

Completed tests

ce91021

Adding documentation

4571a39

Piinks removed the work in progress; do not review label Sep 18, 2019

Piinks commented Sep 19, 2019

View reviewed changes

.cirrus.yml Show resolved Hide resolved

Piinks commented Sep 19, 2019

View reviewed changes

bin/internal/goldens.version Outdated Show resolved Hide resolved

Piinks added 2 commits September 18, 2019 17:05

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

53a0ea5

Missed docs

3ed223f

Piinks added the work in progress; do not review label Sep 19, 2019

goderbauer reviewed Sep 19, 2019

View reviewed changes

Piinks mentioned this pull request Sep 20, 2019

Benchmark for un-triaged image results on Flutter Gold #40634

Merged

8 tasks

Piinks added 3 commits September 24, 2019 14:33

++

33889ef

Changing endpoints, updated tests.

1e166ae

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

13554da

Analyzer therapy

40a0881

Piinks added a: images Loading, displaying, rendering images c: contributor-productivity Team-specific productivity, code health, technical debt. labels Oct 10, 2019

goderbauer reviewed Oct 10, 2019

View reviewed changes

Piinks added 3 commits October 10, 2019 17:15

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

9b8f7b6

Adding clean up of old test results at comparator init

0a79aa4

Review Feedback

da99d29

goderbauer approved these changes Oct 11, 2019

View reviewed changes

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

953b1ef

Piinks added 2 commits October 18, 2019 10:58

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

a1d6039

Updating test output

973f7ac

fluttergithubbot added f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. c: API break Backwards-incompatible API changes labels Oct 18, 2019

Piinks removed f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. labels Oct 21, 2019

goderbauer approved these changes Oct 21, 2019

View reviewed changes

Piinks merged commit 8df0d65 into flutter:master Oct 21, 2019

Piinks added a commit that referenced this pull request Oct 22, 2019

Revert "Skia Gold Support for Local & PreSubmit Testing in package:fl…

de911e8

…utter (#40710)" This reverts commit 8df0d65.

Piinks mentioned this pull request Oct 22, 2019

Revert "Skia Gold Support for Local & PreSubmit Testing in package:flutter" #43227

Merged

Piinks added a commit that referenced this pull request Oct 22, 2019

Revert "Skia Gold Support for Local & PreSubmit Testing in package:fl…

3a3939a

…utter (#40710)" (#43227) This reverts commit 8df0d65.

Piinks commented Oct 22, 2019

View reviewed changes

Piinks mentioned this pull request Oct 23, 2019

Re-land Local & Pre-Submit Support for Skia Gold #43371

Merged

9 tasks

Piinks deleted the flutterGold branch November 22, 2019 00:58

Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019

Skia Gold Support for Local & PreSubmit Testing in package:flutter (f…

6ca0cd1

…lutter#40710)

Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019

Revert "Skia Gold Support for Local & PreSubmit Testing in package:fl…

daa3170

…utter (flutter#40710)" (flutter#43227) This reverts commit 8df0d65.

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

Conversation

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

Uh oh!

Description

Tests

Checklist

Breaking Change

Uh oh!

Uh oh!

Uh oh!

Piinks commented Sep 19, 2019

Uh oh!

goderbauer left a comment

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!

Uh oh!

Uh oh!

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Piinks commented Oct 17, 2019

Uh oh!

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

Uh oh!

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Piinks commented Oct 21, 2019

Uh oh!

Piinks Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

kjlubick Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

kjlubick Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

kjlubick Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Piinks commented Sep 17, 2019 •

edited

Loading

Piinks commented Oct 18, 2019 •

edited

Loading