From 4820e0cd9f0983cdcafd44ff08b25e702e548963 Mon Sep 17 00:00:00 2001 From: Kate Lovett Date: Fri, 22 Mar 2024 15:45:35 -0500 Subject: [PATCH] Fix skipping golden comparator for CI environments (#145619) Fixes https://github.com/flutter/flutter/issues/145618 The local file comparator was being used in post submit for the Linux coverage shard. This corrects it to choose the skipping comparator. --- .../flutter_goldens/lib/flutter_goldens.dart | 13 +++-- .../test/comparator_selection_test.dart | 56 +++++++++---------- 2 files changed, 35 insertions(+), 34 deletions(-) diff --git a/packages/flutter_goldens/lib/flutter_goldens.dart b/packages/flutter_goldens/lib/flutter_goldens.dart index 487c1c1fbc9..bec4d8a7c39 100644 --- a/packages/flutter_goldens/lib/flutter_goldens.dart +++ b/packages/flutter_goldens/lib/flutter_goldens.dart @@ -44,7 +44,9 @@ Future testExecutable(FutureOr Function() testMain, {String? namePre goldenFileComparator = await FlutterPreSubmitFileComparator.fromDefaultComparator(platform, namePrefix: namePrefix, log: print); } else if (FlutterSkippingFileComparator.isForEnvironment(platform)) { goldenFileComparator = FlutterSkippingFileComparator.fromDefaultComparator( - 'Golden file testing is not executed on Cirrus, or LUCI environments outside of flutter/flutter.', + '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.', namePrefix: namePrefix, log: print ); @@ -432,13 +434,12 @@ class FlutterSkippingFileComparator extends FlutterGoldenFileComparator { /// used. /// /// If we are in a CI environment, LUCI or Cirrus, but are not using the other - /// comparators, we skip. + /// 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') + return platform.environment.containsKey('SWARMING_TASK_ID') // Some builds are still being run on Cirrus, we should skip these. - || platform.environment.containsKey('CIRRUS_CI')) - // If we are in CI, skip on branches that are not main. - && !_isMainBranch(platform.environment['GIT_BRANCH']); + || platform.environment.containsKey('CIRRUS_CI'); } } diff --git a/packages/flutter_goldens/test/comparator_selection_test.dart b/packages/flutter_goldens/test/comparator_selection_test.dart index 5a09c46994b..dcacb21da90 100644 --- a/packages/flutter_goldens/test/comparator_selection_test.dart +++ b/packages/flutter_goldens/test/comparator_selection_test.dart @@ -60,18 +60,18 @@ void main() { expect(_testRecommendations(hasFlutterRoot: true, hasGold: true), _Comparator.local); // If we don't have gold but are on CI, we skip regardless. - expect(_testRecommendations(hasLuci: true), _Comparator.local); // TODO(ianh): this should be skip - expect(_testRecommendations(hasLuci: true, hasTryJob: true), _Comparator.local); // TODO(ianh): this should be skip - expect(_testRecommendations(hasCirrus: true), _Comparator.local); // TODO(ianh): this should be skip - expect(_testRecommendations(hasCirrus: true, hasTryJob: true), _Comparator.local); // TODO(ianh): this should be skip - expect(_testRecommendations(hasLuci: true, hasCirrus: true), _Comparator.local); // TODO(ianh): this should be skip - expect(_testRecommendations(hasLuci: true, hasCirrus: true, hasTryJob: true), _Comparator.local); // TODO(ianh): this should be skip - expect(_testRecommendations(hasFlutterRoot: true, hasLuci: true), _Comparator.local); // TODO(ianh): this should be skip - expect(_testRecommendations(hasFlutterRoot: true, hasLuci: true, hasTryJob: true), _Comparator.local); // TODO(ianh): this should be skip - expect(_testRecommendations(hasFlutterRoot: true, hasCirrus: true), _Comparator.local); // TODO(ianh): this should be skip - expect(_testRecommendations(hasFlutterRoot: true, hasCirrus: true, hasTryJob: true), _Comparator.local); // TODO(ianh): this should be skip - expect(_testRecommendations(hasFlutterRoot: true, hasLuci: true, hasCirrus: true), _Comparator.local); // TODO(ianh): this should be skip - expect(_testRecommendations(hasFlutterRoot: true, hasLuci: true, hasCirrus: true, hasTryJob: true), _Comparator.local); // TODO(ianh): this should be skip + 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. expect(_testRecommendations(hasGold: true, hasLuci: true), _Comparator.post); @@ -86,8 +86,8 @@ void main() { 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.local); // TODO(ianh): this should be skip - expect(_testRecommendations(hasCirrus: true, hasGold: true, hasFlutterRoot: true, hasTryJob: true), _Comparator.local); // TODO(ianh): this should be skip + 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', () { @@ -136,18 +136,18 @@ void main() { expect(_testRecommendations(os: 'linux', hasFlutterRoot: true, hasGold: true), _Comparator.local); // If we don't have gold but are on CI, we skip regardless. - expect(_testRecommendations(os: 'linux', hasLuci: true), _Comparator.local); // TODO(ianh): this should be skip - expect(_testRecommendations(os: 'linux', hasLuci: true, hasTryJob: true), _Comparator.local); // TODO(ianh): this should be skip - expect(_testRecommendations(os: 'linux', hasCirrus: true), _Comparator.local); // TODO(ianh): this should be skip - expect(_testRecommendations(os: 'linux', hasCirrus: true, hasTryJob: true), _Comparator.local); // TODO(ianh): this should be skip - expect(_testRecommendations(os: 'linux', hasLuci: true, hasCirrus: true), _Comparator.local); // TODO(ianh): this should be skip - expect(_testRecommendations(os: 'linux', hasLuci: true, hasCirrus: true, hasTryJob: true), _Comparator.local); // TODO(ianh): this should be skip - expect(_testRecommendations(os: 'linux', hasFlutterRoot: true, hasLuci: true), _Comparator.local); // TODO(ianh): this should be skip - expect(_testRecommendations(os: 'linux', hasFlutterRoot: true, hasLuci: true, hasTryJob: true), _Comparator.local); // TODO(ianh): this should be skip - expect(_testRecommendations(os: 'linux', hasFlutterRoot: true, hasCirrus: true), _Comparator.local); // TODO(ianh): this should be skip - expect(_testRecommendations(os: 'linux', hasFlutterRoot: true, hasCirrus: true, hasTryJob: true), _Comparator.local); // TODO(ianh): this should be skip - expect(_testRecommendations(os: 'linux', hasFlutterRoot: true, hasLuci: true, hasCirrus: true), _Comparator.local); // TODO(ianh): this should be skip - expect(_testRecommendations(os: 'linux', hasFlutterRoot: true, hasLuci: true, hasCirrus: true, hasTryJob: true), _Comparator.local); // TODO(ianh): this should be skip + 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); + expect(_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); @@ -162,8 +162,8 @@ void main() { 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.local); // TODO(ianh): this should be skip - expect(_testRecommendations(os: 'linux', hasCirrus: true, hasGold: true, hasFlutterRoot: true, hasTryJob: true), _Comparator.local); // TODO(ianh): this should be skip + 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', () {