firka/flutter
3
0
Fork 1
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Remove abi key permanently from golden file testing (#149858)

Browse Source 
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.
This commit is contained in:
Kate Lovett
2024-06-06 18:05:52 -05:00
committed by GitHub
parent 87f68a8876
commit 01d1e74af5
4 changed files with 20 additions and 51 deletions
Download Patch File Download Diff File Expand all files Collapse all files

20
docs/contributing/testing/Writing-a-golden-file-test-for-package-flutter.md
View File

@@ -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.

13
packages/flutter_goldens/lib/flutter_goldens.dart
View File

@@ -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<void> testExecutable(FutureOr<void> 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<void> testExecutable(FutureOr<void> 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<void> testExecutable(FutureOr<void> Function() testMain, {String? namePre
fs: fs,
process: process,
httpClient: httpClient,
abi: Abi.current(),
);
} else {
goldenFileComparator = await FlutterLocalFileComparator.fromLocalFileComparator(
@@ -104,7 +100,6 @@ Future<void> testExecutable(FutureOr<void> 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.

11
packages/flutter_goldens/lib/skia_client.dart
View File

@@ -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<String, dynamic> keys = <String, dynamic>{
'Platform' : platform.operatingSystem,
'Abi': '$abi',
'CI' : 'luci',
if (_isImpeller)
'impeller': 'swiftshader',
@@ -579,11 +573,6 @@ class SkiaGoldClient {
final Map<String, Object?> parameters = <String, Object?>{
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)

27
packages/flutter_goldens/test/flutter_goldens_test.dart
View File

@@ -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<int> _kTestPngBytes = <int>[
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);