Skip to content

Refactoring Gold to enable both Luci & Cirrus support #49815

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
fluttergithubbot merged 35 commits into from
Mar 14, 2020
Merged

Refactoring Gold to enable both Luci & Cirrus support #49815

fluttergithubbot merged 35 commits into from
Mar 14, 2020

Conversation

@Piinks
Copy link
Contributor

@Piinks Piinks commented Jan 30, 2020 •
edited
Loading

Description

This will enable Skia Gold testing on the Luci CI environments.
This will allow us to run golden file tests on luci for the framework as well as driver tests.

Related Issues

Fixes #49749
Fixes #49837
Related to: #49750

Tests

Added authentication tests for luci environment, as well as validation for comparator selection.

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.

@Piinks Piinks added a: tests "flutter test", flutter_test, or one of our tests c: contributor-productivity Team-specific productivity, code health, technical debt. framework flutter/packages/flutter repository. See also f: labels. team-infra Owned by Infrastructure team labels Jan 30, 2020
@Piinks Piinks mentioned this pull request Jan 30, 2020
Closed
@goderbauer
Copy link
Member

goderbauer commented Jan 30, 2020

/cc @blasten FYI and maybe you can also review it :)

blasten
blasten reviewed Jan 30, 2020
View reviewed changes
packages/flutter_goldens_client/lib/skia_client.dart Outdated
/// This ensures that the goldctl tool is authorized and ready for testing. It
/// will only be called once for each instance of
/// [FlutterSkiaGoldFileComparator].
Future<void> luciAuth() async {
Copy link

@blasten blasten Jan 30, 2020

Choose a reason for hiding this comment

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

Would it make sense to detect if it's a LUCI environment in auth() and then do this work? @kjlubick Is there a way to detect a LUCI host?

Copy link
Contributor Author

@Piinks Piinks Jan 30, 2020

Choose a reason for hiding this comment

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

🤦‍♀ Much better idea. Thank you! Detecting luci is easy, the environment has a SWARMING_TASK_ID variable. I'll update this tomorrow.

@Piinks Piinks changed the title Adding luci authentication method to skia client WIP Enabling Flutter Gold on Luci Jan 31, 2020
@Piinks Piinks mentioned this pull request Jan 31, 2020
Merged
13 tasks
@Piinks
Copy link
Contributor Author

Piinks commented Jan 31, 2020

I think I am going to just enable Luci testing fully in this PR. It will take a little bit more refactoring for the cirrus-specific areas of the skia client, but should have it up soon.

@Piinks
Copy link
Contributor Author

Piinks commented Feb 5, 2020

FYI @kjlubick and @blasten
This is still a work in progress. I need to validate a number of things first (marked with TODOs), and update the recipe to have goldctl available first.

kjlubick
kjlubick reviewed Feb 6, 2020
View reviewed changes
kjlubick
kjlubick reviewed Feb 6, 2020
View reviewed changes
@Piinks Piinks mentioned this pull request Feb 6, 2020
Merged
Piinks
Piinks commented Feb 7, 2020
View reviewed changes
Piinks
Piinks commented Feb 7, 2020
View reviewed changes
packages/flutter/lib/src/cupertino/action_sheet.dart
///
/// To display action buttons that look like standard iOS action sheet buttons,
/// provide [CupertinoActionSheetAction]s for the [actions] given to this action sheet.
/// provide [CupertinoActionSheetAction]s for the [actions] given to this action
Copy link
Contributor Author

@Piinks Piinks Feb 7, 2020

Choose a reason for hiding this comment

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

Trivial >80 characters style change to trigger the appropriate test shards.

@fluttergithubbot fluttergithubbot added the f: cupertino flutter/packages/flutter/cupertino repository label Feb 7, 2020
@Piinks Piinks changed the title Enabling Flutter Gold on Luci Refactoring Gold to enable Luci & Cirrus Mar 10, 2020
@fluttergithubbot fluttergithubbot added the f: cupertino flutter/packages/flutter/cupertino repository label Mar 10, 2020
@Piinks Piinks requested review from blasten, cyanglaz and dnfield March 10, 2020 17:50
@Piinks
Copy link
Contributor Author

Piinks commented Mar 10, 2020

This is ready for review.

@cyanglaz I removed the ability for it to turn the tree red on master. We can implement this in the actual driver test use case, or make that change later. Currently there are other conditions where it may turn the tree red when we don't want it too, like when Cirrus denies decrypting the service account for first time contributors. That will not be an issue once we are off cirrus and running tests solely on luci. 😉

@Piinks Piinks changed the title Refactoring Gold to enable Luci & Cirrus Refactoring Gold to enable both Luci & Cirrus support Mar 10, 2020
kjlubick
kjlubick approved these changes Mar 10, 2020
View reviewed changes
@cyanglaz cyanglaz mentioned this pull request Mar 13, 2020
Merged
13 tasks
@Piinks Piinks requested a review from goderbauer March 13, 2020 21:17
goderbauer
goderbauer reviewed Mar 13, 2020
View reviewed changes
goderbauer
goderbauer approved these changes Mar 13, 2020
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

blasten
blasten approved these changes Mar 13, 2020
View reviewed changes
Copy link

@blasten blasten left a comment

Choose a reason for hiding this comment

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

Thanks @Piinks!

@fluttergithubbot fluttergithubbot merged commit 7edcc92 into flutter:master Mar 14, 2020
Piinks added a commit that referenced this pull request Mar 16, 2020
@Piinks
Revert "Refactoring Gold to enable both Luci & Cirrus support (#49815)"
fdc3eb1 
This reverts commit 7edcc92.
@Piinks Piinks mentioned this pull request Mar 16, 2020
Merged
Piinks added a commit that referenced this pull request Mar 16, 2020
@Piinks
Revert "Refactoring Gold to enable both Luci & Cirrus support (#49815)…
48fc135 
…" (#52670)

This reverts commit 7edcc92.
@lock
Copy link

lock bot commented Apr 2, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@lock lock bot locked and limited conversation to collaborators Apr 2, 2020
@Piinks Piinks deleted the luciAuth branch March 14, 2025 17:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@goderbauer goderbauer goderbauer approved these changes

@kjlubick kjlubick kjlubick approved these changes

@cyanglaz cyanglaz Awaiting requested review from cyanglaz

@dnfield dnfield Awaiting requested review from dnfield

+1 more reviewer

@blasten blasten blasten approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

a: tests "flutter test", flutter_test, or one of our tests c: contributor-productivity Team-specific productivity, code health, technical debt. f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels. team-infra Owned by Infrastructure team will affect goldens Changes to golden files

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Enable access Golden File comparator on LUCI Skia Gold service access from LUCI

8 participants

@Piinks @goderbauer @kjlubick @cyanglaz @blasten @googlebot @digiter @fluttergithubbot