Skip to content

Commit 91ea024

Browse files
PiinksPiinks
authored
Don't send images to Gold on release branches (#139706)
Part of fixing #139673, it will need to be cherry picked into the stable branch to fully resolve. Originally I tried to come at this from `ci.yaml`, but the syntax does not exist to either conditionally include a dependency based on the branch we are on, or to disable a shard for a given branch (as opposed to enabling it which is supported). I could double every CI shard that uses Gold to try to serve my purpose, but it is already a very large and cumbersome file to keep up to date. Doubling it does not feel like the best solution. Using a RegEx is not my favorite, but I am using the same one we use in our CI, and left a note there to update it if it should ever change. Since there is already a whole infra built around it, I feel it is pretty safe so we can fix the stable tree. We already had mitigated Gold affecting release branches in the past through flutter/cocoon (#58814), but #139673 exposed a rather rare edge case. A change was CP'd into the stable branch that introduced golden file image changes. Typically this would not cause an issue since any change that has landed on the master branch has golden files accounted for. In this case, the CP'd change on master has generated a different image on canvaskit due to another change that was not on stable. So when the CP landed, it generated a new image Gold had never seen before. Gold only tracks the master branch, so we cannot approve the image, and so cannot fix the stable tree. This would disable the failing check on release branches and fix the tree.
1 parent 1fa54ea commit 91ea024

File tree

2 files changed

+147
-7
lines changed

2 files changed

+147
-7
lines changed

‎packages/flutter_goldens/lib/flutter_goldens.dart

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ export 'package:flutter_goldens_client/skia_client.dart';
1919
// https://github.com/flutter/flutter/wiki/Writing-a-golden-file-test-for-package%3Aflutter
2020

2121
const String _kFlutterRootKey = 'FLUTTER_ROOT';
22+
final RegExp _kMainBranch = RegExp(r'master|main');
2223

2324
/// Main method that can be used in a `flutter_test_config.dart` file to set
2425
/// [goldenFileComparator] to an instance of [FlutterGoldenFileComparator] that
@@ -259,7 +260,9 @@ class FlutterPostSubmitFileComparator extends FlutterGoldenFileComparator {
259260
final bool luciPostSubmit = platform.environment.containsKey('SWARMING_TASK_ID')
260261
&& platform.environment.containsKey('GOLDCTL')
261262
// Luci tryjob environments contain this value to inform the [FlutterPreSubmitComparator].
262-
&& !platform.environment.containsKey('GOLD_TRYJOB');
263+
&& !platform.environment.containsKey('GOLD_TRYJOB')
264+
// Only run on main branch.
265+
&& _kMainBranch.hasMatch(platform.environment['GIT_BRANCH'] ?? '');
263266

264267
return luciPostSubmit;
265268
}
@@ -346,7 +349,9 @@ class FlutterPreSubmitFileComparator extends FlutterGoldenFileComparator {
346349
static bool isAvailableForEnvironment(Platform platform) {
347350
final bool luciPreSubmit = platform.environment.containsKey('SWARMING_TASK_ID')
348351
&& platform.environment.containsKey('GOLDCTL')
349-
&& platform.environment.containsKey('GOLD_TRYJOB');
352+
&& platform.environment.containsKey('GOLD_TRYJOB')
353+
// Only run on the main branch
354+
&& _kMainBranch.hasMatch(platform.environment['GIT_BRANCH'] ?? '');
350355
return luciPreSubmit;
351356
}
352357
}
@@ -413,9 +418,11 @@ class FlutterSkippingFileComparator extends FlutterGoldenFileComparator {
413418
/// If we are in a CI environment, LUCI or Cirrus, but are not using the other
414419
/// comparators, we skip.
415420
static bool isAvailableForEnvironment(Platform platform) {
416-
return platform.environment.containsKey('SWARMING_TASK_ID')
421+
return (platform.environment.containsKey('SWARMING_TASK_ID')
417422
// Some builds are still being run on Cirrus, we should skip these.
418-
|| platform.environment.containsKey('CIRRUS_CI');
423+
|| platform.environment.containsKey('CIRRUS_CI'))
424+
// If we are in CI, skip on branches that are not main.
425+
&& !_kMainBranch.hasMatch(platform.environment['GIT_BRANCH'] ?? '');
419426
}
420427
}
421428

‎packages/flutter_goldens/test/flutter_goldens_test.dart

Lines changed: 136 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -716,6 +716,55 @@ void main() {
716716
'FLUTTER_ROOT': _kFlutterRoot,
717717
'SWARMING_TASK_ID' : '12345678990',
718718
'GOLDCTL' : 'goldctl',
719+
'GIT_BRANCH' : 'master',
720+
},
721+
operatingSystem: 'macos',
722+
);
723+
expect(
724+
FlutterPostSubmitFileComparator.isAvailableForEnvironment(platform),
725+
isTrue,
726+
);
727+
});
728+
729+
test('returns false on release branches in postsubmit', () {
730+
platform = FakePlatform(
731+
environment: <String, String>{
732+
'FLUTTER_ROOT': _kFlutterRoot,
733+
'SWARMING_TASK_ID' : 'sweet task ID',
734+
'GOLDCTL' : 'some/path',
735+
'GIT_BRANCH' : 'flutter-3.16-candidate.0',
736+
},
737+
operatingSystem: 'macos',
738+
);
739+
expect(
740+
FlutterPostSubmitFileComparator.isAvailableForEnvironment(platform),
741+
isFalse,
742+
);
743+
});
744+
745+
test('returns true on master branch in postsubmit', () {
746+
platform = FakePlatform(
747+
environment: <String, String>{
748+
'FLUTTER_ROOT': _kFlutterRoot,
749+
'SWARMING_TASK_ID' : 'sweet task ID',
750+
'GOLDCTL' : 'some/path',
751+
'GIT_BRANCH' : 'master',
752+
},
753+
operatingSystem: 'macos',
754+
);
755+
expect(
756+
FlutterPostSubmitFileComparator.isAvailableForEnvironment(platform),
757+
isTrue,
758+
);
759+
});
760+
761+
test('returns true on main branch in postsubmit', () {
762+
platform = FakePlatform(
763+
environment: <String, String>{
764+
'FLUTTER_ROOT': _kFlutterRoot,
765+
'SWARMING_TASK_ID' : 'sweet task ID',
766+
'GOLDCTL' : 'some/path',
767+
'GIT_BRANCH' : 'main',
719768
},
720769
operatingSystem: 'macos',
721770
);
@@ -828,13 +877,65 @@ void main() {
828877
});
829878

830879
group('correctly determines testing environment', () {
880+
test('returns false on release branches in presubmit', () {
881+
platform = FakePlatform(
882+
environment: <String, String>{
883+
'FLUTTER_ROOT': _kFlutterRoot,
884+
'SWARMING_TASK_ID' : 'sweet task ID',
885+
'GOLDCTL' : 'some/path',
886+
'GOLD_TRYJOB' : 'true',
887+
'GIT_BRANCH' : 'flutter-3.16-candidate.0',
888+
},
889+
operatingSystem: 'macos',
890+
);
891+
expect(
892+
FlutterPreSubmitFileComparator.isAvailableForEnvironment(platform),
893+
isFalse,
894+
);
895+
});
896+
897+
test('returns true on master branch in presubmit', () {
898+
platform = FakePlatform(
899+
environment: <String, String>{
900+
'FLUTTER_ROOT': _kFlutterRoot,
901+
'SWARMING_TASK_ID' : 'sweet task ID',
902+
'GOLDCTL' : 'some/path',
903+
'GOLD_TRYJOB' : 'true',
904+
'GIT_BRANCH' : 'master',
905+
},
906+
operatingSystem: 'macos',
907+
);
908+
expect(
909+
FlutterPreSubmitFileComparator.isAvailableForEnvironment(platform),
910+
isTrue,
911+
);
912+
});
913+
914+
test('returns true on main branch in presubmit', () {
915+
platform = FakePlatform(
916+
environment: <String, String>{
917+
'FLUTTER_ROOT': _kFlutterRoot,
918+
'SWARMING_TASK_ID' : 'sweet task ID',
919+
'GOLDCTL' : 'some/path',
920+
'GOLD_TRYJOB' : 'true',
921+
'GIT_BRANCH' : 'main',
922+
},
923+
operatingSystem: 'macos',
924+
);
925+
expect(
926+
FlutterPreSubmitFileComparator.isAvailableForEnvironment(platform),
927+
isTrue,
928+
);
929+
});
930+
831931
test('returns true for Luci', () {
832932
platform = FakePlatform(
833933
environment: <String, String>{
834934
'FLUTTER_ROOT': _kFlutterRoot,
835935
'SWARMING_TASK_ID' : '12345678990',
836936
'GOLDCTL' : 'goldctl',
837937
'GOLD_TRYJOB' : 'git/ref/12345/head',
938+
'GIT_BRANCH' : 'master',
838939
},
839940
operatingSystem: 'macos',
840941
);
@@ -908,6 +1009,39 @@ void main() {
9081009

9091010
group('Skipping', () {
9101011
group('correctly determines testing environment', () {
1012+
test('returns true on release branches in presubmit', () {
1013+
platform = FakePlatform(
1014+
environment: <String, String>{
1015+
'FLUTTER_ROOT': _kFlutterRoot,
1016+
'SWARMING_TASK_ID' : 'sweet task ID',
1017+
'GOLDCTL' : 'some/path',
1018+
'GOLD_TRYJOB' : 'true',
1019+
'GIT_BRANCH' : 'flutter-3.16-candidate.0',
1020+
},
1021+
operatingSystem: 'macos',
1022+
);
1023+
expect(
1024+
FlutterSkippingFileComparator.isAvailableForEnvironment(platform),
1025+
isTrue,
1026+
);
1027+
});
1028+
1029+
test('returns true on release branches in postsubmit', () {
1030+
platform = FakePlatform(
1031+
environment: <String, String>{
1032+
'FLUTTER_ROOT': _kFlutterRoot,
1033+
'SWARMING_TASK_ID' : 'sweet task ID',
1034+
'GOLDCTL' : 'some/path',
1035+
'GIT_BRANCH' : 'flutter-3.16-candidate.0',
1036+
},
1037+
operatingSystem: 'macos',
1038+
);
1039+
expect(
1040+
FlutterSkippingFileComparator.isAvailableForEnvironment(platform),
1041+
isTrue,
1042+
);
1043+
});
1044+
9111045
test('returns true on Cirrus builds', () {
9121046
platform = FakePlatform(
9131047
environment: <String, String>{
@@ -936,16 +1070,15 @@ void main() {
9361070
);
9371071
});
9381072

939-
test('returns false - no CI', () {
1073+
test('returns false - not in CI', () {
9401074
platform = FakePlatform(
9411075
environment: <String, String>{
9421076
'FLUTTER_ROOT': _kFlutterRoot,
9431077
},
9441078
operatingSystem: 'macos',
9451079
);
9461080
expect(
947-
FlutterSkippingFileComparator.isAvailableForEnvironment(
948-
platform),
1081+
FlutterSkippingFileComparator.isAvailableForEnvironment(platform),
9491082
isFalse,
9501083
);
9511084
});

0 commit comments

Comments
 (0)