Refactoring Gold to enable both Luci & Cirrus support (#49815)

packages/flutter/lib/src/cupertino/action_sheet.dart

@@ -85,7 +85,8 @@ const double _kDividerThickness = 1.0;
 /// sheet title and message text style.
 ///
 /// 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
+/// sheet.
 ///
 /// To include a iOS-style cancel button separate from the other buttons,
 /// provide an [CupertinoActionSheetAction] for the [cancelButton] given to this

packages/flutter_goldens/lib/flutter_goldens.dart

@@ -28,13 +28,13 @@ const String _kFlutterRootKey = 'FLUTTER_ROOT';
 /// instantiated is based on the current testing environment.
 Future<void> main(FutureOr<void> testMain()) async {
   const Platform platform = LocalPlatform();
-  if (FlutterSkiaGoldFileComparator.isAvailableForEnvironment(platform)) {
-    goldenFileComparator = await FlutterSkiaGoldFileComparator.fromDefaultComparator(platform);
+  if (FlutterPostSubmitFileComparator.isAvailableForEnvironment(platform)) {
+    goldenFileComparator = await FlutterPostSubmitFileComparator.fromDefaultComparator(platform);
   } else if (FlutterPreSubmitFileComparator.isAvailableForEnvironment(platform)) {
     goldenFileComparator = await FlutterPreSubmitFileComparator.fromDefaultComparator(platform);
-  } else if (FlutterSkippingGoldenFileComparator.isAvailableForEnvironment(platform)) {
-    goldenFileComparator = FlutterSkippingGoldenFileComparator.fromDefaultComparator(
-      'Golden file testing is unavailable on LUCI and  some Cirrus shards.'
+  } else if (FlutterSkippingFileComparator.isAvailableForEnvironment(platform)) {
+    goldenFileComparator = FlutterSkippingFileComparator.fromDefaultComparator(
+      'Golden file testing is not executed on some Cirrus shards.'
     );
   } else {
     goldenFileComparator = await FlutterLocalFileComparator.fromDefaultComparator(platform);
@@ -50,7 +50,7 @@ Future<void> main(FutureOr<void> testMain()) async {
 /// different [FlutterGoldenFileComparator]s, depending on the current testing
 /// environment.
 ///
-///   * The [FlutterSkiaGoldFileComparator] is utilized during post-submit
+///   * The [FlutterPostSubmitFileComparator] is utilized during post-submit
 ///     testing, after a pull request has landed on the master branch. This
 ///     comparator uses the [SkiaGoldClient] and the `goldctl` tool to upload
 ///     tests to the [Flutter Gold dashboard](https://flutter-gold.skia.org).
@@ -58,28 +58,34 @@ Future<void> main(FutureOr<void> testMain()) async {
 ///     repository.
 ///
 ///   * The [FlutterPreSubmitFileComparator] is utilized in pre-submit testing,
-///     before a pull request can land on the master branch. This comparator
-///     uses the [SkiaGoldClient] to request the baseline images kept by the
-///     [Flutter Gold dashboard](https://flutter-gold.skia.org). It then
-///     compares the current test image to the baseline images using the
-///     standard [GoldenFileComparator.compareLists] to detect any pixel
-///     difference. The [SkiaGoldClient] is also used here to check the active
-///     ignores from the dashboard, in order to allow intended changes to pass
-///     tests.
+///     before a pull request lands on the master branch. When authorized, this
+///     comparator uses the [SkiaGoldClient] to execute tryjobs, allowing
+///     contributors to view and check in visual differences before landing the
+///     change.
 ///
-///   * The [FlutterLocalFileComparator] is used for any other tests run outside
-///     of the above conditions. Similar to the
+///       * When unable to authenticate the `goldctl` tool, this comparator
+///         uses the [SkiaGoldClient] to request the baseline images kept by the
+///         [Flutter Gold dashboard](https://flutter-gold.skia.org). It then
+///         compares the current test image to the baseline images using the
+///         standard [GoldenFileComparator.compareLists] to detect any pixel
+///         difference. The [SkiaGoldClient] is also used in this case to check
+///         the active ignores from the dashboard, in order to allow intended
+///         changes to pass tests.
+///
+///   * The [FlutterLocalFileComparator] is used for local development testing.
+///     Similar to the unauthorized implementation of the
 ///     [FlutterPreSubmitFileComparator], this comparator will use the
 ///     [SkiaGoldClient] to request baseline images from
-///     [Flutter Gold](https://flutter-gold.skia.org) and compares for the
-///     current test image. If a difference is detected, this comparator will
+///     [Flutter Gold](https://flutter-gold.skia.org) and manually compare
+///     pixels. If a difference is detected, this comparator will
 ///     generate failure output illustrating the found difference. If a baseline
 ///     is not found for a given test image, it will consider it a new test and
 ///     output the new image for verification.
-///  The [FlutterSkippingGoldenFileComparator] is utilized to skip tests outside
-///  of the appropriate environments. Currently, tests executing in post-submit
-///  on the LUCI build environment are skipped, as post-submit checks are done
-///  on Cirrus. This comparator is also used when an internet connection is
+///
+///  The [FlutterSkippingFileComparator] is utilized to skip tests outside
+///  of the appropriate environments described above. Currently, some Cirrus
+///  test shards do not execute golden file testing, and as such do not require
+///  a comparator. This comparator is also used when an internet connection is
 ///  unavailable.
 abstract class FlutterGoldenFileComparator extends GoldenFileComparator {
   /// Creates a [FlutterGoldenFileComparator] that will resolve golden file
@@ -131,7 +137,7 @@ abstract class FlutterGoldenFileComparator extends GoldenFileComparator {
   /// Calculate the appropriate basedir for the current test context.
   ///
   /// The optional [suffix] argument is used by the
-  /// [FlutterSkiaGoldFileComparator] and the [FlutterPreSubmitFileComparator].
+  /// [FlutterPostSubmitFileComparator] and the [FlutterPreSubmitFileComparator].
   /// These [FlutterGoldenFileComparators] randomize their base directories to
   /// maintain thread safety while using the `goldctl` tool.
   @protected
@@ -148,7 +154,7 @@ abstract class FlutterGoldenFileComparator extends GoldenFileComparator {
 
     if (!local) {
       comparisonRoot = fs.systemTempDirectory.childDirectory(
-        'skia_goldens$suffix'
+        'skia_goldens_$suffix'
       );
     } else {
       comparisonRoot = flutterRoot.childDirectory(
@@ -184,12 +190,11 @@ abstract class FlutterGoldenFileComparator extends GoldenFileComparator {
   }
 }
 
-/// A [FlutterGoldenFileComparator] for testing golden images with Skia Gold.
+/// A [FlutterGoldenFileComparator] for testing golden images with Skia Gold in
+/// post-submit.
 ///
 /// For testing across all platforms, the [SkiaGoldClient] is used to upload
-/// images for framework-related golden tests and process results. Currently
-/// these tests are designed to be run post-submit on Cirrus CI, informed by the
-/// environment.
+/// images for framework-related golden tests and process results.
 ///
 /// See also:
 ///
@@ -201,13 +206,13 @@ abstract class FlutterGoldenFileComparator extends GoldenFileComparator {
 ///  * [FlutterLocalFileComparator], another
 ///    [FlutterGoldenFileComparator] that tests golden images locally on your
 ///    current machine.
-class FlutterSkiaGoldFileComparator extends FlutterGoldenFileComparator {
-  /// Creates a [FlutterSkiaGoldFileComparator] that will test golden file
+class FlutterPostSubmitFileComparator extends FlutterGoldenFileComparator {
+  /// Creates a [FlutterPostSubmitFileComparator] that will test golden file
   /// images against Skia Gold.
   ///
   /// The [fs] and [platform] parameters are useful in tests, where the default
   /// file system and platform can be replaced by mock instances.
-  FlutterSkiaGoldFileComparator(
+  FlutterPostSubmitFileComparator(
     final Uri basedir,
     final SkiaGoldClient skiaClient, {
     final FileSystem fs = const LocalFileSystem(),
@@ -219,12 +224,12 @@ class FlutterSkiaGoldFileComparator extends FlutterGoldenFileComparator {
     platform: platform,
   );
 
-  /// Creates a new [FlutterSkiaGoldFileComparator] that mirrors the relative
+  /// Creates a new [FlutterPostSubmitFileComparator] that mirrors the relative
   /// path resolution of the default [goldenFileComparator].
   ///
   /// The [goldens] and [defaultComparator] parameters are visible for testing
   /// purposes only.
-  static Future<FlutterSkiaGoldFileComparator> fromDefaultComparator(
+  static Future<FlutterPostSubmitFileComparator> fromDefaultComparator(
     final Platform platform, {
     SkiaGoldClient goldens,
     LocalFileComparator defaultComparator,
@@ -238,10 +243,15 @@ class FlutterSkiaGoldFileComparator extends FlutterGoldenFileComparator {
     );
     baseDirectory.createSync(recursive: true);
 
-    goldens ??= SkiaGoldClient(baseDirectory);
+    goldens ??= SkiaGoldClient(
+      baseDirectory,
+      ci: platform.environment.containsKey('CIRRUS_CI')
+        ? ContinuousIntegrationEnvironment.cirrus
+        : ContinuousIntegrationEnvironment.luci,
+    );
     await goldens.auth();
     await goldens.imgtestInit();
-    return FlutterSkiaGoldFileComparator(baseDirectory.uri, goldens);
+    return FlutterPostSubmitFileComparator(baseDirectory.uri, goldens);
   }
 
   @override
@@ -253,32 +263,40 @@ class FlutterSkiaGoldFileComparator extends FlutterGoldenFileComparator {
     return skiaClient.imgtestAdd(golden.path, goldenFile);
   }
 
-  /// Decides based on the current environment whether goldens tests should be
-  /// performed against Skia Gold.
+  /// Decides based on the current environment if goldens tests should be
+  /// executed through Skia Gold.
   static bool isAvailableForEnvironment(Platform platform) {
     final String cirrusPR = platform.environment['CIRRUS_PR'] ?? '';
     final String cirrusBranch = platform.environment['CIRRUS_BRANCH'] ?? '';
-    return platform.environment.containsKey('CIRRUS_CI')
+    final bool cirrusPostSubmit = platform.environment.containsKey('CIRRUS_CI')
       && cirrusPR.isEmpty
       && cirrusBranch == 'master'
       && platform.environment.containsKey('GOLD_SERVICE_ACCOUNT');
+
+    final bool luciPostSubmit = platform.environment.containsKey('SWARMING_TASK_ID')
+      // Luci tryjob environments contain this value to inform the [FlutterPreSubmitComparator].
+      && !platform.environment.containsKey('GOLD_TRYJOB');
+
+    return cirrusPostSubmit || luciPostSubmit;
   }
 }
 
 /// A [FlutterGoldenFileComparator] for testing golden images before changes are
 /// merged into the master branch.
 ///
-/// This comparator utilizes the [SkiaGoldClient] to request baseline images for
-/// the given device under test for comparison. This comparator is only
-/// initialized during pre-submit testing on Cirrus CI.
+/// When authorized (on luci and most cirrus testing conditions), the comparator
+/// executes tryjobs using the [SkiaGoldClient].
+///
+/// When unauthorized, this comparator utilizes the [SkiaGoldClient] to request
+/// baseline images for the given device under test for manual comparison.
 ///
 /// See also:
 ///
 ///  * [GoldenFileComparator], the abstract class that
 ///    [FlutterGoldenFileComparator] implements.
-///  * [FlutterSkiaGoldFileComparator], another
+///  * [FlutterPostSubmitFileComparator], another
 ///    [FlutterGoldenFileComparator] that uploads tests to the Skia Gold
-///    dashboard.
+///    dashboard in post-submit.
 ///  * [FlutterLocalFileComparator], another
 ///    [FlutterGoldenFileComparator] that tests golden images locally on your
 ///    current machine.
@@ -322,10 +340,22 @@ class FlutterPreSubmitFileComparator extends FlutterGoldenFileComparator {
     if (!baseDirectory.existsSync())
       baseDirectory.createSync(recursive: true);
 
-    goldens ??= SkiaGoldClient(baseDirectory);
+    goldens ??= SkiaGoldClient(
+      baseDirectory,
+      ci: platform.environment.containsKey('CIRRUS_CI')
+        ? ContinuousIntegrationEnvironment.cirrus
+        : ContinuousIntegrationEnvironment.luci,
+    );
 
-    final bool hasWritePermission = !platform.environment['GOLD_SERVICE_ACCOUNT'].startsWith('ENCRYPTED');
-    if (hasWritePermission) {
+    bool onCirrusWithPermission = false;
+    if (platform.environment.containsKey('GOLD_SERVICE_ACCOUNT')) {
+      // Some contributors may not have permission on Cirrus to decrypt the
+      // service account.
+      onCirrusWithPermission =
+        !platform.environment['GOLD_SERVICE_ACCOUNT'].startsWith('ENCRYPTED');
+    }
+    final bool onLuci = platform.environment.containsKey('SWARMING_TASK_ID');
+    if (onCirrusWithPermission || onLuci) {
       await goldens.auth();
       await goldens.tryjobInit();
       return _AuthorizedFlutterPreSubmitComparator(
@@ -356,13 +386,17 @@ class FlutterPreSubmitFileComparator extends FlutterGoldenFileComparator {
     return false;
   }
 
-  /// Decides based on the current environment whether goldens tests should be
-  /// performed as pre-submit tests with Skia Gold.
+  /// Decides based on the current environment if goldens tests should be
+  /// executed as pre-submit tests with Skia Gold.
   static bool isAvailableForEnvironment(Platform platform) {
     final String cirrusPR = platform.environment['CIRRUS_PR'] ?? '';
-    return platform.environment.containsKey('CIRRUS_CI')
+    final bool cirrusPreSubmit = platform.environment.containsKey('CIRRUS_CI')
       && cirrusPR.isNotEmpty
       && platform.environment.containsKey('GOLD_SERVICE_ACCOUNT');
+
+    final bool luciPreSubmit = platform.environment.containsKey('SWARMING_TASK_ID')
+      && platform.environment.containsKey('GOLD_TRYJOB');
+    return cirrusPreSubmit || luciPreSubmit;
   }
 }
 
@@ -442,19 +476,15 @@ class _UnauthorizedFlutterPreSubmitComparator extends FlutterPreSubmitFileCompar
   }
 }
 
-/// A [FlutterGoldenFileComparator] for controlling post-submit testing
-/// conditions that do not execute golden file tests.
+/// A [FlutterGoldenFileComparator] for testing conditions that do not execute
+/// golden file tests.
 ///
-/// Currently, this comparator is used in post-submit checks on LUCI and with
-/// some Cirrus shards that do not run framework tests. This comparator is also
-/// used when an internet connection is not available for contacting Gold.
+/// Currently, this comparator is used in some Cirrus test shards, and when an
+/// internet connection is not available for contacting Gold.
 ///
 /// See also:
 ///
-///  * [FlutterGoldensRepositoryFileComparator], another
-///    [FlutterGoldenFileComparator] that tests golden images using the
-///    flutter/goldens repository.
-///  * [FlutterSkiaGoldFileComparator], another [FlutterGoldenFileComparator]
+///  * [FlutterPostSubmitFileComparator], another [FlutterGoldenFileComparator]
 ///    that tests golden images through Skia Gold.
 ///  * [FlutterPreSubmitFileComparator], another
 ///    [FlutterGoldenFileComparator] that tests golden images before changes are
@@ -462,24 +492,24 @@ class _UnauthorizedFlutterPreSubmitComparator extends FlutterPreSubmitFileCompar
 ///  * [FlutterLocalFileComparator], another
 ///    [FlutterGoldenFileComparator] that tests golden images locally on your
 ///    current machine.
-class FlutterSkippingGoldenFileComparator extends FlutterGoldenFileComparator {
-  /// Creates a [FlutterSkippingGoldenFileComparator] that will skip tests that
+class FlutterSkippingFileComparator extends FlutterGoldenFileComparator {
+  /// Creates a [FlutterSkippingFileComparator] that will skip tests that
   /// are not in the right environment for golden file testing.
-  FlutterSkippingGoldenFileComparator(
+  FlutterSkippingFileComparator(
     final Uri basedir,
     final SkiaGoldClient skiaClient,
     this.reason,
   ) : assert(reason != null),
       super(basedir, skiaClient);
 
-  /// Describes the reason for using the [FlutterSkippingGoldenFileComparator].
+  /// Describes the reason for using the [FlutterSkippingFileComparator].
   ///
   /// Cannot be null.
   final String reason;
 
-  /// Creates a new [FlutterSkippingGoldenFileComparator] that mirrors the
+  /// Creates a new [FlutterSkippingFileComparator] that mirrors the
   /// relative path resolution of the default [goldenFileComparator].
-  static FlutterSkippingGoldenFileComparator fromDefaultComparator(
+  static FlutterSkippingFileComparator fromDefaultComparator(
     String reason, {
     LocalFileComparator defaultComparator,
   }) {
@@ -487,7 +517,7 @@ class FlutterSkippingGoldenFileComparator extends FlutterGoldenFileComparator {
     const FileSystem fs = LocalFileSystem();
     final Uri basedir = defaultComparator.basedir;
     final SkiaGoldClient skiaClient = SkiaGoldClient(fs.directory(basedir));
-    return FlutterSkippingGoldenFileComparator(basedir, skiaClient, reason);
+    return FlutterSkippingFileComparator(basedir, skiaClient, reason);
   }
 
   @override
@@ -501,8 +531,11 @@ class FlutterSkippingGoldenFileComparator extends FlutterGoldenFileComparator {
   @override
   Future<void> update(Uri golden, Uint8List imageBytes) => null;
 
-  /// Decides based on the current environment whether this comparator should be
+  /// Decides, based on the current environment, if this comparator should be
   /// used.
+  ///
+  /// If we are in a CI environment, luci or Cirrus, but are not using the other
+  /// comparators, we skip.
   static bool isAvailableForEnvironment(Platform platform) {
     return platform.environment.containsKey('SWARMING_TASK_ID')
       || platform.environment.containsKey('CIRRUS_CI');
@@ -526,13 +559,13 @@ class FlutterSkippingGoldenFileComparator extends FlutterGoldenFileComparator {
 ///
 ///  * [GoldenFileComparator], the abstract class that
 ///    [FlutterGoldenFileComparator] implements.
-///  * [FlutterSkiaGoldFileComparator], another
+///  * [FlutterPostSubmitFileComparator], another
 ///    [FlutterGoldenFileComparator] that uploads tests to the Skia Gold
 ///    dashboard.
 ///  * [FlutterPreSubmitFileComparator], another
 ///    [FlutterGoldenFileComparator] that tests golden images before changes are
 ///    merged into the master branch.
-///  * [FlutterSkippingGoldenFileComparator], another
+///  * [FlutterSkippingFileComparator], another
 ///    [FlutterGoldenFileComparator] that controls post-submit testing
 ///    conditions that do not execute golden file tests.
 class FlutterLocalFileComparator extends FlutterGoldenFileComparator with LocalComparisonOutput {
@@ -580,14 +613,14 @@ class FlutterLocalFileComparator extends FlutterGoldenFileComparator with LocalC
     try {
       await goldens.getExpectations();
     } on io.OSError catch (_) {
-      return FlutterSkippingGoldenFileComparator(
+      return FlutterSkippingFileComparator(
         baseDirectory.uri,
         goldens,
         'OSError occurred, could not reach Gold. '
           'Switching to FlutterSkippingGoldenFileComparator.',
       );
     } on io.SocketException catch (_) {
-      return FlutterSkippingGoldenFileComparator(
+      return FlutterSkippingFileComparator(
         baseDirectory.uri,
         goldens,
         'SocketException occurred, could not reach Gold. '

packages/flutter_goldens/test/flutter_goldens_test.dart

@@ -108,6 +108,7 @@ void main() {
         process: process,
         platform: platform,
         httpClient: mockHttpClient,
+        ci: ContinuousIntegrationEnvironment.cirrus,
       );
 
       when(process.run(any))
@@ -165,6 +166,76 @@ void main() {
       );
     });
 
+    test('correctly inits tryjob for luci', () async {
+      platform = FakePlatform(
+        environment: <String, String>{
+          'FLUTTER_ROOT': _kFlutterRoot,
+          'GOLDCTL' : 'goldctl',
+          'SWARMING_TASK_ID' : '4ae997b50dfd4d11',
+          'LOGDOG_STREAM_PREFIX' : 'buildbucket/cr-buildbucket.appspot.com/8885996262141582672',
+          'GOLD_TRYJOB' : 'refs/pull/49815/head',
+        },
+        operatingSystem: 'macos'
+      );
+
+      skiaClient = SkiaGoldClient(
+        workDirectory,
+        fs: fs,
+        process: process,
+        platform: platform,
+        httpClient: mockHttpClient,
+        ci: ContinuousIntegrationEnvironment.luci,
+      );
+
+      final List<String> ciArguments = skiaClient.getCIArguments();
+
+      expect(
+        ciArguments,
+        equals(
+          <String>[
+            '--changelist', '49815',
+            '--cis', 'buildbucket',
+            '--jobid', '8885996262141582672',
+          ],
+        ),
+      );
+    });
+
+    test('correctly inits tryjob for cirrus', () async {
+      platform = FakePlatform(
+        environment: <String, String>{
+          'FLUTTER_ROOT': _kFlutterRoot,
+          'GOLDCTL' : 'goldctl',
+          'CIRRUS_CI' : 'true',
+          'CIRRUS_TASK_ID' : '8885996262141582672',
+          'CIRRUS_PR' : '49815',
+        },
+        operatingSystem: 'macos'
+      );
+
+      skiaClient = SkiaGoldClient(
+        workDirectory,
+        fs: fs,
+        process: process,
+        platform: platform,
+        httpClient: mockHttpClient,
+        ci: ContinuousIntegrationEnvironment.cirrus,
+      );
+
+      final List<String> ciArguments = skiaClient.getCIArguments();
+
+      expect(
+        ciArguments,
+        equals(
+          <String>[
+            '--changelist', '49815',
+            '--cis', 'cirrus',
+            '--jobid', '8885996262141582672',
+          ],
+        ),
+      );
+    });
+
     group('Request Handling', () {
       String testName;
       String pullRequestNumber;
@@ -456,12 +527,12 @@ void main() {
   });
 
   group('FlutterGoldenFileComparator', () {
-    FlutterSkiaGoldFileComparator comparator;
+    FlutterPostSubmitFileComparator comparator;
 
     setUp(() {
       final Directory basedir = fs.directory('flutter/test/library/')
         ..createSync(recursive: true);
-      comparator = FlutterSkiaGoldFileComparator(
+      comparator = FlutterPostSubmitFileComparator(
         basedir.uri,
         MockSkiaGoldClient(),
         fs: fs,
@@ -497,7 +568,7 @@ void main() {
       setUp(() {
         final Directory basedir = fs.directory('flutter/test/library/')
           ..createSync(recursive: true);
-        comparator = FlutterSkiaGoldFileComparator(
+        comparator = FlutterPostSubmitFileComparator(
           basedir.uri,
           mockSkiaClient,
           fs: fs,
@@ -506,7 +577,21 @@ void main() {
       });
 
       group('correctly determines testing environment', () {
-        test('returns true', () {
+        test('returns true for Luci', () {
+          platform = FakePlatform(
+            environment: <String, String>{
+              'FLUTTER_ROOT': _kFlutterRoot,
+              'SWARMING_TASK_ID' : '12345678990',
+            },
+            operatingSystem: 'macos'
+          );
+          expect(
+            FlutterPostSubmitFileComparator.isAvailableForEnvironment(platform),
+            isTrue,
+          );
+        });
+
+        test('returns true for Cirrus', () {
           platform = FakePlatform(
             environment: <String, String>{
               'FLUTTER_ROOT': _kFlutterRoot,
@@ -518,7 +603,7 @@ void main() {
             operatingSystem: 'macos'
           );
           expect(
-            FlutterSkiaGoldFileComparator.isAvailableForEnvironment(platform),
+            FlutterPostSubmitFileComparator.isAvailableForEnvironment(platform),
             isTrue,
           );
         });
@@ -535,7 +620,7 @@ void main() {
             operatingSystem: 'macos'
           );
           expect(
-            FlutterSkiaGoldFileComparator.isAvailableForEnvironment(platform),
+            FlutterPostSubmitFileComparator.isAvailableForEnvironment(platform),
             isFalse,
           );
         });
@@ -551,7 +636,7 @@ void main() {
             operatingSystem: 'macos'
           );
           expect(
-            FlutterSkiaGoldFileComparator.isAvailableForEnvironment(platform),
+            FlutterPostSubmitFileComparator.isAvailableForEnvironment(platform),
             isFalse,
           );
         });
@@ -566,7 +651,7 @@ void main() {
             operatingSystem: 'macos'
           );
           expect(
-            FlutterSkiaGoldFileComparator.isAvailableForEnvironment(platform),
+            FlutterPostSubmitFileComparator.isAvailableForEnvironment(platform),
             isFalse,
           );
         });
@@ -583,7 +668,7 @@ void main() {
             operatingSystem: 'macos'
           );
           expect(
-            FlutterSkiaGoldFileComparator.isAvailableForEnvironment(platform),
+            FlutterPostSubmitFileComparator.isAvailableForEnvironment(platform),
             isFalse,
           );
         });
@@ -595,7 +680,7 @@ void main() {
       final MockSkiaGoldClient mockSkiaClient = MockSkiaGoldClient();
 
       group('correctly determines testing environment', () {
-        test('returns true', () {
+        test('returns true for Cirrus', () {
           platform = FakePlatform(
             environment: <String, String>{
               'FLUTTER_ROOT': _kFlutterRoot,
@@ -611,6 +696,21 @@ void main() {
           );
         });
 
+        test('returns true for Luci', () {
+          platform = FakePlatform(
+            environment: <String, String>{
+              'FLUTTER_ROOT': _kFlutterRoot,
+              'SWARMING_TASK_ID' : '12345678990',
+              'GOLD_TRYJOB' : 'git/ref/12345/head'
+            },
+            operatingSystem: 'macos'
+          );
+          expect(
+            FlutterPreSubmitFileComparator.isAvailableForEnvironment(platform),
+            isTrue,
+          );
+        });
+
         test('returns false - no PR', () {
           platform = FakePlatform(
             environment: <String, String>{
@@ -642,7 +742,7 @@ void main() {
           );
         });
 
-        test('returns false - not on Cirrus', () {
+        test('returns false - not on Cirrus or Luci', () {
           platform = FakePlatform(
             environment: <String, String>{
               'FLUTTER_ROOT': _kFlutterRoot,
@@ -765,21 +865,7 @@ void main() {
 
     group('Skipping', () {
       group('correctly determines testing environment', () {
-        test('returns true on LUCI', () {
-          platform = FakePlatform(
-            environment: <String, String>{
-              'FLUTTER_ROOT': _kFlutterRoot,
-              'SWARMING_TASK_ID' : '1234567890',
-            },
-            operatingSystem: 'macos'
-          );
-          expect(
-            FlutterSkippingGoldenFileComparator.isAvailableForEnvironment(platform),
-            isTrue,
-          );
-        });
-
-        test('returns true on Cirrus', () {
+        test('returns true on Cirrus shards that don\'t run golden tests', () {
           platform = FakePlatform(
             environment: <String, String>{
               'FLUTTER_ROOT': _kFlutterRoot,
@@ -788,10 +874,11 @@ void main() {
             operatingSystem: 'macos'
           );
           expect(
-            FlutterSkippingGoldenFileComparator.isAvailableForEnvironment(platform),
+            FlutterSkippingFileComparator.isAvailableForEnvironment(platform),
             isTrue,
           );
         });
+
         test('returns false - no CI', () {
           platform = FakePlatform(
             environment: <String, String>{
@@ -800,7 +887,7 @@ void main() {
             operatingSystem: 'macos'
           );
           expect(
-            FlutterSkippingGoldenFileComparator.isAvailableForEnvironment(
+            FlutterSkippingFileComparator.isAvailableForEnvironment(
               platform),
             isFalse,
           );
@@ -891,7 +978,7 @@ void main() {
           goldens: mockSkiaClient,
           baseDirectory: mockDirectory,
         );
-        expect(comparator.runtimeType, FlutterSkippingGoldenFileComparator);
+        expect(comparator.runtimeType, FlutterSkippingFileComparator);
 
         when(mockSkiaClient.getExpectations())
           .thenAnswer((_) => throw const SocketException("Can't reach Gold"));
@@ -900,7 +987,7 @@ void main() {
           goldens: mockSkiaClient,
           baseDirectory: mockDirectory,
         );
-        expect(comparator.runtimeType, FlutterSkippingGoldenFileComparator);
+        expect(comparator.runtimeType, FlutterSkippingFileComparator);
       });
     });
   });

packages/flutter_goldens_client/lib/skia_client.dart

@@ -21,6 +21,12 @@ const String _kGoldctlKey = 'GOLDCTL';
 const String _kServiceAccountKey = 'GOLD_SERVICE_ACCOUNT';
 const String _kTestBrowserKey = 'FLUTTER_TEST_BROWSER';
 
+/// Enum representing the supported CI environments used by flutter/flutter.
+enum ContinuousIntegrationEnvironment {
+  luci,
+  cirrus,
+}
+
 /// A client for uploading image tests and making baseline requests to the
 /// Flutter Gold Dashboard.
 class SkiaGoldClient {
@@ -29,6 +35,7 @@ class SkiaGoldClient {
     this.fs = const LocalFileSystem(),
     this.process = const LocalProcessManager(),
     this.platform = const LocalPlatform(),
+    this.ci,
     io.HttpClient httpClient,
   }) : assert(workDirectory != null),
        assert(fs != null),
@@ -38,23 +45,26 @@ class SkiaGoldClient {
 
   /// The file system to use for storing the local clone of the repository.
   ///
-  /// This is useful in tests, where a local file system (the default) can
-  /// be replaced by a memory file system.
+  /// This is useful in tests, where a local file system (the default) can be
+  /// replaced by a memory file system.
   final FileSystem fs;
 
   /// A wrapper for the [dart:io.Platform] API.
   ///
-  /// This is useful in tests, where the system platform (the default) can
-  /// be replaced by a mock platform instance.
+  /// This is useful in tests, where the system platform (the default) can be
+  /// replaced by a mock platform instance.
   final Platform platform;
 
   /// A controller for launching sub-processes.
   ///
-  /// This is useful in tests, where the real process manager (the default)
-  /// can be replaced by a mock process manager that doesn't really create
+  /// This is useful in tests, where the real process manager (the default) can
+  /// be replaced by a mock process manager that doesn't really create
   /// sub-processes.
   final ProcessManager process;
 
+  /// What testing environment we may be in, like Cirrus or Luci.
+  final ContinuousIntegrationEnvironment ci;
+
   /// A client for making Http requests to the Flutter Gold dashboard.
   final io.HttpClient httpClient;
 
@@ -69,9 +79,9 @@ class SkiaGoldClient {
   /// A map of known golden file tests and their associated positive image
   /// hashes.
   ///
-  /// This is set and used by the [FlutterLocalFileComparator] and
-  /// [FlutterPreSubmitFileComparator] to test against golden masters maintained
-  /// in the Flutter Gold dashboard.
+  /// This is set and used by the [FlutterLocalFileComparator] and the
+  /// [_UnauthorizedFlutterPreSubmitComparator] to test against golden masters
+  /// maintained in the Flutter Gold dashboard.
   Map<String, List<String>> get expectations => _expectations;
   Map<String, List<String>> _expectations;
 
@@ -94,31 +104,57 @@ class SkiaGoldClient {
   /// Prepares the local work space for golden file testing and calls the
   /// goldctl `auth` command.
   ///
-  /// This ensures that the goldctl tool is authorized and ready for testing. It
-  /// will only be called once for each instance of
-  /// [FlutterSkiaGoldFileComparator].
+  /// This ensures that the goldctl tool is authorized and ready for testing.
+  /// Used by the [FlutterPostSubmitFileComparator] and the
+  /// [_AuthorizedFlutterPreSubmitComparator].
+  ///
+  /// Based on the current environment, the goldctl tool may be authorized by
+  /// a service account provided by Cirrus, or through the context provided by a
+  /// luci environment.
   Future<void> auth() async {
     if (await clientIsAuthorized())
       return;
 
-    if (_serviceAccount.isEmpty) {
-      final StringBuffer buf = StringBuffer()
-        ..writeln('The Gold service account is unavailable.')
-        ..writeln('Without a service account, Gold can not be authorized.')
-        ..writeln('Please check your user permissions and current comparator.');
-      throw Exception(buf.toString());
-    }
-
-    final File authorization = workDirectory.childFile('serviceAccount.json');
-    await authorization.writeAsString(_serviceAccount);
+    List<String> authArguments;
+    String failureContext;
+
+    switch (ci) {
+      case ContinuousIntegrationEnvironment.luci:
+        authArguments = <String>[
+          'auth',
+          '--work-dir', workDirectory
+            .childDirectory('temp')
+            .path,
+          '--luci',
+        ];
+        failureContext =
+        'Luci environments authenticate using the file provided '
+          'by LUCI_CONTEXT. There may be an error with this file or Gold '
+          'authentication.';
+        break;
+      case ContinuousIntegrationEnvironment.cirrus:
+        if (_serviceAccount.isEmpty) {
+          final StringBuffer buf = StringBuffer()
+            ..writeln('The Gold service account is unavailable.')..writeln(
+              'Without a service account, Gold can not be authorized.')..writeln(
+              'Please check your user permissions and current comparator.');
+          throw Exception(buf.toString());
+        }
 
-    final List<String> authArguments = <String>[
-      'auth',
-      '--service-account', authorization.path,
-      '--work-dir', workDirectory
-        .childDirectory('temp')
-        .path,
-    ];
+        final File authorization = workDirectory.childFile('serviceAccount.json');
+        await authorization.writeAsString(_serviceAccount);
+        authArguments = <String>[
+          'auth',
+          '--service-account', authorization.path,
+          '--work-dir', workDirectory
+            .childDirectory('temp')
+            .path,
+        ];
+        failureContext = 'This could be caused by incorrect user permissions on '
+          'Cirrus, if the debug information below contains ENCRYPTED, the wrong '
+          'comparator was chosen for the test case.';
+        break;
+    }
 
     final io.ProcessResult result = await io.Process.run(
       _goldctl,
@@ -128,10 +164,7 @@ class SkiaGoldClient {
     if (result.exitCode != 0) {
       final StringBuffer buf = StringBuffer()
         ..writeln('Skia Gold authorization failed.')
-        ..writeln('This could be caused by incorrect user permissions, if the ')
-        ..writeln('debug information below contains ENCRYPTED, the wrong ')
-        ..writeln('comparator was chosen for the test case.')
-        ..writeln()
+        ..writeln(failureContext)
         ..writeln('Debug information for Gold:')
         ..writeln('stdout: ${result.stdout}')
         ..writeln('stderr: ${result.stderr}');
@@ -145,6 +178,10 @@ class SkiaGoldClient {
   /// It will only be called once for each instance of an
   /// [_UnauthorizedFlutterPreSubmitComparator].
   Future<void> emptyAuth() async {
+    // We only use emptyAuth when the service account cannot be decrypted on
+    // Cirrus.
+    assert(ci == ContinuousIntegrationEnvironment.cirrus);
+
     final List<String> authArguments = <String>[
       'auth',
       '--work-dir', workDirectory
@@ -171,7 +208,8 @@ class SkiaGoldClient {
   /// Executes the `imgtest init` command in the goldctl tool.
   ///
   /// The `imgtest` command collects and uploads test results to the Skia Gold
-  /// backend, the `init` argument initializes the current test.
+  /// backend, the `init` argument initializes the current test. Used by the
+  /// [FlutterPostSubmitFileComparator].
   Future<void> imgtestInit() async {
     final File keys = workDirectory.childFile('keys.json');
     final File failures = workDirectory.childFile('failures.json');
@@ -227,7 +265,7 @@ class SkiaGoldClient {
   /// result.
   ///
   /// The [testName] and [goldenFile] parameters reference the current
-  /// comparison being evaluated by the [FlutterSkiaGoldFileComparator].
+  /// comparison being evaluated by the [FlutterPostSubmitFileComparator].
   Future<bool> imgtestAdd(String testName, File goldenFile) async {
     assert(testName != null);
     assert(goldenFile != null);
@@ -260,7 +298,8 @@ class SkiaGoldClient {
   /// Executes the `imgtest init` command in the goldctl tool for tryjobs.
   ///
   /// The `imgtest` command collects and uploads test results to the Skia Gold
-  /// backend, the `init` argument initializes the current tryjob.
+  /// backend, the `init` argument initializes the current tryjob. Used by the
+  /// [_AuthorizedFlutterPreSubmitComparator].
   Future<void> tryjobInit() async {
     final File keys = workDirectory.childFile('keys.json');
     final File failures = workDirectory.childFile('failures.json');
@@ -268,9 +307,6 @@ class SkiaGoldClient {
     await keys.writeAsString(_getKeysJSON());
     await failures.create();
     final String commitHash = await _getCurrentCommit();
-    final String pullRequest = platform.environment['CIRRUS_PR'];
-    final String cirrusTaskID = platform.environment['CIRRUS_TASK_ID'];
-
 
     final List<String> imgtestInitArguments = <String>[
       'imgtest', 'init',
@@ -283,12 +319,11 @@ class SkiaGoldClient {
       '--failure-file', failures.path,
       '--passfail',
       '--crs', 'github',
-      '--changelist', pullRequest,
-      '--cis', 'cirrus',
-      '--jobid', cirrusTaskID,
       '--patchset_id', commitHash,
     ];
 
+    imgtestInitArguments.addAll(getCIArguments());
+
     if (imgtestInitArguments.contains(null)) {
       final StringBuffer buf = StringBuffer()
         ..writeln('A null argument was provided for Skia Gold tryjob init.')
@@ -452,11 +487,12 @@ class SkiaGoldClient {
   /// Returns a boolean value for whether or not the given test and current pull
   /// request are ignored on Flutter Gold.
   ///
-  /// This is only relevant when used by the [FlutterPreSubmitFileComparator]
-  /// when a golden file test fails. In order to land a change to an existing
-  /// golden file, an ignore must be set up in Flutter Gold. This will serve as
-  /// a flag to permit the change to land, protect against any unwanted changes,
-  /// and ensure that changes that have landed are triaged.
+  /// This is only relevant when used by the
+  /// [_UnauthorizedFlutterPreSubmitComparator] when a golden file test fails.
+  /// In order to land a change to an existing golden file, an ignore must be
+  /// set up in Flutter Gold. This will serve as a flag to permit the change to
+  /// land, protect against any unwanted changes, and ensure that changes that
+  /// have landed are triaged.
   Future<bool> testIsIgnoredForPullRequest(String pullRequest, String testName) async {
     bool ignoreIsActive = false;
     testName = cleanTestName(testName);
@@ -574,6 +610,7 @@ class SkiaGoldClient {
   String _getKeysJSON() {
     final Map<String, dynamic> keys = <String, dynamic>{
       'Platform' : platform.operatingSystem,
+      'CI' : ci.toString().split('.').last,
     };
     if (platform.environment[_kTestBrowserKey] != null)
       keys['Browser'] = platform.environment[_kTestBrowserKey];
@@ -601,6 +638,34 @@ class SkiaGoldClient {
     }
     return false;
   }
+
+  /// Returns a list of arguments for initializing a tryjob based on the testing
+  /// environment.
+  List<String> getCIArguments() {
+    String pullRequest;
+    String jobId;
+    String cis;
+
+    switch (ci) {
+      case ContinuousIntegrationEnvironment.luci:
+        jobId = platform.environment['LOGDOG_STREAM_PREFIX'].split('/').last;
+        final List<String> refs = platform.environment['GOLD_TRYJOB'].split('/');
+        pullRequest = refs[refs.length - 2];
+        cis = 'buildbucket';
+        break;
+      case ContinuousIntegrationEnvironment.cirrus:
+        pullRequest = platform.environment['CIRRUS_PR'];
+        jobId = platform.environment['CIRRUS_TASK_ID'];
+        cis = 'cirrus';
+        break;
+    }
+
+    return <String>[
+      '--changelist', pullRequest,
+      '--cis', cis,
+      '--jobid', jobId,
+    ];
+  }
 }
 
 /// Used to make HttpRequests during testing.
@@ -623,7 +688,10 @@ class SkiaGoldDigest {
     return SkiaGoldDigest(
       imageHash: json['digest'] as String,
       paramSet: Map<String, dynamic>.from(json['paramset'] as Map<String, dynamic> ??
-        <String, List<String>>{'Platform': <String>[]}),
+        <String, List<String>>{
+          'Platform': <String>[],
+          'Browser' : <String>[],
+        }),
       testName: json['test'] as String,
       status: json['status'] as String,
     );