From cda35151265f00fab84d8128b086d5534f690c1b Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 6 Jan 2025 19:37:36 -0800 Subject: [PATCH] Use `flutter` repo for engine golds instead of `flutter-engine`. (#160556) Closes https://github.com/flutter/flutter/issues/157206. I also added a `prefix` that will default to `engine.` to avoid accidentally stomping on golden names across repos. /cc @gaaclarke for visibility, @Piinks for visibility. (I would love to get rid of this "engine copy" of the client as part of longer-term mono repo deduplication). --- .../src/flutter/testing/dart/canvas_test.dart | 2 +- .../lib/skia_gold_client.dart | 24 +++++++-- .../test/skia_gold_client_test.dart | 51 ++++++++++++++++++- 3 files changed, 70 insertions(+), 7 deletions(-) diff --git a/engine/src/flutter/testing/dart/canvas_test.dart b/engine/src/flutter/testing/dart/canvas_test.dart index f68c26a47b348..60bfb20ac1631 100644 --- a/engine/src/flutter/testing/dart/canvas_test.dart +++ b/engine/src/flutter/testing/dart/canvas_test.dart @@ -173,7 +173,7 @@ String get _flutterBuildPath { } void main() async { - final ImageComparer comparer = await ImageComparer.create(); + final ImageComparer comparer = await ImageComparer.create(verbose: true); testNoCrashes(); diff --git a/engine/src/flutter/testing/skia_gold_client/lib/skia_gold_client.dart b/engine/src/flutter/testing/skia_gold_client/lib/skia_gold_client.dart index 84c27c06d50e2..8f88e54b2e309 100644 --- a/engine/src/flutter/testing/skia_gold_client/lib/skia_gold_client.dart +++ b/engine/src/flutter/testing/skia_gold_client/lib/skia_gold_client.dart @@ -20,8 +20,8 @@ const String _kGoldctlKey = 'GOLDCTL'; const String _kPresubmitEnvName = 'GOLD_TRYJOB'; const String _kLuciEnvName = 'LUCI_CONTEXT'; -const String _skiaGoldHost = 'https://flutter-engine-gold.skia.org'; -const String _instance = 'flutter-engine'; +const String _skiaGoldHost = 'https://flutter-gold.skia.org'; +const String _instance = 'flutter'; /// Uploads images and makes baseline requests to Skia Gold. /// @@ -47,10 +47,16 @@ interface class SkiaGoldClient { /// spammy for regular use. factory SkiaGoldClient( io.Directory workDirectory, { + String prefix = 'engine.', Map? dimensions, bool verbose = false, }) { - return SkiaGoldClient.forTesting(workDirectory, dimensions: dimensions, verbose: verbose); + return SkiaGoldClient.forTesting( + workDirectory, + dimensions: dimensions, + verbose: verbose, + prefix: prefix, + ); } /// Creates a [SkiaGoldClient] for testing. @@ -73,6 +79,7 @@ interface class SkiaGoldClient { this.workDirectory, { this.dimensions, this.verbose = false, + String? prefix, io.HttpClient? httpClient, ProcessManager? processManager, StringSink? stderr, @@ -80,6 +87,7 @@ interface class SkiaGoldClient { Engine? engineRoot, }) : httpClient = httpClient ?? io.HttpClient(), process = processManager ?? const LocalProcessManager(), + _prefix = prefix, _stderr = stderr ?? io.stderr, _environment = environment ?? io.Platform.environment, _engineRoot = engineRoot ?? Engine.findWithin() { @@ -166,6 +174,9 @@ interface class SkiaGoldClient { /// client will create image and JSON files for the `goldctl` tool to use. final io.Directory workDirectory; + /// Prefix to add to all test names, if any. + final String? _prefix; + String get _tempPath => path.join(workDirectory.path, 'temp'); String get _keysPath => path.join(workDirectory.path, 'keys.json'); String get _failuresPath => path.join(workDirectory.path, 'failures.json'); @@ -345,6 +356,11 @@ interface class SkiaGoldClient { // Clean the test name to remove the file extension. testName = path.basenameWithoutExtension(testName); + // Add a prefix to avoid repo-wide conflicts. + if (_prefix != null) { + testName = '$_prefix$testName'; + } + // In release branches, we add a unique test suffix to the test name. // For example "testName" -> "testName_Release_3_21". // See ../README.md#release-testing for more information. @@ -408,7 +424,7 @@ interface class SkiaGoldClient { ..writeln('testing. Golden file images in flutter/engine are triaged ') ..writeln('in pre-submit during code review for the given PR.') ..writeln() - ..writeln('Visit https://flutter-engine-gold.skia.org/ to view and approve ') + ..writeln('Visit https://flutter-gold.skia.org/ to view and approve ') ..writeln('the image(s), or revert the associated change. For more ') ..writeln('information, visit the wiki: ') ..writeln( diff --git a/engine/src/flutter/testing/skia_gold_client/test/skia_gold_client_test.dart b/engine/src/flutter/testing/skia_gold_client/test/skia_gold_client_test.dart index ebc9298bee023..3361fe954d7d2 100644 --- a/engine/src/flutter/testing/skia_gold_client/test/skia_gold_client_test.dart +++ b/engine/src/flutter/testing/skia_gold_client/test/skia_gold_client_test.dart @@ -22,7 +22,7 @@ void main() { const Map presubmitEnv = { 'GIT_BRANCH': 'master', 'GOLDCTL': 'python tools/goldctl.py', - 'GOLD_TRYJOB': 'flutter/engine/1234567890', + 'GOLD_TRYJOB': 'flutter/flutter/1234567890', 'LOGDOG_STREAM_PREFIX': 'buildbucket/cr-buildbucket.appspot.com/1234567890/+/logdog', 'LUCI_CONTEXT': '{}', }; @@ -50,6 +50,7 @@ void main() { required Map environment, ReleaseVersion? engineVersion, Map? dimensions, + String? prefix, bool verbose = false, io.ProcessResult Function(List command) onRun = _runUnhandled, }) { @@ -62,6 +63,7 @@ void main() { verbose: verbose, stderr: fixture.outputSink, environment: environment, + prefix: prefix, ); } @@ -306,6 +308,51 @@ void main() { } }); + test('addImg uses prefix, if specified', () async { + final _TestFixture fixture = _TestFixture(); + try { + final SkiaGoldClient client = createClient( + fixture, + environment: presubmitEnv, + prefix: 'engine.', + onRun: (List command) { + if (command case ['git', ...]) { + return io.ProcessResult(0, 0, mockCommitHash, ''); + } + if (command case ['python tools/goldctl.py', 'imgtest', 'init', ...]) { + return io.ProcessResult(0, 0, '', ''); + } + expect(command, [ + 'python tools/goldctl.py', + 'imgtest', + 'add', + '--work-dir', + p.join(fixture.workDirectory.path, 'temp'), + '--test-name', + 'engine.test-name', + '--png-file', + p.join(fixture.workDirectory.path, 'temp', 'golden.png'), + '--add-test-optional-key', + 'image_matching_algorithm:fuzzy', + '--add-test-optional-key', + 'fuzzy_max_different_pixels:10', + '--add-test-optional-key', + 'fuzzy_pixel_delta_threshold:0', + ]); + return io.ProcessResult(0, 0, '', ''); + }, + ); + + await client.addImg( + 'test-name.foo', + io.File(p.join(fixture.workDirectory.path, 'temp', 'golden.png')), + screenshotSize: 1000, + ); + } finally { + fixture.dispose(); + } + }); + test('addImg [pre-submit] executes successfully with a release version', () async { // Adds a suffix of "_Release_3_21" to the test name. final _TestFixture fixture = _TestFixture( @@ -639,7 +686,7 @@ void main() { final String hash = client.getTraceID('test-name'); fixture.httpClient.setJsonResponse( - Uri.parse('https://flutter-engine-gold.skia.org/json/v2/latestpositivedigest/$hash'), + Uri.parse('https://flutter-gold.skia.org/json/v2/latestpositivedigest/$hash'), {'digest': 'digest'}, );