Refactor golden_tests_harvester, throw when not --dry-run, add tests. (flutter/engine#51364)

Closes https://github.com/flutter/flutter/issues/144948.

I went a little farther, as I'm not looking forward to debugging this in the future without tests or examples.

engine/src/flutter/tools/golden_tests_harvester/README.md

@@ -1,14 +1,31 @@
 # Golden Tests Harvester
 
-Reaps the output of impeller's golden image tests and sends it to Skia gold.
+A command-line tool that uploads golden images to Skia gold from a directory.
 
 ## Usage
 
+This program assumes you've _already run_ a suite of golden tests that produce
+a directory of images with a JSON digest named `digests.json`, which is
+[documented in `lib/golden_tests_harvester.dart`][lib].
+
+Provide the directory as the only positional argument to the program:
+
+[lib]: lib/golden_tests_harvester.dart
+
 ```sh
-cd $SRC
-./out/host_debug_unopt_arm64/impeller_golden_tests --working_dir=~/Desktop/temp
-cd flutter/tools/golden_tests_harvester
-dart run ./bin/golden_tests_harvester.dart ~/Desktop/temp
+dart ./tools/golden_tests_harvester/bin/golden_tests_harvester.dart <path/to/digests>
 ```
 
-See also [golden_tests](../impeller/golden_tests/).
+> [!INFO]
+> Skia Gold must be setup and configured to accept the images being uploaded.
+>
+> In practice, that means it's the process is running on CI.
+
+_Optionally_, you can run in `--dry-run` mode to see what would be uploaded
+without actually uploading anything:
+
+```sh
+dart ./tools/golden_tests_harvester/bin/golden_tests_harvester.dart --dry-run <path/to/digests>
+```
+
+This flag is automatically set when running locally (i.e. outside of CI).

engine/src/flutter/tools/golden_tests_harvester/bin/golden_tests_harvester.dart

@@ -2,91 +2,76 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-import 'dart:convert';
-import 'dart:io';
+import 'dart:io' as io;
 
+import 'package:args/args.dart';
 import 'package:golden_tests_harvester/golden_tests_harvester.dart';
-import 'package:path/path.dart' as p;
-import 'package:process/src/interface/process_manager.dart';
 import 'package:skia_gold_client/skia_gold_client.dart';
 
-const String _kLuciEnvName = 'LUCI_CONTEXT';
+final bool _isLocalEnvWithoutSkiaGold =
+    !SkiaGoldClient.isAvailable(environment: io.Platform.environment) ||
+    !SkiaGoldClient.isLuciEnv(environment: io.Platform.environment);
 
-bool get isLuciEnv => Platform.environment.containsKey(_kLuciEnvName);
+final ArgParser _argParser = ArgParser()
+  ..addFlag(
+    'help',
+    abbr: 'h',
+    negatable: false,
+    help: 'Prints this usage information.',
+  )
+  ..addFlag(
+    'dry-run',
+    defaultsTo: _isLocalEnvWithoutSkiaGold,
+    help: 'Do not upload images to Skia Gold.',
+  );
 
-/// Fake SkiaGoldClient that is used if the harvester is run outside of Luci.
-class FakeSkiaGoldClient implements SkiaGoldClient {
-  FakeSkiaGoldClient(this._workingDirectory, {this.dimensions, this.verbose = false});
-
-  final Directory _workingDirectory;
-
-  @override
-  final Map<String, String>? dimensions;
-
-  @override
-  final bool verbose;
-
-  @override
-  Future<void> addImg(String testName, File goldenFile,
-      {double differentPixelsRate = 0.01,
-      int pixelColorDelta = 0,
-      required int screenshotSize}) async {
-    Logger.instance.log(
-        'addImg testName:$testName goldenFile:${goldenFile.path} screenshotSize:$screenshotSize differentPixelsRate:$differentPixelsRate pixelColorDelta:$pixelColorDelta');
-  }
-
-  @override
-  Future<void> auth() async {
-    Logger.instance.log('auth dimensions:${dimensions ?? 'null'}');
-  }
-
-  @override
-  Future<String?> getExpectationForTest(String testName) {
-    throw UnimplementedError();
-  }
-
-  @override
-  String getTraceID(String testName) {
-    throw UnimplementedError();
-  }
-
-  @override
-  HttpClient get httpClient => throw UnimplementedError();
-
-  @override
-  ProcessManager get process => throw UnimplementedError();
-
-  @override
-  Directory get workDirectory => _workingDirectory;
-}
-
-void _printUsage() {
-  Logger.instance
-      .log('dart run ./bin/golden_tests_harvester.dart <working_dir>');
-}
-
-Future<void> main(List<String> arguments) async {
-  if (arguments.length != 1) {
-    return _printUsage();
-  }
-
-  final Directory workDirectory = Directory(arguments[0]);
-
-  final File digest = File(p.join(workDirectory.path, 'digest.json'));
-  if (!digest.existsSync()) {
-    Logger.instance
-        .log('Error: digest.json does not exist in ${workDirectory.path}.');
+Future<void> main(List<String> args) async {
+  final ArgResults results = _argParser.parse(args);
+  if (results['help'] as bool) {
+    io.stdout.writeln(_argParser.usage);
     return;
   }
-  final Object? decoded = jsonDecode(digest.readAsStringSync());
-  final Map<String?, Object?> data = (decoded as Map<String?, Object?>?)!;
-  final Map<String, String> dimensions =
-      (data['dimensions'] as Map<String, Object?>?)!.cast<String, String>();
-  final List<Object?> entries = (data['entries'] as List<Object?>?)!;
 
-  final SkiaGoldClient skiaGoldClient = isLuciEnv
-      ? SkiaGoldClient(workDirectory, dimensions: dimensions)
-      : FakeSkiaGoldClient(workDirectory, dimensions: dimensions);
+  final List<String> rest = results.rest;
+  if (rest.length != 1) {
+    io.stderr.writeln('Error: Must provide exactly one argument.');
+    io.stderr.writeln(_argParser.usage);
+    io.exitCode = 1;
+    return;
+  }
 
-  await harvest(skiaGoldClient, workDirectory, entries);
+  final io.Directory workDirectory = io.Directory(rest.single);
+  final AddImageToSkiaGold addImg;
+  final bool dryRun = results['dry-run'] as bool;
+  if (dryRun) {
+    io.stderr.writeln('=== DRY RUN. Results not submitted to Skia Gold. ===');
+    addImg = _dryRunAddImg;
+  } else {
+    // If GOLDCTL is not configured (i.e. on CI), this will throw.
+    final SkiaGoldClient client = SkiaGoldClient(workDirectory);
+    await client.auth();
+    addImg = client.addImg;
+  }
+
+  await harvest(
+    workDirectory: workDirectory,
+    addImg: addImg,
+    stderr: io.stderr,
+  );
+}
+
+Future<void> _dryRunAddImg(
+  String testName,
+  io.File goldenFile, {
+  required int screenshotSize,
+  double differentPixelsRate = 0.01,
+  int pixelColorDelta = 0,
+}) async {
+  io.stderr.writeln('addImg '
+    'testName:$testName '
+    'goldenFile:${goldenFile.path} '
+    'screenshotSize:$screenshotSize '
+    'differentPixelsRate:$differentPixelsRate '
+    'pixelColorDelta:$pixelColorDelta',
+  );
 }

engine/src/flutter/tools/golden_tests_harvester/lib/golden_tests_harvester.dart

@@ -2,42 +2,105 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-import 'dart:io';
+import 'dart:io' as io;
 
 import 'package:path/path.dart' as p;
-import 'package:skia_gold_client/skia_gold_client.dart';
 
-import 'logger.dart';
-
-export 'logger.dart';
-
-/// Reads the digest inside of [workDirectory], sending tests to
-/// [skiaGoldClient].
-Future<void> harvest(SkiaGoldClient skiaGoldClient, Directory workDirectory,
-    List<Object?> entries) async {
-  await skiaGoldClient.auth();
+import 'src/digests_json_format.dart';
 
+/// Uploads the images of digests  in [workDirectory] to Skia Gold.
+///
+/// The directory is expected to match the following structure:
+/// ```txt
+/// workDirectory/
+///   - digest.json
+///   - test_name_1.png
+///   - test_name_2.png
+///   - ...
+/// ```
+///
+/// The format of `digest.json` is expected to match the following:
+/// ```jsonc
+/// {
+///   "dimensions": {
+///     // Key-value pairs of dimensions to provide to Skia Gold.
+///     // For example:
+///     "platform": "linux",
+///   },
+///   "entries": [
+///     // Each entry is a test-run with the following format:
+///     {
+///       // Path must be a direct sibling of digest.json.
+///       "filename": "test_name_1.png",
+///
+///       // Called `screenshotSize` in Skia Gold (width * height).
+///       "width": 100,
+///       "height": 100,
+///
+///       // Called `differentPixelsRate` in Skia Gold.
+///       "maxDiffPixelsPercent": 0.01,
+///
+///       // Called `pixelColorDelta` in Skia Gold.
+///       "maxColorDelta": 0
+///     }
+///   ]
+/// }
+/// ```
+Future<void> harvest({
+  required io.Directory workDirectory,
+  required AddImageToSkiaGold addImg,
+  required StringSink stderr,
+}) async {
+  final io.File file = io.File(p.join(workDirectory.path, 'digest.json'));
+  if (!file.existsSync()) {
+    // Check if the directory exists or if the file is just missing.
+    if (!workDirectory.existsSync()) {
+      throw ArgumentError('Directory not found: ${workDirectory.path}.');
+    }
+    // Lookup sibling files to help the user understand what's missing.
+    final List<io.FileSystemEntity> files = workDirectory.listSync();
+    throw StateError(
+      'File "digest.json" not found in ${workDirectory.path}.\n\n'
+      'Found files: ${files.map((io.FileSystemEntity e) => p.basename(e.path)).join(', ')}',
+    );
+  }
+  final Digests digests = Digests.parse(file.readAsStringSync());
   final List<Future<void>> pendingComparisons = <Future<void>>[];
-  for (final Object? entry in entries) {
-    final Map<String, Object?> map = (entry as Map<String, Object?>?)!;
-    final String filename = (map['filename'] as String?)!;
-    final int width = (map['width'] as int?)!;
-    final int height = (map['height'] as int?)!;
-    final double maxDiffPixelsPercent =
-        (map['maxDiffPixelsPercent'] as double?)!;
-    final int maxColorDelta = (map['maxColorDelta'] as int?)!;
-    final File goldenImage = File(p.join(workDirectory.path, filename));
-    final Future<void> future = skiaGoldClient
-        .addImg(filename, goldenImage,
-            screenshotSize: width * height,
-            differentPixelsRate: maxDiffPixelsPercent,
-            pixelColorDelta: maxColorDelta)
-        .catchError((dynamic err) {
-      Logger.instance.log('skia gold comparison failed: $err');
-      throw Exception('Failed comparison: $filename');
+  for (final DigestEntry entry in digests.entries) {
+    final io.File goldenFile = io.File(p.join(workDirectory.path, entry.filename));
+    final Future<void> future = addImg(
+      entry.filename,
+      goldenFile,
+      screenshotSize: entry.width * entry.height,
+      differentPixelsRate: entry.maxDiffPixelsPercent,
+      pixelColorDelta: entry.maxColorDelta,
+    ).catchError((Object e) {
+      stderr.writeln('Failed to add image to Skia Gold: $e');
+      throw FailedComparisonException(entry.filename);
     });
     pendingComparisons.add(future);
   }
 
   await Future.wait(pendingComparisons);
 }
+
+/// An exception thrown when a comparison fails.
+final class FailedComparisonException implements Exception {
+  /// Creates a new instance of [FailedComparisonException].
+  const FailedComparisonException(this.testName);
+
+  /// The test name that failed.
+  final String testName;
+
+  @override
+  String toString() => 'Failed comparison: $testName';
+}
+
+/// A function that uploads an image to Skia Gold.
+typedef AddImageToSkiaGold = Future<void> Function(
+  String testName,
+  io.File goldenFile, {
+  double differentPixelsRate,
+  int pixelColorDelta,
+  required int screenshotSize,
+});

engine/src/flutter/tools/golden_tests_harvester/lib/logger.dart

@@ -1,20 +0,0 @@
-// Copyright 2013 The Flutter Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-/// Logs messages to the appropriate console.
-class Logger {
-  Logger._();
-
-  /// Singleton accessor for the [Logger].
-  static Logger get instance {
-    _instance ??= Logger._();
-    return _instance!;
-  }
-
-  static Logger? _instance;
-
-  /// Log [message] to the console.
-  // ignore: avoid_print
-  void log(String message) => print(message);
-}

engine/src/flutter/tools/golden_tests_harvester/lib/src/digests_json_format.dart

@@ -0,0 +1,120 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+import 'dart:convert' as convert;
+
+/// A Dart representation of a `digest.json` file.
+///
+/// A single file is typically used to represent the results of a test suite,
+/// or a series of tests that have been run with the same [dimensions]. For
+/// example, "Impeller Unittests on Vulkan running on MacOS" might generate
+/// a `digest.json` file.
+///
+/// Other tools (perhaps implemented in other languages, like C++) can use this
+/// format to communicate with the `golden_tests_harvester` tool without
+/// relying on the tool directly (or the Dart SDK).
+final class Digests {
+  /// Creates a new instance of [Digests].
+  ///
+  /// In practice, [Digests.parse] is typically used to create a new instance
+  /// from an existing `digest.json` file read into memory as string contents.
+  const Digests({
+    required this.dimensions,
+    required this.entries,
+  });
+
+  /// Parses a `digest.json` file from a string.
+  factory Digests.parse(String json) {
+    final Object? decoded = convert.json.decode(json);
+    if (decoded is! Map<String, Object?>) {
+      throw FormatException(
+        'Expected a JSON object as the root, but got $decoded',
+        json,
+      );
+    }
+
+    final Object? dimensions = decoded['dimensions'];
+    if (dimensions is! Map<String, Object?>) {
+      throw FormatException(
+        'Expected a JSON object "dimensions", but got ${dimensions.runtimeType}',
+        dimensions,
+      );
+    }
+
+    final Object? entries = decoded['entries'];
+    if (entries is! List<Object?>) {
+      throw FormatException(
+        'Expected a JSON list "entries", but got ${entries.runtimeType}',
+        entries,
+      );
+    }
+
+    // Now parse the entries.
+    return Digests(
+      dimensions: dimensions.map((String key, Object? value) {
+        if (value is! String) {
+          throw FormatException(
+            'Expected a JSON string for dimension "$key", but got ${value.runtimeType}',
+            value,
+          );
+        }
+        return MapEntry<String, String>(key, value);
+      }),
+      entries: List<DigestEntry>.unmodifiable(entries.map((Object? entry) {
+        if (entry is! Map<String, Object?>) {
+          throw FormatException(
+            'Expected a JSON object for an entry, but got ${entry.runtimeType}',
+            entry,
+          );
+        }
+        return DigestEntry(
+          filename: entry['filename']! as String,
+          width: entry['width']! as int,
+          height: entry['height']! as int,
+          maxDiffPixelsPercent: entry['maxDiffPixelsPercent']! as double,
+          maxColorDelta: entry['maxColorDelta']! as int,
+        );
+      })),
+    );
+  }
+
+  /// A key-value map of dimensions to provide to Skia Gold.
+  final Map<String, String> dimensions;
+
+  /// A list of test-run entries.
+  final List<DigestEntry> entries;
+}
+
+/// A single entry in a `digest.json` file.
+///
+/// Each entry is a test-run (or part of a test-run).
+final class DigestEntry {
+  /// Creates a new instance of [DigestEntry].
+  const DigestEntry({
+    required this.filename,
+    required this.width,
+    required this.height,
+    required this.maxDiffPixelsPercent,
+    required this.maxColorDelta,
+  });
+
+  /// File path that is a direct sibling of the parsed `digest.json`.
+  final String filename;
+
+  /// Width of the image.
+  final int width;
+
+  /// Height of the image.
+  final int height;
+
+  /// Maximum percentage of different pixels.
+  ///
+  /// Within Skia Gold, this is called `differentPixelsRate`.
+  final double maxDiffPixelsPercent;
+
+  /// Maximum color delta.
+  ///
+  /// Within Skia Gold, this is called `pixelColorDelta`.
+  final int maxColorDelta;
+}

engine/src/flutter/tools/golden_tests_harvester/pubspec.yaml

@@ -15,19 +15,30 @@ environment:
 # check the .packages and .package_config to make sure all the paths are
 # relative to this directory into //third_party/dart, or //third_party/pkg
 dependencies:
-  crypto: any
+  args: any
+  meta: any
   path: any
-  process: any
   skia_gold_client:
     path: ../../testing/skia_gold_client
 
+dev_dependencies:
+  litetest: any
+
 dependency_overrides:
+  args:
+    path: ../../../third_party/dart/third_party/pkg/args
+  async_helper:
+    path: ../../../third_party/dart/pkg/async_helper
   collection:
     path: ../../../third_party/dart/third_party/pkg/collection
   crypto:
     path: ../../../third_party/dart/third_party/pkg/crypto
+  expect:
+    path: ../../../third_party/dart/pkg/expect
   file:
     path: ../../../third_party/dart/third_party/pkg/file/packages/file
+  litetest:
+    path: ../../testing/litetest
   meta:
     path: ../../../third_party/dart/pkg/meta
   engine_repo_tools:
@@ -38,5 +49,7 @@ dependency_overrides:
     path: ../../third_party/pkg/platform
   process:
     path: ../../third_party/pkg/process
+  smith:
+    path: ../../../third_party/dart/pkg/smith
   typed_data:
     path: ../../../third_party/dart/third_party/pkg/typed_data

engine/src/flutter/tools/golden_tests_harvester/test/golden_tests_harvester_test.dart

@@ -2,4 +2,172 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-void main() {}
+import 'dart:async';
+import 'dart:io' as io;
+
+import 'package:golden_tests_harvester/golden_tests_harvester.dart';
+import 'package:litetest/litetest.dart';
+import 'package:path/path.dart' as p;
+
+void main() async {
+  Future<void> withTempDirectory(FutureOr<void> Function(io.Directory) callback) async {
+    final io.Directory tempDirectory = await io.Directory.systemTemp.createTemp('golden_tests_harvester_test.');
+    try {
+      await callback(tempDirectory);
+    } finally {
+      await tempDirectory.delete(recursive: true);
+    }
+  }
+
+  test('should fail on a missing directory', () async {
+    await withTempDirectory((io.Directory tempDirectory) async {
+      final StringSink stderr = StringBuffer();
+      final ArgumentError error = await _expectThrow<ArgumentError>(() async {
+        await harvest(
+          workDirectory: io.Directory(p.join(tempDirectory.path, 'non_existent')),
+          addImg: _alwaysThrowsAddImg,
+          stderr: stderr,
+        );
+      });
+      expect(error.message, contains('non_existent'));
+      expect(stderr.toString(), isEmpty);
+    });
+  });
+
+  test('should require a file named "digest.json" in the working directory', () async {
+    await withTempDirectory((io.Directory tempDirectory) async {
+      final StringSink stderr = StringBuffer();
+
+      final StateError error = await _expectThrow<StateError>(() async {
+        await harvest(
+          workDirectory: tempDirectory,
+          addImg: _alwaysThrowsAddImg,
+          stderr: stderr,
+        );
+      });
+      expect(error.toString(), contains('digest.json'));
+      expect(stderr.toString(), isEmpty);
+    });
+  });
+
+  test('should throw if "digest.json" is in an unexpected format', () async {
+    await withTempDirectory((io.Directory tempDirectory) async {
+      final StringSink stderr = StringBuffer();
+      final io.File digestsFile = io.File(p.join(tempDirectory.path, 'digest.json'));
+      await digestsFile.writeAsString('{"dimensions": "not a map", "entries": []}');
+
+      final FormatException error = await _expectThrow<FormatException>(() async {
+        await harvest(
+          workDirectory: tempDirectory,
+          addImg: _alwaysThrowsAddImg,
+          stderr: stderr,
+        );
+      });
+      expect(error.message, contains('dimensions'));
+      expect(stderr.toString(), isEmpty);
+    });
+  });
+
+  test('should fail eagerly if addImg fails', () async {
+    await withTempDirectory((io.Directory tempDirectory) async {
+      final io.File digestsFile = io.File(p.join(tempDirectory.path, 'digest.json'));
+      final StringSink stderr = StringBuffer();
+      await digestsFile.writeAsString('''
+        {
+          "dimensions": {},
+          "entries": [
+            {
+              "filename": "test_name_1.png",
+              "width": 100,
+              "height": 100,
+              "maxDiffPixelsPercent": 0.01,
+              "maxColorDelta": 0
+            }
+          ]
+        }
+      ''');
+
+      final FailedComparisonException error = await _expectThrow<FailedComparisonException>(() async {
+        await harvest(
+          workDirectory: tempDirectory,
+          addImg: _alwaysThrowsAddImg,
+          stderr: stderr,
+        );
+      });
+      expect(error.testName, 'test_name_1.png');
+      expect(stderr.toString(), contains('IntentionalError'));
+    });
+  });
+
+  test('should invoke addImg per test', () async {
+    await withTempDirectory((io.Directory tempDirectory) async {
+      final io.File digestsFile = io.File(p.join(tempDirectory.path, 'digest.json'));
+      await digestsFile.writeAsString('''
+        {
+          "dimensions": {},
+          "entries": [
+            {
+              "filename": "test_name_1.png",
+              "width": 100,
+              "height": 100,
+              "maxDiffPixelsPercent": 0.01,
+              "maxColorDelta": 0
+            },
+            {
+              "filename": "test_name_2.png",
+              "width": 200,
+              "height": 200,
+              "maxDiffPixelsPercent": 0.02,
+              "maxColorDelta": 1
+            }
+          ]
+        }
+      ''');
+      final List<String> addImgCalls = <String>[];
+      final StringSink stderr = StringBuffer();
+      await harvest(
+        workDirectory: tempDirectory,
+        addImg: (
+          String testName,
+          io.File goldenFile, {
+          required int screenshotSize,
+          double differentPixelsRate = 0.01,
+          int pixelColorDelta = 0,
+        }) async {
+          addImgCalls.add('$testName $screenshotSize $differentPixelsRate $pixelColorDelta');
+        },
+        stderr: stderr,
+      );
+      expect(addImgCalls, <String>[
+        'test_name_1.png 10000 0.01 0',
+        'test_name_2.png 40000 0.02 1',
+      ]);
+    });
+
+  });
+}
+
+FutureOr<T> _expectThrow<T extends Object>(FutureOr<void> Function() callback) async {
+  try {
+    await callback();
+    fail('Expected an exception of type $T');
+  } on T catch (e) {
+    return e;
+  } catch (e) {
+    fail('Expected an exception of type $T, but got $e');
+  }
+  // fail(...) unfortunately does not return Never, but it does always throw.
+  throw UnsupportedError('Unreachable');
+}
+
+final class _IntentionalError extends Error {}
+
+Future<void> _alwaysThrowsAddImg(
+  String testName,
+  io.File goldenFile, {
+  required int screenshotSize,
+  double differentPixelsRate = 0.01,
+  int pixelColorDelta = 0,
+}) async {
+  throw _IntentionalError();
+}