From 01d1e74af5f8f8c7998c2b5138995bd63976f7ee Mon Sep 17 00:00:00 2001 From: Kate Lovett Date: Thu, 6 Jun 2024 18:05:52 -0500 Subject: [PATCH] Remove abi key permanently from golden file testing (#149858) We have fiddled with this a bunch, and there is definitively no way to reliably include this, so I am removing it. Since CI does not cover all possible values, we can't consistently look up images for local testing. We thought removing it from the look up would work fine, and it did for most tests, but there were still some that could not find an image without it. A brief history on the abi key - added in https://github.com/flutter/flutter/pull/143621 - disabled in https://github.com/flutter/flutter/pull/148023 - added back in https://github.com/flutter/flutter/pull/148072 - we thought there was only an issue with alphabetizing keys - removed from local image look up in https://github.com/flutter/flutter/pull/149696 I updated the docs page to also discuss what makes a good key and what does not based on what we learned here. --- ...-a-golden-file-test-for-package-flutter.md | 20 ++++++++++++++ .../flutter_goldens/lib/flutter_goldens.dart | 13 --------- packages/flutter_goldens/lib/skia_client.dart | 11 -------- .../test/flutter_goldens_test.dart | 27 ------------------- 4 files changed, 20 insertions(+), 51 deletions(-) diff --git a/docs/contributing/testing/Writing-a-golden-file-test-for-package-flutter.md b/docs/contributing/testing/Writing-a-golden-file-test-for-package-flutter.md index ae881741da..48c583ce10 100644 --- a/docs/contributing/testing/Writing-a-golden-file-test-for-package-flutter.md +++ b/docs/contributing/testing/Writing-a-golden-file-test-for-package-flutter.md @@ -1,3 +1,5 @@ +# Writing a golden file test for package:flutter + _(This page is referenced by comments in the Flutter codebase.)_ **If you want to learn how to write a golden test for your package, see [the `matchesGoldenFile` API docs](https://api.flutter.dev/flutter/flutter_test/matchesGoldenFile.html).** This wiki page describes the special process specifically for the Flutter team itself. @@ -8,6 +10,7 @@ Golden file tests for `package:flutter` use [Flutter Gold](https://flutter-gold. - [Known Issues](#known-issues) - [Build Breakage](#build-breakage) - [Creating a New Golden File Test](#creating-a-new-golden-file-test) +- [Adding a new key in the Skia Client](#Adding-a-new-key-in-the-Skia-Client) - [Updating a Golden File Test](#updating-a-golden-file-test) - [Flutter Gold Login](#flutter-gold-login) - [`flutter-gold` Check](#flutter-gold-check) @@ -76,6 +79,23 @@ New tests can be triaged from these tryjobs, which will cause the pending `flutt And that’s it! Your new golden file(s) will be checked in as the baseline(s) for your new test(s), and your PR will be ready to merge. :tada: +## Adding a new key in the Skia Client + +Approved golden file images on the [Flutter Gold Dashboard] [Flutter Gold](https://flutter-gold.skia.org/?query=source_type%3Dflutter) +are keyed with parameters like platform, CI environment, test name, browser, and image extension. + +When adding new keys, consider all possible values, and whether or not they are covered by the CI environments that are used +to test changes in presubmit and postsubmit testing. If not all possible values are accounted for, false negatives can occur +in local testing. + +For example, we once included an abi key, which in our CI environments at the time could be linux_x64, windows_x64, or mac_x64. +These keys are used to look up approved images for local testing, so when a mac_arm64 machine would run local tests, +no image could be found and the tests would fail. +Omitting the key in the lookup did most often find the right image, but it was not consistently reliable, so we removed it. + +If the CI environments available for testing changes do not cover all value of a particular key, it is not a good key to +include as part of testing. + ## Updating a Golden File Test If renderings change, then the golden baseline in [Flutter Gold](https://flutter-gold.skia.org/?query=source_type%3Dflutter) will need to be updated. diff --git a/packages/flutter_goldens/lib/flutter_goldens.dart b/packages/flutter_goldens/lib/flutter_goldens.dart index 577a63dfbe..4a3d6638e1 100644 --- a/packages/flutter_goldens/lib/flutter_goldens.dart +++ b/packages/flutter_goldens/lib/flutter_goldens.dart @@ -3,7 +3,6 @@ // found in the LICENSE file. import 'dart:async' show FutureOr; -import 'dart:ffi' show Abi; import 'dart:io' as io show HttpClient, OSError, SocketException; import 'package:file/file.dart'; @@ -69,7 +68,6 @@ Future testExecutable(FutureOr Function() testMain, {String? namePre fs: fs, process: process, httpClient: httpClient, - abi: Abi.current(), ); } else if (FlutterPreSubmitFileComparator.isForEnvironment(platform)) { goldenFileComparator = await FlutterPreSubmitFileComparator.fromLocalFileComparator( @@ -80,7 +78,6 @@ Future testExecutable(FutureOr Function() testMain, {String? namePre fs: fs, process: process, httpClient: httpClient, - abi: Abi.current(), ); } else if (FlutterSkippingFileComparator.isForEnvironment(platform)) { goldenFileComparator = FlutterSkippingFileComparator.fromLocalFileComparator( @@ -94,7 +91,6 @@ Future testExecutable(FutureOr Function() testMain, {String? namePre fs: fs, process: process, httpClient: httpClient, - abi: Abi.current(), ); } else { goldenFileComparator = await FlutterLocalFileComparator.fromLocalFileComparator( @@ -104,7 +100,6 @@ Future testExecutable(FutureOr Function() testMain, {String? namePre fs: fs, process: process, httpClient: httpClient, - abi: Abi.current(), ); } await testMain(); @@ -298,7 +293,6 @@ class FlutterPostSubmitFileComparator extends FlutterGoldenFileComparator { required FileSystem fs, required ProcessManager process, required io.HttpClient httpClient, - required Abi abi, }) async { final Directory baseDirectory = FlutterGoldenFileComparator.getBaseDirectory( localFileComparator, @@ -315,7 +309,6 @@ class FlutterPostSubmitFileComparator extends FlutterGoldenFileComparator { fs: fs, process: process, httpClient: httpClient, - abi: abi, ); await goldens.auth(); return FlutterPostSubmitFileComparator( @@ -393,7 +386,6 @@ class FlutterPreSubmitFileComparator extends FlutterGoldenFileComparator { required FileSystem fs, required ProcessManager process, required io.HttpClient httpClient, - required Abi abi, }) async { final Directory baseDirectory = testBasedir ?? FlutterGoldenFileComparator.getBaseDirectory( localFileComparator, @@ -413,7 +405,6 @@ class FlutterPreSubmitFileComparator extends FlutterGoldenFileComparator { fs: fs, process: process, httpClient: httpClient, - abi: abi, ); await goldens.auth(); @@ -496,7 +487,6 @@ class FlutterSkippingFileComparator extends FlutterGoldenFileComparator { required FileSystem fs, required ProcessManager process, required io.HttpClient httpClient, - required Abi abi, }) { final Uri basedir = localFileComparator.basedir; final SkiaGoldClient skiaClient = SkiaGoldClient( @@ -506,7 +496,6 @@ class FlutterSkippingFileComparator extends FlutterGoldenFileComparator { fs: fs, process: process, httpClient: httpClient, - abi: abi, ); return FlutterSkippingFileComparator( basedir, @@ -595,7 +584,6 @@ class FlutterLocalFileComparator extends FlutterGoldenFileComparator with LocalC required FileSystem fs, required ProcessManager process, required io.HttpClient httpClient, - required Abi abi, }) async { baseDirectory ??= FlutterGoldenFileComparator.getBaseDirectory( localFileComparator, @@ -614,7 +602,6 @@ class FlutterLocalFileComparator extends FlutterGoldenFileComparator with LocalC fs: fs, process: process, httpClient: httpClient, - abi: abi, ); try { // Check if we can reach Gold. diff --git a/packages/flutter_goldens/lib/skia_client.dart b/packages/flutter_goldens/lib/skia_client.dart index dc816def60..1f3480c7dc 100644 --- a/packages/flutter_goldens/lib/skia_client.dart +++ b/packages/flutter_goldens/lib/skia_client.dart @@ -3,7 +3,6 @@ // found in the LICENSE file. import 'dart:convert'; -import 'dart:ffi' show Abi; import 'dart:io' as io; import 'package:crypto/crypto.dart'; @@ -52,7 +51,6 @@ class SkiaGoldClient { required this.fs, required this.process, required this.platform, - required this.abi, required this.httpClient, required this.log, }); @@ -77,9 +75,6 @@ class SkiaGoldClient { /// A client for making Http requests to the Flutter Gold dashboard. final io.HttpClient httpClient; - /// The ABI of the current host platform. - final Abi abi; - /// The local [Directory] within the [comparisonRoot] for the current test /// context. In this directory, the client will create image and JSON files /// for the goldctl tool to use. @@ -496,7 +491,6 @@ class SkiaGoldClient { final String? webRenderer = _webRendererValue; final Map keys = { 'Platform' : platform.operatingSystem, - 'Abi': '$abi', 'CI' : 'luci', if (_isImpeller) 'impeller': 'swiftshader', @@ -579,11 +573,6 @@ class SkiaGoldClient { final Map parameters = { if (_isBrowserTest) 'Browser' : _browserKey, - // This method is only used in local testing to look up the given image. - // CI is not comprehensive in possible abi keys, so false negatives can - // be produced if we include it in local testing. For example CI uses - // macos_x64 but a contributor could have macos_arm64. - // 'Abi': '$abi', 'CI' : 'luci', 'Platform' : platform.operatingSystem, if (webRenderer != null) diff --git a/packages/flutter_goldens/test/flutter_goldens_test.dart b/packages/flutter_goldens/test/flutter_goldens_test.dart index c08670db0f..4fba1c044e 100644 --- a/packages/flutter_goldens/test/flutter_goldens_test.dart +++ b/packages/flutter_goldens/test/flutter_goldens_test.dart @@ -5,7 +5,6 @@ // See also dev/automated_tests/flutter_test/flutter_gold_test.dart import 'dart:convert'; -import 'dart:ffi' show Abi; import 'dart:io' hide Directory; import 'package:file/file.dart'; @@ -31,11 +30,6 @@ const List _kTestPngBytes = [ 78, 68, 174, 66, 96, 130, ]; -// We intentionally use an ABI that is unlikely to be the one anyone editing this code -// would assume, so that if there any any assumptions made about the ABI, they will -// be more likely to fail the tests. -const Abi testAbi = Abi.fuchsiaRiscv64; - void main() { group('SkiaGoldClient', () { test('web HTML test', () async { @@ -60,7 +54,6 @@ void main() { process: process, platform: platform, httpClient: fakeHttpClient, - abi: testAbi, log: (String message) => fail('skia gold client printed unexpected output: "$message"'), ); @@ -111,7 +104,6 @@ void main() { process: process, platform: platform, httpClient: fakeHttpClient, - abi: testAbi, log: (String message) => fail('skia gold client printed unexpected output: "$message"'), ); @@ -154,7 +146,6 @@ void main() { process: process, platform: platform, httpClient: fakeHttpClient, - abi: testAbi, log: (String message) => fail('skia gold client printed unexpected output: "$message"'), ); final File authFile = fs.file('/workDirectory/temp/auth_opt.json') @@ -183,7 +174,6 @@ void main() { process: process, platform: platform, httpClient: fakeHttpClient, - abi: testAbi, log: (String message) => fail('skia gold client printed unexpected output: "$message"'), ); final File authFile = fs.file('/workDirectory/temp/auth_opt.json') @@ -216,7 +206,6 @@ void main() { process: process, platform: platform, httpClient: fakeHttpClient, - abi: testAbi, log: (String message) => fail('skia gold client printed unexpected output: "$message"'), ); @@ -248,7 +237,6 @@ void main() { process: process, platform: platform, httpClient: fakeHttpClient, - abi: testAbi, log: (String message) => fail('skia gold client printed unexpected output: "$message"'), ); @@ -299,7 +287,6 @@ void main() { process: process, platform: platform, httpClient: fakeHttpClient, - abi: testAbi, log: (String message) => fail('skia gold client printed unexpected output: "$message"'), ); @@ -358,7 +345,6 @@ void main() { process: process, platform: platform, httpClient: fakeHttpClient, - abi: testAbi, log: (String message) => fail('skia gold client printed unexpected output: "$message"'), ); @@ -421,7 +407,6 @@ void main() { process: process, platform: platform, httpClient: fakeHttpClient, - abi: testAbi, log: (String message) => fail('skia gold client printed unexpected output: "$message"'), ); const RunInvocation goldctlInvocation = RunInvocation( @@ -467,7 +452,6 @@ void main() { process: process, platform: platform, httpClient: fakeHttpClient, - abi: testAbi, log: (String message) => fail('skia gold client printed unexpected output: "$message"'), ); @@ -508,7 +492,6 @@ void main() { process: process, platform: platform, httpClient: fakeHttpClient, - abi: Abi.linuxX64, log: (String message) => fail('skia gold client printed unexpected output: "$message"'), ); @@ -542,7 +525,6 @@ void main() { process: process, platform: platform, httpClient: fakeHttpClient, - abi: Abi.linuxX64, log: (String message) => fail('skia gold client printed unexpected output: "$message"'), ); @@ -571,7 +553,6 @@ void main() { process: process, platform: platform, httpClient: fakeHttpClient, - abi: Abi.linuxX64, log: (String message) => fail('skia gold client printed unexpected output: "$message"'), ); expect( @@ -602,7 +583,6 @@ void main() { process: process, platform: platform, httpClient: fakeHttpClient, - abi: testAbi, log: (String message) => fail('skia gold client printed unexpected output: "$message"'), ); const RunInvocation goldctlInvocation = RunInvocation( @@ -652,7 +632,6 @@ void main() { process: process, platform: platform, httpClient: fakeHttpClient, - abi: testAbi, log: (String message) => fail('skia gold client printed unexpected output: "$message"'), ); const RunInvocation goldctlInvocation = RunInvocation( @@ -698,7 +677,6 @@ void main() { process: process, platform: platform, httpClient: fakeHttpClient, - abi: testAbi, log: (String message) => fail('skia gold client printed unexpected output: "$message"'), ); final Uri imageUrl = Uri.parse( @@ -878,7 +856,6 @@ void main() { fs: fs, process: FakeProcessManager(), httpClient: FakeHttpClient(), - abi: testAbi, ); expect(fakeSkiaClient.initCalls, 0); }); @@ -967,7 +944,6 @@ void main() { fs: fs, process: FakeProcessManager(), httpClient: FakeHttpClient(), - abi: testAbi, ); expect(fakeSkiaClient.tryInitCalls, 0); }); @@ -1073,7 +1049,6 @@ void main() { fs: fs, process: FakeProcessManager(), httpClient: FakeHttpClient(), - abi: testAbi, ); expect(comparator1.runtimeType, FlutterSkippingFileComparator); @@ -1087,7 +1062,6 @@ void main() { fs: fs, process: FakeProcessManager(), httpClient: FakeHttpClient(), - abi: testAbi, ); expect(comparator2.runtimeType, FlutterSkippingFileComparator); @@ -1101,7 +1075,6 @@ void main() { fs: fs, process: FakeProcessManager(), httpClient: FakeHttpClient(), - abi: testAbi, ); expect(comparator3.runtimeType, FlutterSkippingFileComparator);