From e889b287f1518711fd728af075462bed403b4fe3 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 12 Mar 2024 20:45:17 -0700 Subject: [PATCH] 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. --- .../tools/golden_tests_harvester/README.md | 29 ++- .../bin/golden_tests_harvester.dart | 139 +++++++------- .../lib/golden_tests_harvester.dart | 119 +++++++++--- .../golden_tests_harvester/lib/logger.dart | 20 --- .../lib/src/digests_json_format.dart | 120 +++++++++++++ .../tools/golden_tests_harvester/pubspec.yaml | 17 +- .../test/golden_tests_harvester_test.dart | 170 +++++++++++++++++- 7 files changed, 480 insertions(+), 134 deletions(-) delete mode 100644 engine/src/flutter/tools/golden_tests_harvester/lib/logger.dart create mode 100644 engine/src/flutter/tools/golden_tests_harvester/lib/src/digests_json_format.dart diff --git a/engine/src/flutter/tools/golden_tests_harvester/README.md b/engine/src/flutter/tools/golden_tests_harvester/README.md index b3d6451e28..379ebace6b 100644 --- a/engine/src/flutter/tools/golden_tests_harvester/README.md +++ b/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 ``` -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 +``` + +This flag is automatically set when running locally (i.e. outside of CI). diff --git a/engine/src/flutter/tools/golden_tests_harvester/bin/golden_tests_harvester.dart b/engine/src/flutter/tools/golden_tests_harvester/bin/golden_tests_harvester.dart index 021d97505c..a4d318e046 100644 --- a/engine/src/flutter/tools/golden_tests_harvester/bin/golden_tests_harvester.dart +++ b/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? dimensions; - - @override - final bool verbose; - - @override - Future 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 auth() async { - Logger.instance.log('auth dimensions:${dimensions ?? 'null'}'); - } - - @override - Future 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 '); -} - -Future main(List 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 main(List 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 data = (decoded as Map?)!; - final Map dimensions = - (data['dimensions'] as Map?)!.cast(); - final List entries = (data['entries'] as List?)!; - final SkiaGoldClient skiaGoldClient = isLuciEnv - ? SkiaGoldClient(workDirectory, dimensions: dimensions) - : FakeSkiaGoldClient(workDirectory, dimensions: dimensions); + final List 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 _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', + ); } diff --git a/engine/src/flutter/tools/golden_tests_harvester/lib/golden_tests_harvester.dart b/engine/src/flutter/tools/golden_tests_harvester/lib/golden_tests_harvester.dart index dfdc647c7c..1568cf2b3d 100644 --- a/engine/src/flutter/tools/golden_tests_harvester/lib/golden_tests_harvester.dart +++ b/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 harvest(SkiaGoldClient skiaGoldClient, Directory workDirectory, - List 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 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 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> pendingComparisons = >[]; - for (final Object? entry in entries) { - final Map map = (entry as Map?)!; - 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 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 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 Function( + String testName, + io.File goldenFile, { + double differentPixelsRate, + int pixelColorDelta, + required int screenshotSize, +}); diff --git a/engine/src/flutter/tools/golden_tests_harvester/lib/logger.dart b/engine/src/flutter/tools/golden_tests_harvester/lib/logger.dart deleted file mode 100644 index b9f5818b37..0000000000 --- a/engine/src/flutter/tools/golden_tests_harvester/lib/logger.dart +++ /dev/null @@ -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); -} diff --git a/engine/src/flutter/tools/golden_tests_harvester/lib/src/digests_json_format.dart b/engine/src/flutter/tools/golden_tests_harvester/lib/src/digests_json_format.dart new file mode 100644 index 0000000000..8a2674f5e1 --- /dev/null +++ b/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) { + throw FormatException( + 'Expected a JSON object as the root, but got $decoded', + json, + ); + } + + final Object? dimensions = decoded['dimensions']; + if (dimensions is! Map) { + throw FormatException( + 'Expected a JSON object "dimensions", but got ${dimensions.runtimeType}', + dimensions, + ); + } + + final Object? entries = decoded['entries']; + if (entries is! List) { + 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(key, value); + }), + entries: List.unmodifiable(entries.map((Object? entry) { + if (entry is! Map) { + 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 dimensions; + + /// A list of test-run entries. + final List 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; +} diff --git a/engine/src/flutter/tools/golden_tests_harvester/pubspec.yaml b/engine/src/flutter/tools/golden_tests_harvester/pubspec.yaml index 3621938e45..933beeac0e 100644 --- a/engine/src/flutter/tools/golden_tests_harvester/pubspec.yaml +++ b/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 diff --git a/engine/src/flutter/tools/golden_tests_harvester/test/golden_tests_harvester_test.dart b/engine/src/flutter/tools/golden_tests_harvester/test/golden_tests_harvester_test.dart index f9b0dd79fe..9eec6ccfca 100644 --- a/engine/src/flutter/tools/golden_tests_harvester/test/golden_tests_harvester_test.dart +++ b/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 withTempDirectory(FutureOr 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(() 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(() 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(() 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(() 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 addImgCalls = []; + 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, [ + 'test_name_1.png 10000 0.01 0', + 'test_name_2.png 40000 0.02 1', + ]); + }); + + }); +} + +FutureOr _expectThrow(FutureOr 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 _alwaysThrowsAddImg( + String testName, + io.File goldenFile, { + required int screenshotSize, + double differentPixelsRate = 0.01, + int pixelColorDelta = 0, +}) async { + throw _IntentionalError(); +}