From 59f630b7b57ca5fa887335eb8388a9dc7061dbdb Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Thu, 9 Jan 2025 14:27:24 -0800 Subject: [PATCH] Remove Cirrus CI from Flutter goldens. (#161396) Towards https://github.com/flutter/flutter/issues/161387. I believe this effectively a NOP as the `CIRRUS_`-environment variables are never set. --- .../flutter_goldens/lib/flutter_goldens.dart | 14 +- .../test/comparator_selection_test.dart | 259 +----------------- 2 files changed, 10 insertions(+), 263 deletions(-) diff --git a/packages/flutter_goldens/lib/flutter_goldens.dart b/packages/flutter_goldens/lib/flutter_goldens.dart index 94b0199e4f..831ef65dac 100644 --- a/packages/flutter_goldens/lib/flutter_goldens.dart +++ b/packages/flutter_goldens/lib/flutter_goldens.dart @@ -89,9 +89,8 @@ Future testExecutable(FutureOr Function() testMain, {String? namePre } else if (FlutterSkippingFileComparator.isForEnvironment(platform)) { goldenFileComparator = FlutterSkippingFileComparator.fromLocalFileComparator( localFileComparator: goldenFileComparator as LocalFileComparator, - 'Golden file testing is not executed on Cirrus, or LUCI environments ' - 'outside of flutter/flutter, or in test shards that are not configured ' - 'for using goldctl.', + 'Golden file testing is not executed on LUCI environments outside of ' + 'flutter, or in test shards that are not configured for using goldctl.', platform: platform, namePrefix: namePrefix, log: print, @@ -448,7 +447,7 @@ class FlutterPreSubmitFileComparator extends FlutterGoldenFileComparator { /// A [FlutterGoldenFileComparator] for testing conditions that do not execute /// golden file tests. /// -/// Currently, this comparator is used on Cirrus, or in Luci environments when executing tests +/// Currently, this comparator is used on Luci environments when executing tests /// outside of the flutter/flutter repository. /// /// See also: @@ -521,14 +520,11 @@ class FlutterSkippingFileComparator extends FlutterGoldenFileComparator { /// Decides, based on the current environment, if this comparator should be /// used. /// - /// If we are in a CI environment, LUCI or Cirrus, but are not using the other + /// If we are in a CI environment, i.e. LUCI, but are not using the other /// comparators, we skip. Otherwise we would fallback to the local comparator, /// for which failures cannot be resolved in a CI environment. static bool isForEnvironment(Platform platform) { - return platform.environment.containsKey('SWARMING_TASK_ID') - // Some builds are still being run on Cirrus, we should skip these. - || - platform.environment.containsKey('CIRRUS_CI'); + return platform.environment.containsKey('SWARMING_TASK_ID'); } } diff --git a/packages/flutter_goldens/test/comparator_selection_test.dart b/packages/flutter_goldens/test/comparator_selection_test.dart index 3c56b3c080..d1c98f58d4 100644 --- a/packages/flutter_goldens/test/comparator_selection_test.dart +++ b/packages/flutter_goldens/test/comparator_selection_test.dart @@ -11,7 +11,6 @@ enum _Comparator { post, pre, skip, local } _Comparator _testRecommendations({ bool hasFlutterRoot = false, bool hasLuci = false, - bool hasCirrus = false, bool hasGold = false, bool hasTryJob = false, String branch = 'main', @@ -21,11 +20,7 @@ _Comparator _testRecommendations({ environment: { if (hasFlutterRoot) 'FLUTTER_ROOT': '/flutter', if (hasLuci) 'SWARMING_TASK_ID': '8675309', - if (hasCirrus) 'CIRRUS_CI': 'true', - if (hasCirrus) 'CIRRUS_PR': '', - if (hasCirrus) 'CIRRUS_BRANCH': branch, if (hasGold) 'GOLDCTL': 'goldctl', - if (hasGold && hasCirrus) 'GOLD_SERVICE_ACCOUNT': 'service account...', if (hasTryJob) 'GOLD_TRYJOB': 'git/ref/12345/head', 'GIT_BRANCH': branch, }, @@ -54,71 +49,25 @@ void main() { // If we don't have gold but are on CI, we skip regardless. expect(_testRecommendations(hasLuci: true), _Comparator.skip); expect(_testRecommendations(hasLuci: true, hasTryJob: true), _Comparator.skip); - expect(_testRecommendations(hasCirrus: true), _Comparator.skip); - expect(_testRecommendations(hasCirrus: true, hasTryJob: true), _Comparator.skip); - expect(_testRecommendations(hasLuci: true, hasCirrus: true), _Comparator.skip); - expect(_testRecommendations(hasLuci: true, hasCirrus: true, hasTryJob: true), _Comparator.skip); expect(_testRecommendations(hasFlutterRoot: true, hasLuci: true), _Comparator.skip); expect( _testRecommendations(hasFlutterRoot: true, hasLuci: true, hasTryJob: true), _Comparator.skip, ); - expect(_testRecommendations(hasFlutterRoot: true, hasCirrus: true), _Comparator.skip); - expect( - _testRecommendations(hasFlutterRoot: true, hasCirrus: true, hasTryJob: true), - _Comparator.skip, - ); - expect( - _testRecommendations(hasFlutterRoot: true, hasLuci: true, hasCirrus: true), - _Comparator.skip, - ); - expect( - _testRecommendations(hasFlutterRoot: true, hasLuci: true, hasCirrus: true, hasTryJob: true), - _Comparator.skip, - ); - // On Luci, with Gold, post-submit. Flutter root and Cirrus variables should have no effect. + // On Luci, with Gold, post-submit. Flutter root and LUCI variables should have no effect. expect(_testRecommendations(hasGold: true, hasLuci: true), _Comparator.post); - expect(_testRecommendations(hasGold: true, hasLuci: true, hasCirrus: true), _Comparator.post); expect( _testRecommendations(hasGold: true, hasLuci: true, hasFlutterRoot: true), _Comparator.post, ); - expect( - _testRecommendations(hasGold: true, hasLuci: true, hasFlutterRoot: true, hasCirrus: true), - _Comparator.post, - ); - // On Luci, with Gold, pre-submit. Flutter root and Cirrus variables should have no effect. + // On Luci, with Gold, pre-submit. Flutter root and LUCI variables should have no effect. expect(_testRecommendations(hasGold: true, hasLuci: true, hasTryJob: true), _Comparator.pre); - expect( - _testRecommendations(hasGold: true, hasLuci: true, hasCirrus: true, hasTryJob: true), - _Comparator.pre, - ); expect( _testRecommendations(hasGold: true, hasLuci: true, hasFlutterRoot: true, hasTryJob: true), _Comparator.pre, ); - expect( - _testRecommendations( - hasGold: true, - hasLuci: true, - hasFlutterRoot: true, - hasCirrus: true, - hasTryJob: true, - ), - _Comparator.pre, - ); - - // On Cirrus (with Gold and not on Luci), we skip regardless. - expect( - _testRecommendations(hasCirrus: true, hasGold: true, hasFlutterRoot: true), - _Comparator.skip, - ); - expect( - _testRecommendations(hasCirrus: true, hasGold: true, hasFlutterRoot: true, hasTryJob: true), - _Comparator.skip, - ); }); test('Comparator recommendations - release branch', () { @@ -146,27 +95,6 @@ void main() { _testRecommendations(branch: 'flutter-3.16-candidate.0', hasLuci: true, hasTryJob: true), _Comparator.skip, ); - expect( - _testRecommendations(branch: 'flutter-3.16-candidate.0', hasCirrus: true), - _Comparator.skip, - ); - expect( - _testRecommendations(branch: 'flutter-3.16-candidate.0', hasCirrus: true, hasTryJob: true), - _Comparator.skip, - ); - expect( - _testRecommendations(branch: 'flutter-3.16-candidate.0', hasLuci: true, hasCirrus: true), - _Comparator.skip, - ); - expect( - _testRecommendations( - branch: 'flutter-3.16-candidate.0', - hasLuci: true, - hasCirrus: true, - hasTryJob: true, - ), - _Comparator.skip, - ); expect( _testRecommendations(branch: 'flutter-3.16-candidate.0', hasFlutterRoot: true, hasLuci: true), _Comparator.skip, @@ -180,57 +108,12 @@ void main() { ), _Comparator.skip, ); - expect( - _testRecommendations( - branch: 'flutter-3.16-candidate.0', - hasFlutterRoot: true, - hasCirrus: true, - ), - _Comparator.skip, - ); - expect( - _testRecommendations( - branch: 'flutter-3.16-candidate.0', - hasFlutterRoot: true, - hasCirrus: true, - hasTryJob: true, - ), - _Comparator.skip, - ); - expect( - _testRecommendations( - branch: 'flutter-3.16-candidate.0', - hasFlutterRoot: true, - hasLuci: true, - hasCirrus: true, - ), - _Comparator.skip, - ); - expect( - _testRecommendations( - branch: 'flutter-3.16-candidate.0', - hasFlutterRoot: true, - hasLuci: true, - hasCirrus: true, - hasTryJob: true, - ), - _Comparator.skip, - ); - // On Luci, with Gold, post-submit. Flutter root and Cirrus variables should have no effect. Branch should make us skip. + // On Luci, with Gold, post-submit. Flutter root and LUCI variables should have no effect. Branch should make us skip. expect( _testRecommendations(branch: 'flutter-3.16-candidate.0', hasGold: true, hasLuci: true), _Comparator.skip, ); - expect( - _testRecommendations( - branch: 'flutter-3.16-candidate.0', - hasGold: true, - hasLuci: true, - hasCirrus: true, - ), - _Comparator.skip, - ); expect( _testRecommendations( branch: 'flutter-3.16-candidate.0', @@ -240,18 +123,8 @@ void main() { ), _Comparator.skip, ); - expect( - _testRecommendations( - branch: 'flutter-3.16-candidate.0', - hasGold: true, - hasLuci: true, - hasFlutterRoot: true, - hasCirrus: true, - ), - _Comparator.skip, - ); - // On Luci, with Gold, pre-submit. Flutter root and Cirrus variables should have no effect. Branch should make us skip. + // On Luci, with Gold, pre-submit. Flutter root and LUCI variables should have no effect. Branch should make us skip. expect( _testRecommendations( branch: 'flutter-3.16-candidate.0', @@ -266,48 +139,6 @@ void main() { branch: 'flutter-3.16-candidate.0', hasGold: true, hasLuci: true, - hasCirrus: true, - hasTryJob: true, - ), - _Comparator.skip, - ); - expect( - _testRecommendations( - branch: 'flutter-3.16-candidate.0', - hasGold: true, - hasLuci: true, - hasFlutterRoot: true, - hasTryJob: true, - ), - _Comparator.skip, - ); - expect( - _testRecommendations( - branch: 'flutter-3.16-candidate.0', - hasGold: true, - hasLuci: true, - hasFlutterRoot: true, - hasCirrus: true, - hasTryJob: true, - ), - _Comparator.skip, - ); - - // On Cirrus (with Gold and not on Luci), we skip regardless. - expect( - _testRecommendations( - branch: 'flutter-3.16-candidate.0', - hasCirrus: true, - hasGold: true, - hasFlutterRoot: true, - ), - _Comparator.skip, - ); - expect( - _testRecommendations( - branch: 'flutter-3.16-candidate.0', - hasCirrus: true, - hasGold: true, hasFlutterRoot: true, hasTryJob: true, ), @@ -328,13 +159,6 @@ void main() { // If we don't have gold but are on CI, we skip regardless. expect(_testRecommendations(os: 'linux', hasLuci: true), _Comparator.skip); expect(_testRecommendations(os: 'linux', hasLuci: true, hasTryJob: true), _Comparator.skip); - expect(_testRecommendations(os: 'linux', hasCirrus: true), _Comparator.skip); - expect(_testRecommendations(os: 'linux', hasCirrus: true, hasTryJob: true), _Comparator.skip); - expect(_testRecommendations(os: 'linux', hasLuci: true, hasCirrus: true), _Comparator.skip); - expect( - _testRecommendations(os: 'linux', hasLuci: true, hasCirrus: true, hasTryJob: true), - _Comparator.skip, - ); expect( _testRecommendations(os: 'linux', hasFlutterRoot: true, hasLuci: true), _Comparator.skip, @@ -343,65 +167,19 @@ void main() { _testRecommendations(os: 'linux', hasFlutterRoot: true, hasLuci: true, hasTryJob: true), _Comparator.skip, ); - expect( - _testRecommendations(os: 'linux', hasFlutterRoot: true, hasCirrus: true), - _Comparator.skip, - ); - expect( - _testRecommendations(os: 'linux', hasFlutterRoot: true, hasCirrus: true, hasTryJob: true), - _Comparator.skip, - ); - expect( - _testRecommendations(os: 'linux', hasFlutterRoot: true, hasLuci: true, hasCirrus: true), - _Comparator.skip, - ); - expect( - _testRecommendations( - os: 'linux', - hasFlutterRoot: true, - hasLuci: true, - hasCirrus: true, - hasTryJob: true, - ), - _Comparator.skip, - ); // On Luci, with Gold, post-submit. Flutter root and Cirrus variables should have no effect. expect(_testRecommendations(os: 'linux', hasGold: true, hasLuci: true), _Comparator.post); - expect( - _testRecommendations(os: 'linux', hasGold: true, hasLuci: true, hasCirrus: true), - _Comparator.post, - ); expect( _testRecommendations(os: 'linux', hasGold: true, hasLuci: true, hasFlutterRoot: true), _Comparator.post, ); - expect( - _testRecommendations( - os: 'linux', - hasGold: true, - hasLuci: true, - hasFlutterRoot: true, - hasCirrus: true, - ), - _Comparator.post, - ); - // On Luci, with Gold, pre-submit. Flutter root and Cirrus variables should have no effect. + // On Luci, with Gold, pre-submit. Flutter root should have no effect. expect( _testRecommendations(os: 'linux', hasGold: true, hasLuci: true, hasTryJob: true), _Comparator.pre, ); - expect( - _testRecommendations( - os: 'linux', - hasGold: true, - hasLuci: true, - hasCirrus: true, - hasTryJob: true, - ), - _Comparator.pre, - ); expect( _testRecommendations( os: 'linux', @@ -412,33 +190,6 @@ void main() { ), _Comparator.pre, ); - expect( - _testRecommendations( - os: 'linux', - hasGold: true, - hasLuci: true, - hasFlutterRoot: true, - hasCirrus: true, - hasTryJob: true, - ), - _Comparator.pre, - ); - - // On Cirrus (with Gold and not on Luci), we skip regardless. - expect( - _testRecommendations(os: 'linux', hasCirrus: true, hasGold: true, hasFlutterRoot: true), - _Comparator.skip, - ); - expect( - _testRecommendations( - os: 'linux', - hasCirrus: true, - hasGold: true, - hasFlutterRoot: true, - hasTryJob: true, - ), - _Comparator.skip, - ); }); test('Branch names', () {