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);