Initialize the Skia client more efficiently (#95055)

packages/flutter_goldens/lib/flutter_goldens.dart

@@ -227,12 +227,12 @@ class FlutterPostSubmitFileComparator extends FlutterGoldenFileComparator {
 
     goldens ??= SkiaGoldClient(baseDirectory);
     await goldens.auth();
-    await goldens.imgtestInit();
     return FlutterPostSubmitFileComparator(baseDirectory.uri, goldens);
   }
 
   @override
   Future<bool> compare(Uint8List imageBytes, Uri golden) async {
+    await skiaClient.imgtestInit();
     golden = _addPrefix(golden);
     await update(golden, imageBytes);
     final File goldenFile = getGoldenFile(golden);
@@ -309,16 +309,15 @@ class FlutterPreSubmitFileComparator extends FlutterGoldenFileComparator {
     goldens ??= SkiaGoldClient(baseDirectory);
 
     await goldens.auth();
-    await goldens.tryjobInit();
     return FlutterPreSubmitFileComparator(
       baseDirectory.uri,
-        goldens,
-        platform: platform,
+      goldens, platform: platform,
     );
   }
 
   @override
   Future<bool> compare(Uint8List imageBytes, Uri golden) async {
+    await skiaClient.tryjobInit();
     golden = _addPrefix(golden);
     await update(golden, imageBytes);
     final File goldenFile = getGoldenFile(golden);

packages/flutter_goldens/test/flutter_goldens_test.dart

@@ -100,7 +100,7 @@ void main() {
         httpClient: fakeHttpClient,
       );
 
-      process.fallbackProcessResult = ProcessResult(123, 1, 'fail', 'fail');
+      process.fallbackProcessResult = ProcessResult(123, 1, 'Fallback failure', 'Fallback failure');
 
       expect(
         skiaClient.auth(),
@@ -125,8 +125,6 @@ void main() {
         httpClient: fakeHttpClient,
       );
 
-      process.fallbackProcessResult = ProcessResult(123, 1, 'fail', 'fail');
-
       const RunInvocation gitInvocation = RunInvocation(
         <String>['git', 'rev-parse', 'HEAD'],
         '/flutter',
@@ -145,7 +143,8 @@ void main() {
         null,
       );
       process.processResults[gitInvocation] = ProcessResult(12345678, 0, '12345678', '');
-      process.processResults[goldctlInvocation] = ProcessResult(123, 1, 'fail', 'fail');
+      process.processResults[goldctlInvocation] = ProcessResult(123, 1, 'Expected failure', 'Expected failure');
+      process.fallbackProcessResult = ProcessResult(123, 1, 'Fallback failure', 'Fallback failure');
 
       expect(
         skiaClient.imgtestInit(),
@@ -153,6 +152,112 @@ void main() {
       );
     });
 
+    test('Only calls init once', () async {
+      platform = FakePlatform(
+        environment: <String, String>{
+          'FLUTTER_ROOT': _kFlutterRoot,
+          'GOLDCTL' : 'goldctl',
+        },
+        operatingSystem: 'macos'
+      );
+
+      skiaClient = SkiaGoldClient(
+        workDirectory,
+        fs: fs,
+        process: process,
+        platform: platform,
+        httpClient: fakeHttpClient,
+      );
+
+      const RunInvocation gitInvocation = RunInvocation(
+        <String>['git', 'rev-parse', 'HEAD'],
+        '/flutter',
+      );
+      const RunInvocation goldctlInvocation = RunInvocation(
+        <String>[
+          'goldctl',
+          'imgtest', 'init',
+          '--instance', 'flutter',
+          '--work-dir', '/workDirectory/temp',
+          '--commit', '1234',
+          '--keys-file', '/workDirectory/keys.json',
+          '--failure-file', '/workDirectory/failures.json',
+          '--passfail',
+        ],
+        null,
+      );
+      process.processResults[gitInvocation] = ProcessResult(1234, 0, '1234', '');
+      process.processResults[goldctlInvocation] = ProcessResult(5678, 0, '5678', '');
+      process.fallbackProcessResult = ProcessResult(123, 1, 'Fallback failure', 'Fallback failure');
+
+      // First call
+      await skiaClient.imgtestInit();
+
+      // Remove fake process result.
+      // If the init call is executed again, the fallback process will throw.
+      process.processResults.remove(goldctlInvocation);
+
+      // Second call
+      await skiaClient.imgtestInit();
+    });
+
+    test('Only calls tryjob init once', () 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: fakeHttpClient,
+      );
+
+      const RunInvocation gitInvocation = RunInvocation(
+        <String>['git', 'rev-parse', 'HEAD'],
+        '/flutter',
+      );
+      const RunInvocation goldctlInvocation = RunInvocation(
+        <String>[
+          'goldctl',
+          'imgtest', 'init',
+          '--instance', 'flutter',
+          '--work-dir', '/workDirectory/temp',
+          '--commit', '1234',
+          '--keys-file', '/workDirectory/keys.json',
+          '--failure-file', '/workDirectory/failures.json',
+          '--passfail',
+          '--crs', 'github',
+          '--patchset_id', '1234',
+          '--changelist', '49815',
+          '--cis', 'buildbucket',
+          '--jobid', '8885996262141582672',
+        ],
+        null,
+      );
+      process.processResults[gitInvocation] = ProcessResult(1234, 0, '1234', '');
+      process.processResults[goldctlInvocation] = ProcessResult(5678, 0, '5678', '');
+      process.fallbackProcessResult = ProcessResult(123, 1, 'Fallback failure', 'Fallback failure');
+
+      // First call
+      await skiaClient.tryjobInit();
+
+      // Remove fake process result.
+      // If the init call is executed again, the fallback process will throw.
+      process.processResults.remove(goldctlInvocation);
+
+      // Second call
+      await skiaClient.tryjobInit();
+    });
+
     test('throws for error state from imgtestAdd', () {
       final File goldenFile = fs.file('/workDirectory/temp/golden_file_test.png')
         ..createSync(recursive: true);
@@ -172,7 +277,6 @@ void main() {
         httpClient: fakeHttpClient,
       );
 
-      process.fallbackProcessResult = ProcessResult(123, 1, 'fail', 'fail');
       const RunInvocation goldctlInvocation = RunInvocation(
         <String>[
           'goldctl',
@@ -184,7 +288,8 @@ void main() {
         ],
         null,
       );
-      process.processResults[goldctlInvocation] = ProcessResult(123, 1, 'fail', 'fail');
+      process.processResults[goldctlInvocation] = ProcessResult(123, 1, 'Expected failure', 'Expected failure');
+      process.fallbackProcessResult = ProcessResult(123, 1, 'Fallback failure', 'Fallback failure');
 
       expect(
         skiaClient.imgtestAdd('golden_file_test', goldenFile),
@@ -327,7 +432,7 @@ void main() {
   });
 
   group('FlutterGoldenFileComparator', () {
-    late FlutterPostSubmitFileComparator comparator;
+    late FlutterGoldenFileComparator comparator;
 
     setUp(() {
       final Directory basedir = fs.directory('flutter/test/library/')
@@ -362,9 +467,10 @@ void main() {
     });
 
     group('Post-Submit', () {
-      final FakeSkiaGoldClient fakeSkiaClient = FakeSkiaGoldClient();
+      late FakeSkiaGoldClient fakeSkiaClient;
 
       setUp(() {
+        fakeSkiaClient = FakeSkiaGoldClient();
         final Directory basedir = fs.directory('flutter/test/library/')
           ..createSync(recursive: true);
         comparator = FlutterPostSubmitFileComparator(
@@ -375,6 +481,24 @@ void main() {
         );
       });
 
+      test('calls init during compare', () {
+        expect(fakeSkiaClient.initCalls, 0);
+        comparator.compare(
+          Uint8List.fromList(_kTestPngBytes),
+          Uri.parse('flutter.golden_test.1.png'),
+        );
+        expect(fakeSkiaClient.initCalls, 1);
+      });
+
+      test('does not call init in during construction', () {
+        expect(fakeSkiaClient.initCalls, 0);
+        FlutterPostSubmitFileComparator.fromDefaultComparator(
+          platform,
+          goldens: fakeSkiaClient,
+        );
+        expect(fakeSkiaClient.initCalls, 0);
+      });
+
       group('correctly determines testing environment', () {
         test('returns true for configured Luci', () {
           platform = FakePlatform(
@@ -441,6 +565,38 @@ void main() {
     });
 
     group('Pre-Submit', () {
+      late FakeSkiaGoldClient fakeSkiaClient;
+
+      setUp(() {
+        fakeSkiaClient = FakeSkiaGoldClient();
+        final Directory basedir = fs.directory('flutter/test/library/')
+          ..createSync(recursive: true);
+        comparator = FlutterPreSubmitFileComparator(
+          basedir.uri,
+          fakeSkiaClient,
+          fs: fs,
+          platform: platform,
+        );
+      });
+
+      test('calls init during compare', () {
+        expect(fakeSkiaClient.tryInitCalls, 0);
+        comparator.compare(
+          Uint8List.fromList(_kTestPngBytes),
+          Uri.parse('flutter.golden_test.1.png'),
+        );
+        expect(fakeSkiaClient.tryInitCalls, 1);
+      });
+
+      test('does not call init in during construction', () {
+        expect(fakeSkiaClient.tryInitCalls, 0);
+        FlutterPostSubmitFileComparator.fromDefaultComparator(
+          platform,
+          goldens: fakeSkiaClient,
+        );
+        expect(fakeSkiaClient.tryInitCalls, 0);
+      });
+
       group('correctly determines testing environment', () {
         test('returns true for Luci', () {
           platform = FakePlatform(
@@ -707,6 +863,21 @@ class FakeSkiaGoldClient extends Fake implements SkiaGoldClient {
     return expectationForTestValues[testName] ?? '';
   }
 
+  @override
+  Future<void> auth() async {}
+
+  int initCalls = 0;
+  @override
+  Future<void> imgtestInit() async => initCalls += 1;
+  @override
+  Future<bool> imgtestAdd(String testName, File goldenFile) async => true;
+
+  int tryInitCalls = 0;
+  @override
+  Future<void> tryjobInit() async => tryInitCalls += 1;
+  @override
+  Future<bool> tryjobAdd(String testName, File goldenFile) async => true;
+
   Map<String, List<int>> imageBytesValues = <String, List<int>>{};
   @override
   Future<List<int>> getImageBytes(String imageHash) async => imageBytesValues[imageHash]!;

packages/flutter_goldens_client/lib/skia_client.dart

@@ -109,12 +109,24 @@ class SkiaGoldClient {
     }
   }
 
+  /// Signals if this client is initialized for uploading images to the Gold
+  /// service.
+  ///
+  /// Since Flutter framework tests are executed in parallel, and in random
+  /// order, this will signal is this instance of the Gold client has been
+  /// initialized.
+  bool _initialized = false;
+
   /// 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. Used by the
   /// [FlutterPostSubmitFileComparator].
   Future<void> imgtestInit() async {
+    // This client has already been intialized
+    if (_initialized)
+      return;
+
     final File keys = workDirectory.childFile('keys.json');
     final File failures = workDirectory.childFile('failures.json');
 
@@ -147,6 +159,7 @@ class SkiaGoldClient {
     final io.ProcessResult result = await process.run(imgtestInitCommand);
 
     if (result.exitCode != 0) {
+      _initialized = false;
       final StringBuffer buf = StringBuffer()
         ..writeln('Skia Gold imgtest init failed.')
         ..writeln('An error occurred when initializing golden file test with ')
@@ -157,6 +170,7 @@ class SkiaGoldClient {
         ..writeln('stderr: ${result.stderr}');
       throw Exception(buf.toString());
     }
+    _initialized = true;
   }
 
   /// Executes the `imgtest add` command in the goldctl tool.
@@ -204,12 +218,24 @@ class SkiaGoldClient {
     return true;
   }
 
+  /// Signals if this client is initialized for uploading tryjobs to the Gold
+  /// service.
+  ///
+  /// Since Flutter framework tests are executed in parallel, and in random
+  /// order, this will signal is this instance of the Gold client has been
+  /// initialized for tryjobs.
+  bool _tryjobInitialized = false;
+
   /// 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. Used by the
   /// [FlutterPreSubmitFileComparator].
   Future<void> tryjobInit() async {
+    // This client has already been initialized
+    if (_tryjobInitialized)
+      return;
+
     final File keys = workDirectory.childFile('keys.json');
     final File failures = workDirectory.childFile('failures.json');
 
@@ -245,6 +271,7 @@ class SkiaGoldClient {
     final io.ProcessResult result = await process.run(imgtestInitCommand);
 
     if (result.exitCode != 0) {
+      _tryjobInitialized = false;
       final StringBuffer buf = StringBuffer()
         ..writeln('Skia Gold tryjobInit failure.')
         ..writeln('An error occurred when initializing golden file tryjob with ')
@@ -255,6 +282,7 @@ class SkiaGoldClient {
         ..writeln('stderr: ${result.stderr}');
       throw Exception(buf.toString());
     }
+    _tryjobInitialized = true;
   }
 
   /// Executes the `imgtest add` command in the goldctl tool for tryjobs.