Skip to content

Commit

Permalink
Don't send images to Gold on release branches (flutter#139706)
Browse files Browse the repository at this point in the history 
Part of fixing flutter#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 (flutter#58814), but flutter#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.
  • Loading branch information
@Piinks
Piinks authored Dec 7, 2023
1 parent 1fa54ea commit 91ea024
Show file tree
Hide file tree
Showing 2 changed files with 147 additions and 7 deletions.
15 changes: 11 additions & 4 deletions packages/flutter_goldens/lib/flutter_goldens.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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'] ?? '');
}
}

Expand Down
139 changes: 136 additions & 3 deletions packages/flutter_goldens/test/flutter_goldens_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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: <String, String>{
'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: <String, String>{
'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: <String, String>{
'FLUTTER_ROOT': _kFlutterRoot,
'SWARMING_TASK_ID' : 'sweet task ID',
'GOLDCTL' : 'some/path',
'GIT_BRANCH' : 'main',
},
operatingSystem: 'macos',
);
Expand Down Expand Up @@ -828,13 +877,65 @@ void main() {
});

group('correctly determines testing environment', () {
test('returns false on release branches in presubmit', () {
platform = FakePlatform(
environment: <String, String>{
'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: <String, String>{
'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: <String, String>{
'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: <String, String>{
'FLUTTER_ROOT': _kFlutterRoot,
'SWARMING_TASK_ID' : '12345678990',
'GOLDCTL' : 'goldctl',
'GOLD_TRYJOB' : 'git/ref/12345/head',
'GIT_BRANCH' : 'master',
},
operatingSystem: 'macos',
);
Expand Down Expand Up @@ -908,6 +1009,39 @@ void main() {

group('Skipping', () {
group('correctly determines testing environment', () {
test('returns true on release branches in presubmit', () {
platform = FakePlatform(
environment: <String, String>{
'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: <String, String>{
'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: <String, String>{
Expand Down Expand Up @@ -936,16 +1070,15 @@ void main() {
);
});

test('returns false - no CI', () {
test('returns false - not in CI', () {
platform = FakePlatform(
environment: <String, String>{
'FLUTTER_ROOT': _kFlutterRoot,
},
operatingSystem: 'macos',
);
expect(
FlutterSkippingFileComparator.isAvailableForEnvironment(
platform),
FlutterSkippingFileComparator.isAvailableForEnvironment(platform),
isFalse,
);
});
Expand Down

0 comments on commit 91ea024

Please sign in to comment.