Skip to content

Commit 01d1e74

Browse files
PiinksPiinks
authored
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 flutter/flutter#143621 - disabled in flutter/flutter#148023 - added back in flutter/flutter#148072 - we thought there was only an issue with alphabetizing keys - removed from local image look up in flutter/flutter#149696 I updated the docs page to also discuss what makes a good key and what does not based on what we learned here.
1 parent 87f68a8 commit 01d1e74

File tree

4 files changed

+20
-51
lines changed

4 files changed

+20
-51
lines changed

‎docs/contributing/testing/Writing-a-golden-file-test-for-package-flutter.md‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
# Writing a golden file test for package:flutter
2+
13
_(This page is referenced by comments in the Flutter codebase.)_
24

35
**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.
810
- [Known Issues](#known-issues)
911
- [Build Breakage](#build-breakage)
1012
- [Creating a New Golden File Test](#creating-a-new-golden-file-test)
13+
- [Adding a new key in the Skia Client](#Adding-a-new-key-in-the-Skia-Client)
1114
- [Updating a Golden File Test](#updating-a-golden-file-test)
1215
- [Flutter Gold Login](#flutter-gold-login)
1316
- [`flutter-gold` Check](#flutter-gold-check)
@@ -76,6 +79,23 @@ New tests can be triaged from these tryjobs, which will cause the pending `flutt
7679

7780
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:
7881

82+
## Adding a new key in the Skia Client
83+
84+
Approved golden file images on the [Flutter Gold Dashboard] [Flutter Gold](https://flutter-gold.skia.org/?query=source_type%3Dflutter)
85+
are keyed with parameters like platform, CI environment, test name, browser, and image extension.
86+
87+
When adding new keys, consider all possible values, and whether or not they are covered by the CI environments that are used
88+
to test changes in presubmit and postsubmit testing. If not all possible values are accounted for, false negatives can occur
89+
in local testing.
90+
91+
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.
92+
These keys are used to look up approved images for local testing, so when a mac_arm64 machine would run local tests,
93+
no image could be found and the tests would fail.
94+
Omitting the key in the lookup did most often find the right image, but it was not consistently reliable, so we removed it.
95+
96+
If the CI environments available for testing changes do not cover all value of a particular key, it is not a good key to
97+
include as part of testing.
98+
7999
## Updating a Golden File Test
80100

81101
If renderings change, then the golden baseline in [Flutter Gold](https://flutter-gold.skia.org/?query=source_type%3Dflutter) will need to be updated.

‎packages/flutter_goldens/lib/flutter_goldens.dart‎

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33
// found in the LICENSE file.
44

55
import 'dart:async' show FutureOr;
6-
import 'dart:ffi' show Abi;
76
import 'dart:io' as io show HttpClient, OSError, SocketException;
87

98
import 'package:file/file.dart';
@@ -69,7 +68,6 @@ Future<void> testExecutable(FutureOr<void> Function() testMain, {String? namePre
6968
fs: fs,
7069
process: process,
7170
httpClient: httpClient,
72-
abi: Abi.current(),
7371
);
7472
} else if (FlutterPreSubmitFileComparator.isForEnvironment(platform)) {
7573
goldenFileComparator = await FlutterPreSubmitFileComparator.fromLocalFileComparator(
@@ -80,7 +78,6 @@ Future<void> testExecutable(FutureOr<void> Function() testMain, {String? namePre
8078
fs: fs,
8179
process: process,
8280
httpClient: httpClient,
83-
abi: Abi.current(),
8481
);
8582
} else if (FlutterSkippingFileComparator.isForEnvironment(platform)) {
8683
goldenFileComparator = FlutterSkippingFileComparator.fromLocalFileComparator(
@@ -94,7 +91,6 @@ Future<void> testExecutable(FutureOr<void> Function() testMain, {String? namePre
9491
fs: fs,
9592
process: process,
9693
httpClient: httpClient,
97-
abi: Abi.current(),
9894
);
9995
} else {
10096
goldenFileComparator = await FlutterLocalFileComparator.fromLocalFileComparator(
@@ -104,7 +100,6 @@ Future<void> testExecutable(FutureOr<void> Function() testMain, {String? namePre
104100
fs: fs,
105101
process: process,
106102
httpClient: httpClient,
107-
abi: Abi.current(),
108103
);
109104
}
110105
await testMain();
@@ -298,7 +293,6 @@ class FlutterPostSubmitFileComparator extends FlutterGoldenFileComparator {
298293
required FileSystem fs,
299294
required ProcessManager process,
300295
required io.HttpClient httpClient,
301-
required Abi abi,
302296
}) async {
303297
final Directory baseDirectory = FlutterGoldenFileComparator.getBaseDirectory(
304298
localFileComparator,
@@ -315,7 +309,6 @@ class FlutterPostSubmitFileComparator extends FlutterGoldenFileComparator {
315309
fs: fs,
316310
process: process,
317311
httpClient: httpClient,
318-
abi: abi,
319312
);
320313
await goldens.auth();
321314
return FlutterPostSubmitFileComparator(
@@ -393,7 +386,6 @@ class FlutterPreSubmitFileComparator extends FlutterGoldenFileComparator {
393386
required FileSystem fs,
394387
required ProcessManager process,
395388
required io.HttpClient httpClient,
396-
required Abi abi,
397389
}) async {
398390
final Directory baseDirectory = testBasedir ?? FlutterGoldenFileComparator.getBaseDirectory(
399391
localFileComparator,
@@ -413,7 +405,6 @@ class FlutterPreSubmitFileComparator extends FlutterGoldenFileComparator {
413405
fs: fs,
414406
process: process,
415407
httpClient: httpClient,
416-
abi: abi,
417408
);
418409

419410
await goldens.auth();
@@ -496,7 +487,6 @@ class FlutterSkippingFileComparator extends FlutterGoldenFileComparator {
496487
required FileSystem fs,
497488
required ProcessManager process,
498489
required io.HttpClient httpClient,
499-
required Abi abi,
500490
}) {
501491
final Uri basedir = localFileComparator.basedir;
502492
final SkiaGoldClient skiaClient = SkiaGoldClient(
@@ -506,7 +496,6 @@ class FlutterSkippingFileComparator extends FlutterGoldenFileComparator {
506496
fs: fs,
507497
process: process,
508498
httpClient: httpClient,
509-
abi: abi,
510499
);
511500
return FlutterSkippingFileComparator(
512501
basedir,
@@ -595,7 +584,6 @@ class FlutterLocalFileComparator extends FlutterGoldenFileComparator with LocalC
595584
required FileSystem fs,
596585
required ProcessManager process,
597586
required io.HttpClient httpClient,
598-
required Abi abi,
599587
}) async {
600588
baseDirectory ??= FlutterGoldenFileComparator.getBaseDirectory(
601589
localFileComparator,
@@ -614,7 +602,6 @@ class FlutterLocalFileComparator extends FlutterGoldenFileComparator with LocalC
614602
fs: fs,
615603
process: process,
616604
httpClient: httpClient,
617-
abi: abi,
618605
);
619606
try {
620607
// Check if we can reach Gold.

‎packages/flutter_goldens/lib/skia_client.dart‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33
// found in the LICENSE file.
44

55
import 'dart:convert';
6-
import 'dart:ffi' show Abi;
76
import 'dart:io' as io;
87

98
import 'package:crypto/crypto.dart';
@@ -52,7 +51,6 @@ class SkiaGoldClient {
5251
required this.fs,
5352
required this.process,
5453
required this.platform,
55-
required this.abi,
5654
required this.httpClient,
5755
required this.log,
5856
});
@@ -77,9 +75,6 @@ class SkiaGoldClient {
7775
/// A client for making Http requests to the Flutter Gold dashboard.
7876
final io.HttpClient httpClient;
7977

80-
/// The ABI of the current host platform.
81-
final Abi abi;
82-
8378
/// The local [Directory] within the [comparisonRoot] for the current test
8479
/// context. In this directory, the client will create image and JSON files
8580
/// for the goldctl tool to use.
@@ -496,7 +491,6 @@ class SkiaGoldClient {
496491
final String? webRenderer = _webRendererValue;
497492
final Map<String, dynamic> keys = <String, dynamic>{
498493
'Platform' : platform.operatingSystem,
499-
'Abi': '$abi',
500494
'CI' : 'luci',
501495
if (_isImpeller)
502496
'impeller': 'swiftshader',
@@ -579,11 +573,6 @@ class SkiaGoldClient {
579573
final Map<String, Object?> parameters = <String, Object?>{
580574
if (_isBrowserTest)
581575
'Browser' : _browserKey,
582-
// This method is only used in local testing to look up the given image.
583-
// CI is not comprehensive in possible abi keys, so false negatives can
584-
// be produced if we include it in local testing. For example CI uses
585-
// macos_x64 but a contributor could have macos_arm64.
586-
// 'Abi': '$abi',
587576
'CI' : 'luci',
588577
'Platform' : platform.operatingSystem,
589578
if (webRenderer != null)

‎packages/flutter_goldens/test/flutter_goldens_test.dart‎

Lines changed: 0 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55
// See also dev/automated_tests/flutter_test/flutter_gold_test.dart
66

77
import 'dart:convert';
8-
import 'dart:ffi' show Abi;
98
import 'dart:io' hide Directory;
109

1110
import 'package:file/file.dart';
@@ -31,11 +30,6 @@ const List<int> _kTestPngBytes = <int>[
3130
78, 68, 174, 66, 96, 130,
3231
];
3332

34-
// We intentionally use an ABI that is unlikely to be the one anyone editing this code
35-
// would assume, so that if there any any assumptions made about the ABI, they will
36-
// be more likely to fail the tests.
37-
const Abi testAbi = Abi.fuchsiaRiscv64;
38-
3933
void main() {
4034
group('SkiaGoldClient', () {
4135
test('web HTML test', () async {
@@ -60,7 +54,6 @@ void main() {
6054
process: process,
6155
platform: platform,
6256
httpClient: fakeHttpClient,
63-
abi: testAbi,
6457
log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
6558
);
6659

@@ -111,7 +104,6 @@ void main() {
111104
process: process,
112105
platform: platform,
113106
httpClient: fakeHttpClient,
114-
abi: testAbi,
115107
log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
116108
);
117109

@@ -154,7 +146,6 @@ void main() {
154146
process: process,
155147
platform: platform,
156148
httpClient: fakeHttpClient,
157-
abi: testAbi,
158149
log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
159150
);
160151
final File authFile = fs.file('/workDirectory/temp/auth_opt.json')
@@ -183,7 +174,6 @@ void main() {
183174
process: process,
184175
platform: platform,
185176
httpClient: fakeHttpClient,
186-
abi: testAbi,
187177
log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
188178
);
189179
final File authFile = fs.file('/workDirectory/temp/auth_opt.json')
@@ -216,7 +206,6 @@ void main() {
216206
process: process,
217207
platform: platform,
218208
httpClient: fakeHttpClient,
219-
abi: testAbi,
220209
log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
221210
);
222211

@@ -248,7 +237,6 @@ void main() {
248237
process: process,
249238
platform: platform,
250239
httpClient: fakeHttpClient,
251-
abi: testAbi,
252240
log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
253241
);
254242

@@ -299,7 +287,6 @@ void main() {
299287
process: process,
300288
platform: platform,
301289
httpClient: fakeHttpClient,
302-
abi: testAbi,
303290
log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
304291
);
305292

@@ -358,7 +345,6 @@ void main() {
358345
process: process,
359346
platform: platform,
360347
httpClient: fakeHttpClient,
361-
abi: testAbi,
362348
log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
363349
);
364350

@@ -421,7 +407,6 @@ void main() {
421407
process: process,
422408
platform: platform,
423409
httpClient: fakeHttpClient,
424-
abi: testAbi,
425410
log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
426411
);
427412
const RunInvocation goldctlInvocation = RunInvocation(
@@ -467,7 +452,6 @@ void main() {
467452
process: process,
468453
platform: platform,
469454
httpClient: fakeHttpClient,
470-
abi: testAbi,
471455
log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
472456
);
473457

@@ -508,7 +492,6 @@ void main() {
508492
process: process,
509493
platform: platform,
510494
httpClient: fakeHttpClient,
511-
abi: Abi.linuxX64,
512495
log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
513496
);
514497

@@ -542,7 +525,6 @@ void main() {
542525
process: process,
543526
platform: platform,
544527
httpClient: fakeHttpClient,
545-
abi: Abi.linuxX64,
546528
log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
547529
);
548530

@@ -571,7 +553,6 @@ void main() {
571553
process: process,
572554
platform: platform,
573555
httpClient: fakeHttpClient,
574-
abi: Abi.linuxX64,
575556
log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
576557
);
577558
expect(
@@ -602,7 +583,6 @@ void main() {
602583
process: process,
603584
platform: platform,
604585
httpClient: fakeHttpClient,
605-
abi: testAbi,
606586
log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
607587
);
608588
const RunInvocation goldctlInvocation = RunInvocation(
@@ -652,7 +632,6 @@ void main() {
652632
process: process,
653633
platform: platform,
654634
httpClient: fakeHttpClient,
655-
abi: testAbi,
656635
log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
657636
);
658637
const RunInvocation goldctlInvocation = RunInvocation(
@@ -698,7 +677,6 @@ void main() {
698677
process: process,
699678
platform: platform,
700679
httpClient: fakeHttpClient,
701-
abi: testAbi,
702680
log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
703681
);
704682
final Uri imageUrl = Uri.parse(
@@ -878,7 +856,6 @@ void main() {
878856
fs: fs,
879857
process: FakeProcessManager(),
880858
httpClient: FakeHttpClient(),
881-
abi: testAbi,
882859
);
883860
expect(fakeSkiaClient.initCalls, 0);
884861
});
@@ -967,7 +944,6 @@ void main() {
967944
fs: fs,
968945
process: FakeProcessManager(),
969946
httpClient: FakeHttpClient(),
970-
abi: testAbi,
971947
);
972948
expect(fakeSkiaClient.tryInitCalls, 0);
973949
});
@@ -1073,7 +1049,6 @@ void main() {
10731049
fs: fs,
10741050
process: FakeProcessManager(),
10751051
httpClient: FakeHttpClient(),
1076-
abi: testAbi,
10771052
);
10781053
expect(comparator1.runtimeType, FlutterSkippingFileComparator);
10791054

@@ -1087,7 +1062,6 @@ void main() {
10871062
fs: fs,
10881063
process: FakeProcessManager(),
10891064
httpClient: FakeHttpClient(),
1090-
abi: testAbi,
10911065
);
10921066
expect(comparator2.runtimeType, FlutterSkippingFileComparator);
10931067

@@ -1101,7 +1075,6 @@ void main() {
11011075
fs: fs,
11021076
process: FakeProcessManager(),
11031077
httpClient: FakeHttpClient(),
1104-
abi: testAbi,
11051078
);
11061079
expect(comparator3.runtimeType, FlutterSkippingFileComparator);
11071080

0 commit comments

Comments
 (0)