Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
Piinks merged 27 commits into from
Oct 21, 2019
Merged

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

Piinks merged 27 commits into from
Oct 21, 2019

Conversation

@Piinks
Copy link
Contributor

@Piinks Piinks commented Sep 17, 2019 •
edited
Loading

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:

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.

@fluttergithubbot 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
@Piinks Piinks removed f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. labels Sep 17, 2019
Piinks
Piinks commented Sep 19, 2019
View reviewed changes
Piinks
Piinks commented Sep 19, 2019
View reviewed changes
@Piinks
Copy link
Contributor Author

Piinks commented Sep 19, 2019

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

goderbauer
goderbauer reviewed Sep 19, 2019
View reviewed changes
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Piinks Piinks mentioned this pull request Sep 20, 2019
Merged
8 tasks
@Piinks Piinks added a: images Loading, displaying, rendering images c: contributor-productivity Team-specific productivity, code health, technical debt. labels Oct 10, 2019
goderbauer
goderbauer reviewed Oct 10, 2019
View reviewed changes
goderbauer
goderbauer approved these changes Oct 11, 2019
View reviewed changes
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Piinks
Copy link
Contributor Author

Piinks commented Oct 17, 2019

FYI @kjlubick this is ready to land. :)

@Piinks
Copy link
Contributor Author

Piinks commented Oct 18, 2019 •
edited
Loading

@goderbauer I have made 2 changes here.

  1. 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.

  2. 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?

@fluttergithubbot 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 Piinks removed f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. labels Oct 21, 2019
goderbauer
goderbauer approved these changes Oct 21, 2019
View reviewed changes
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those changes sound good.

@Piinks
Copy link
Contributor Author

Piinks commented Oct 21, 2019

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

@Piinks Piinks merged commit 8df0d65 into flutter:master Oct 21, 2019
Piinks added a commit that referenced this pull request Oct 22, 2019
@Piinks
Revert "Skia Gold Support for Local & PreSubmit Testing in package:fl…
de911e8 
…utter (#40710)"

This reverts commit 8df0d65.
@Piinks Piinks mentioned this pull request Oct 22, 2019
Merged
Piinks added a commit that referenced this pull request Oct 22, 2019
@Piinks
Revert "Skia Gold Support for Local & PreSubmit Testing in package:fl…
3a3939a 
…utter (#40710)" (#43227)

This reverts commit 8df0d65.
Piinks
Piinks commented Oct 22, 2019
View reviewed changes
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'
Copy link
Contributor Author

@Piinks Piinks Oct 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@kjlubick kjlubick Oct 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@kjlubick kjlubick Oct 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@kjlubick kjlubick Oct 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Piinks Piinks mentioned this pull request Oct 23, 2019
Merged
9 tasks
@Piinks 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
@Piinks @Inconnu08
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
@Piinks @Inconnu08
Revert "Skia Gold Support for Local & PreSubmit Testing in package:fl…
daa3170 
…utter (flutter#40710)" (flutter#43227)

This reverts commit 8df0d65.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@kjlubick kjlubick kjlubick left review comments

@goderbauer goderbauer goderbauer approved these changes

+2 more reviewers

@dnfield dnfield dnfield left review comments

@jonahwilliams jonahwilliams jonahwilliams left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

a: images Loading, displaying, rendering images a: tests "flutter test", flutter_test, or one of our tests c: API break Backwards-incompatible API changes c: contributor-productivity Team-specific productivity, code health, technical debt. framework flutter/packages/flutter repository. See also f: labels. will affect goldens Changes to golden files

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@Piinks @fluttergithubbot @goderbauer @kjlubick @dnfield @jonahwilliams @googlebot