From 91ea024cbc39e100f2079ac932b561b541c4ad16 Mon Sep 17 00:00:00 2001 From: Kate Lovett Date: Thu, 7 Dec 2023 13:34:05 -0600 Subject: [PATCH] Don't send images to Gold on release branches (#139706) Part of fixing https://github.com/flutter/flutter/issues/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 (https://github.com/flutter/flutter/issues/58814), but https://github.com/flutter/flutter/issues/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. --- .../flutter_goldens/lib/flutter_goldens.dart | 15 +- .../test/flutter_goldens_test.dart | 139 +++++++++++++++++- 2 files changed, 147 insertions(+), 7 deletions(-) diff --git a/packages/flutter_goldens/lib/flutter_goldens.dart b/packages/flutter_goldens/lib/flutter_goldens.dart index 580770c420cb8..eef5ed3939656 100644 --- a/packages/flutter_goldens/lib/flutter_goldens.dart +++ b/packages/flutter_goldens/lib/flutter_goldens.dart @@ -19,6 +19,7 @@ export 'package:flutter_goldens_client/skia_client.dart'; // https://github.com/flutter/flutter/wiki/Writing-a-golden-file-test-for-package%3Aflutter const String _kFlutterRootKey = 'FLUTTER_ROOT'; +final RegExp _kMainBranch = RegExp(r'master|main'); /// Main method that can be used in a `flutter_test_config.dart` file to set /// [goldenFileComparator] to an instance of [FlutterGoldenFileComparator] that @@ -259,7 +260,9 @@ class FlutterPostSubmitFileComparator extends FlutterGoldenFileComparator { final bool luciPostSubmit = platform.environment.containsKey('SWARMING_TASK_ID') && platform.environment.containsKey('GOLDCTL') // Luci tryjob environments contain this value to inform the [FlutterPreSubmitComparator]. - && !platform.environment.containsKey('GOLD_TRYJOB'); + && !platform.environment.containsKey('GOLD_TRYJOB') + // Only run on main branch. + && _kMainBranch.hasMatch(platform.environment['GIT_BRANCH'] ?? ''); return luciPostSubmit; } @@ -346,7 +349,9 @@ class FlutterPreSubmitFileComparator extends FlutterGoldenFileComparator { static bool isAvailableForEnvironment(Platform platform) { final bool luciPreSubmit = platform.environment.containsKey('SWARMING_TASK_ID') && platform.environment.containsKey('GOLDCTL') - && platform.environment.containsKey('GOLD_TRYJOB'); + && platform.environment.containsKey('GOLD_TRYJOB') + // Only run on the main branch + && _kMainBranch.hasMatch(platform.environment['GIT_BRANCH'] ?? ''); return luciPreSubmit; } } @@ -413,9 +418,11 @@ class FlutterSkippingFileComparator extends FlutterGoldenFileComparator { /// If we are in a CI environment, LUCI or Cirrus, but are not using the other /// comparators, we skip. static bool isAvailableForEnvironment(Platform platform) { - return platform.environment.containsKey('SWARMING_TASK_ID') + return (platform.environment.containsKey('SWARMING_TASK_ID') // Some builds are still being run on Cirrus, we should skip these. - || platform.environment.containsKey('CIRRUS_CI'); + || platform.environment.containsKey('CIRRUS_CI')) + // If we are in CI, skip on branches that are not main. + && !_kMainBranch.hasMatch(platform.environment['GIT_BRANCH'] ?? ''); } } diff --git a/packages/flutter_goldens/test/flutter_goldens_test.dart b/packages/flutter_goldens/test/flutter_goldens_test.dart index e64198f0bc486..3e2c05ea9cfc1 100644 --- a/packages/flutter_goldens/test/flutter_goldens_test.dart +++ b/packages/flutter_goldens/test/flutter_goldens_test.dart @@ -716,6 +716,55 @@ void main() { 'FLUTTER_ROOT': _kFlutterRoot, 'SWARMING_TASK_ID' : '12345678990', 'GOLDCTL' : 'goldctl', + 'GIT_BRANCH' : 'master', + }, + operatingSystem: 'macos', + ); + expect( + FlutterPostSubmitFileComparator.isAvailableForEnvironment(platform), + isTrue, + ); + }); + + test('returns false on release branches in postsubmit', () { + platform = FakePlatform( + environment: { + 'FLUTTER_ROOT': _kFlutterRoot, + 'SWARMING_TASK_ID' : 'sweet task ID', + 'GOLDCTL' : 'some/path', + 'GIT_BRANCH' : 'flutter-3.16-candidate.0', + }, + operatingSystem: 'macos', + ); + expect( + FlutterPostSubmitFileComparator.isAvailableForEnvironment(platform), + isFalse, + ); + }); + + test('returns true on master branch in postsubmit', () { + platform = FakePlatform( + environment: { + 'FLUTTER_ROOT': _kFlutterRoot, + 'SWARMING_TASK_ID' : 'sweet task ID', + 'GOLDCTL' : 'some/path', + 'GIT_BRANCH' : 'master', + }, + operatingSystem: 'macos', + ); + expect( + FlutterPostSubmitFileComparator.isAvailableForEnvironment(platform), + isTrue, + ); + }); + + test('returns true on main branch in postsubmit', () { + platform = FakePlatform( + environment: { + 'FLUTTER_ROOT': _kFlutterRoot, + 'SWARMING_TASK_ID' : 'sweet task ID', + 'GOLDCTL' : 'some/path', + 'GIT_BRANCH' : 'main', }, operatingSystem: 'macos', ); @@ -828,6 +877,57 @@ void main() { }); group('correctly determines testing environment', () { + test('returns false on release branches in presubmit', () { + platform = FakePlatform( + environment: { + 'FLUTTER_ROOT': _kFlutterRoot, + 'SWARMING_TASK_ID' : 'sweet task ID', + 'GOLDCTL' : 'some/path', + 'GOLD_TRYJOB' : 'true', + 'GIT_BRANCH' : 'flutter-3.16-candidate.0', + }, + operatingSystem: 'macos', + ); + expect( + FlutterPreSubmitFileComparator.isAvailableForEnvironment(platform), + isFalse, + ); + }); + + test('returns true on master branch in presubmit', () { + platform = FakePlatform( + environment: { + 'FLUTTER_ROOT': _kFlutterRoot, + 'SWARMING_TASK_ID' : 'sweet task ID', + 'GOLDCTL' : 'some/path', + 'GOLD_TRYJOB' : 'true', + 'GIT_BRANCH' : 'master', + }, + operatingSystem: 'macos', + ); + expect( + FlutterPreSubmitFileComparator.isAvailableForEnvironment(platform), + isTrue, + ); + }); + + test('returns true on main branch in presubmit', () { + platform = FakePlatform( + environment: { + 'FLUTTER_ROOT': _kFlutterRoot, + 'SWARMING_TASK_ID' : 'sweet task ID', + 'GOLDCTL' : 'some/path', + 'GOLD_TRYJOB' : 'true', + 'GIT_BRANCH' : 'main', + }, + operatingSystem: 'macos', + ); + expect( + FlutterPreSubmitFileComparator.isAvailableForEnvironment(platform), + isTrue, + ); + }); + test('returns true for Luci', () { platform = FakePlatform( environment: { @@ -835,6 +935,7 @@ void main() { 'SWARMING_TASK_ID' : '12345678990', 'GOLDCTL' : 'goldctl', 'GOLD_TRYJOB' : 'git/ref/12345/head', + 'GIT_BRANCH' : 'master', }, operatingSystem: 'macos', ); @@ -908,6 +1009,39 @@ void main() { group('Skipping', () { group('correctly determines testing environment', () { + test('returns true on release branches in presubmit', () { + platform = FakePlatform( + environment: { + 'FLUTTER_ROOT': _kFlutterRoot, + 'SWARMING_TASK_ID' : 'sweet task ID', + 'GOLDCTL' : 'some/path', + 'GOLD_TRYJOB' : 'true', + 'GIT_BRANCH' : 'flutter-3.16-candidate.0', + }, + operatingSystem: 'macos', + ); + expect( + FlutterSkippingFileComparator.isAvailableForEnvironment(platform), + isTrue, + ); + }); + + test('returns true on release branches in postsubmit', () { + platform = FakePlatform( + environment: { + 'FLUTTER_ROOT': _kFlutterRoot, + 'SWARMING_TASK_ID' : 'sweet task ID', + 'GOLDCTL' : 'some/path', + 'GIT_BRANCH' : 'flutter-3.16-candidate.0', + }, + operatingSystem: 'macos', + ); + expect( + FlutterSkippingFileComparator.isAvailableForEnvironment(platform), + isTrue, + ); + }); + test('returns true on Cirrus builds', () { platform = FakePlatform( environment: { @@ -936,7 +1070,7 @@ void main() { ); }); - test('returns false - no CI', () { + test('returns false - not in CI', () { platform = FakePlatform( environment: { 'FLUTTER_ROOT': _kFlutterRoot, @@ -944,8 +1078,7 @@ void main() { operatingSystem: 'macos', ); expect( - FlutterSkippingFileComparator.isAvailableForEnvironment( - platform), + FlutterSkippingFileComparator.isAvailableForEnvironment(platform), isFalse, ); });