diff --git a/packages/flutter/lib/src/cupertino/action_sheet.dart b/packages/flutter/lib/src/cupertino/action_sheet.dart index 93c4a29b477..ce19f755d3c 100644 --- a/packages/flutter/lib/src/cupertino/action_sheet.dart +++ b/packages/flutter/lib/src/cupertino/action_sheet.dart @@ -85,8 +85,7 @@ const double _kDividerThickness = 1.0; /// sheet title and message text style. /// /// To display action buttons that look like standard iOS action sheet buttons, -/// provide [CupertinoActionSheetAction]s for the [actions] given to this action -/// sheet. +/// provide [CupertinoActionSheetAction]s for the [actions] given to this action sheet. /// /// To include a iOS-style cancel button separate from the other buttons, /// provide an [CupertinoActionSheetAction] for the [cancelButton] given to this diff --git a/packages/flutter_goldens/lib/flutter_goldens.dart b/packages/flutter_goldens/lib/flutter_goldens.dart index d88a11d4b7f..10d6ba146aa 100644 --- a/packages/flutter_goldens/lib/flutter_goldens.dart +++ b/packages/flutter_goldens/lib/flutter_goldens.dart @@ -28,13 +28,13 @@ const String _kFlutterRootKey = 'FLUTTER_ROOT'; /// instantiated is based on the current testing environment. Future main(FutureOr testMain()) async { const Platform platform = LocalPlatform(); - if (FlutterPostSubmitFileComparator.isAvailableForEnvironment(platform)) { - goldenFileComparator = await FlutterPostSubmitFileComparator.fromDefaultComparator(platform); + if (FlutterSkiaGoldFileComparator.isAvailableForEnvironment(platform)) { + goldenFileComparator = await FlutterSkiaGoldFileComparator.fromDefaultComparator(platform); } else if (FlutterPreSubmitFileComparator.isAvailableForEnvironment(platform)) { goldenFileComparator = await FlutterPreSubmitFileComparator.fromDefaultComparator(platform); - } else if (FlutterSkippingFileComparator.isAvailableForEnvironment(platform)) { - goldenFileComparator = FlutterSkippingFileComparator.fromDefaultComparator( - 'Golden file testing is not executed on some Cirrus shards.' + } else if (FlutterSkippingGoldenFileComparator.isAvailableForEnvironment(platform)) { + goldenFileComparator = FlutterSkippingGoldenFileComparator.fromDefaultComparator( + 'Golden file testing is unavailable on LUCI and some Cirrus shards.' ); } else { goldenFileComparator = await FlutterLocalFileComparator.fromDefaultComparator(platform); @@ -50,7 +50,7 @@ Future main(FutureOr testMain()) async { /// different [FlutterGoldenFileComparator]s, depending on the current testing /// environment. /// -/// * The [FlutterPostSubmitFileComparator] is utilized during post-submit +/// * The [FlutterSkiaGoldFileComparator] is utilized during post-submit /// testing, after a pull request has landed on the master branch. This /// comparator uses the [SkiaGoldClient] and the `goldctl` tool to upload /// tests to the [Flutter Gold dashboard](https://flutter-gold.skia.org). @@ -58,34 +58,28 @@ Future main(FutureOr testMain()) async { /// repository. /// /// * The [FlutterPreSubmitFileComparator] is utilized in pre-submit testing, -/// before a pull request lands on the master branch. When authorized, this -/// comparator uses the [SkiaGoldClient] to execute tryjobs, allowing -/// contributors to view and check in visual differences before landing the -/// change. +/// before a pull request can land on the master branch. This comparator +/// uses the [SkiaGoldClient] to request the baseline images kept by the +/// [Flutter Gold dashboard](https://flutter-gold.skia.org). It then +/// compares the current test image to the baseline images using the +/// standard [GoldenFileComparator.compareLists] to detect any pixel +/// difference. The [SkiaGoldClient] is also used here to check the active +/// ignores from the dashboard, in order to allow intended changes to pass +/// tests. /// -/// * When unable to authenticate the `goldctl` tool, this comparator -/// uses the [SkiaGoldClient] to request the baseline images kept by the -/// [Flutter Gold dashboard](https://flutter-gold.skia.org). It then -/// compares the current test image to the baseline images using the -/// standard [GoldenFileComparator.compareLists] to detect any pixel -/// difference. The [SkiaGoldClient] is also used in this case to check -/// the active ignores from the dashboard, in order to allow intended -/// changes to pass tests. -/// -/// * The [FlutterLocalFileComparator] is used for local development testing. -/// Similar to the unauthorized implementation of the +/// * The [FlutterLocalFileComparator] is used for any other tests run outside +/// of the above conditions. Similar to the /// [FlutterPreSubmitFileComparator], this comparator will use the /// [SkiaGoldClient] to request baseline images from -/// [Flutter Gold](https://flutter-gold.skia.org) and manually compare -/// pixels. If a difference is detected, this comparator will +/// [Flutter Gold](https://flutter-gold.skia.org) and compares for the +/// current test image. If a difference is detected, this comparator will /// generate failure output illustrating the found difference. If a baseline /// is not found for a given test image, it will consider it a new test and /// output the new image for verification. -/// -/// The [FlutterSkippingFileComparator] is utilized to skip tests outside -/// of the appropriate environments described above. Currently, some Cirrus -/// test shards do not execute golden file testing, and as such do not require -/// a comparator. This comparator is also used when an internet connection is +/// The [FlutterSkippingGoldenFileComparator] is utilized to skip tests outside +/// of the appropriate environments. Currently, tests executing in post-submit +/// on the LUCI build environment are skipped, as post-submit checks are done +/// on Cirrus. This comparator is also used when an internet connection is /// unavailable. abstract class FlutterGoldenFileComparator extends GoldenFileComparator { /// Creates a [FlutterGoldenFileComparator] that will resolve golden file @@ -137,7 +131,7 @@ abstract class FlutterGoldenFileComparator extends GoldenFileComparator { /// Calculate the appropriate basedir for the current test context. /// /// The optional [suffix] argument is used by the - /// [FlutterPostSubmitFileComparator] and the [FlutterPreSubmitFileComparator]. + /// [FlutterSkiaGoldFileComparator] and the [FlutterPreSubmitFileComparator]. /// These [FlutterGoldenFileComparators] randomize their base directories to /// maintain thread safety while using the `goldctl` tool. @protected @@ -154,7 +148,7 @@ abstract class FlutterGoldenFileComparator extends GoldenFileComparator { if (!local) { comparisonRoot = fs.systemTempDirectory.childDirectory( - 'skia_goldens_$suffix' + 'skia_goldens$suffix' ); } else { comparisonRoot = flutterRoot.childDirectory( @@ -190,11 +184,12 @@ abstract class FlutterGoldenFileComparator extends GoldenFileComparator { } } -/// A [FlutterGoldenFileComparator] for testing golden images with Skia Gold in -/// post-submit. +/// A [FlutterGoldenFileComparator] for testing golden images with Skia Gold. /// /// For testing across all platforms, the [SkiaGoldClient] is used to upload -/// images for framework-related golden tests and process results. +/// images for framework-related golden tests and process results. Currently +/// these tests are designed to be run post-submit on Cirrus CI, informed by the +/// environment. /// /// See also: /// @@ -206,13 +201,13 @@ abstract class FlutterGoldenFileComparator extends GoldenFileComparator { /// * [FlutterLocalFileComparator], another /// [FlutterGoldenFileComparator] that tests golden images locally on your /// current machine. -class FlutterPostSubmitFileComparator extends FlutterGoldenFileComparator { - /// Creates a [FlutterPostSubmitFileComparator] that will test golden file +class FlutterSkiaGoldFileComparator extends FlutterGoldenFileComparator { + /// Creates a [FlutterSkiaGoldFileComparator] that will test golden file /// images against Skia Gold. /// /// The [fs] and [platform] parameters are useful in tests, where the default /// file system and platform can be replaced by mock instances. - FlutterPostSubmitFileComparator( + FlutterSkiaGoldFileComparator( final Uri basedir, final SkiaGoldClient skiaClient, { final FileSystem fs = const LocalFileSystem(), @@ -224,12 +219,12 @@ class FlutterPostSubmitFileComparator extends FlutterGoldenFileComparator { platform: platform, ); - /// Creates a new [FlutterPostSubmitFileComparator] that mirrors the relative + /// Creates a new [FlutterSkiaGoldFileComparator] that mirrors the relative /// path resolution of the default [goldenFileComparator]. /// /// The [goldens] and [defaultComparator] parameters are visible for testing /// purposes only. - static Future fromDefaultComparator( + static Future fromDefaultComparator( final Platform platform, { SkiaGoldClient goldens, LocalFileComparator defaultComparator, @@ -243,15 +238,10 @@ class FlutterPostSubmitFileComparator extends FlutterGoldenFileComparator { ); baseDirectory.createSync(recursive: true); - goldens ??= SkiaGoldClient( - baseDirectory, - ci: platform.environment.containsKey('CIRRUS_CI') - ? ContinuousIntegrationEnvironment.cirrus - : ContinuousIntegrationEnvironment.luci, - ); + goldens ??= SkiaGoldClient(baseDirectory); await goldens.auth(); await goldens.imgtestInit(); - return FlutterPostSubmitFileComparator(baseDirectory.uri, goldens); + return FlutterSkiaGoldFileComparator(baseDirectory.uri, goldens); } @override @@ -263,40 +253,32 @@ class FlutterPostSubmitFileComparator extends FlutterGoldenFileComparator { return skiaClient.imgtestAdd(golden.path, goldenFile); } - /// Decides based on the current environment if goldens tests should be - /// executed through Skia Gold. + /// Decides based on the current environment whether goldens tests should be + /// performed against Skia Gold. static bool isAvailableForEnvironment(Platform platform) { final String cirrusPR = platform.environment['CIRRUS_PR'] ?? ''; final String cirrusBranch = platform.environment['CIRRUS_BRANCH'] ?? ''; - final bool cirrusPostSubmit = platform.environment.containsKey('CIRRUS_CI') + return platform.environment.containsKey('CIRRUS_CI') && cirrusPR.isEmpty && cirrusBranch == 'master' && platform.environment.containsKey('GOLD_SERVICE_ACCOUNT'); - - final bool luciPostSubmit = platform.environment.containsKey('SWARMING_TASK_ID') - // Luci tryjob environments contain this value to inform the [FlutterPreSubmitComparator]. - && !platform.environment.containsKey('GOLD_TRYJOB'); - - return cirrusPostSubmit || luciPostSubmit; } } /// A [FlutterGoldenFileComparator] for testing golden images before changes are /// merged into the master branch. /// -/// When authorized (on luci and most cirrus testing conditions), the comparator -/// executes tryjobs using the [SkiaGoldClient]. -/// -/// When unauthorized, this comparator utilizes the [SkiaGoldClient] to request -/// baseline images for the given device under test for manual comparison. +/// This comparator utilizes the [SkiaGoldClient] to request baseline images for +/// the given device under test for comparison. This comparator is only +/// initialized during pre-submit testing on Cirrus CI. /// /// See also: /// /// * [GoldenFileComparator], the abstract class that /// [FlutterGoldenFileComparator] implements. -/// * [FlutterPostSubmitFileComparator], another +/// * [FlutterSkiaGoldFileComparator], another /// [FlutterGoldenFileComparator] that uploads tests to the Skia Gold -/// dashboard in post-submit. +/// dashboard. /// * [FlutterLocalFileComparator], another /// [FlutterGoldenFileComparator] that tests golden images locally on your /// current machine. @@ -340,22 +322,10 @@ class FlutterPreSubmitFileComparator extends FlutterGoldenFileComparator { if (!baseDirectory.existsSync()) baseDirectory.createSync(recursive: true); - goldens ??= SkiaGoldClient( - baseDirectory, - ci: platform.environment.containsKey('CIRRUS_CI') - ? ContinuousIntegrationEnvironment.cirrus - : ContinuousIntegrationEnvironment.luci, - ); + goldens ??= SkiaGoldClient(baseDirectory); - bool onCirrusWithPermission = false; - if (platform.environment.containsKey('GOLD_SERVICE_ACCOUNT')) { - // Some contributors may not have permission on Cirrus to decrypt the - // service account. - onCirrusWithPermission = - !platform.environment['GOLD_SERVICE_ACCOUNT'].startsWith('ENCRYPTED'); - } - final bool onLuci = platform.environment.containsKey('SWARMING_TASK_ID'); - if (onCirrusWithPermission || onLuci) { + final bool hasWritePermission = !platform.environment['GOLD_SERVICE_ACCOUNT'].startsWith('ENCRYPTED'); + if (hasWritePermission) { await goldens.auth(); await goldens.tryjobInit(); return _AuthorizedFlutterPreSubmitComparator( @@ -386,17 +356,13 @@ class FlutterPreSubmitFileComparator extends FlutterGoldenFileComparator { return false; } - /// Decides based on the current environment if goldens tests should be - /// executed as pre-submit tests with Skia Gold. + /// Decides based on the current environment whether goldens tests should be + /// performed as pre-submit tests with Skia Gold. static bool isAvailableForEnvironment(Platform platform) { final String cirrusPR = platform.environment['CIRRUS_PR'] ?? ''; - final bool cirrusPreSubmit = platform.environment.containsKey('CIRRUS_CI') + return platform.environment.containsKey('CIRRUS_CI') && cirrusPR.isNotEmpty && platform.environment.containsKey('GOLD_SERVICE_ACCOUNT'); - - final bool luciPreSubmit = platform.environment.containsKey('SWARMING_TASK_ID') - && platform.environment.containsKey('GOLD_TRYJOB'); - return cirrusPreSubmit || luciPreSubmit; } } @@ -476,15 +442,19 @@ class _UnauthorizedFlutterPreSubmitComparator extends FlutterPreSubmitFileCompar } } -/// A [FlutterGoldenFileComparator] for testing conditions that do not execute -/// golden file tests. +/// A [FlutterGoldenFileComparator] for controlling post-submit testing +/// conditions that do not execute golden file tests. /// -/// Currently, this comparator is used in some Cirrus test shards, and when an -/// internet connection is not available for contacting Gold. +/// Currently, this comparator is used in post-submit checks on LUCI and with +/// some Cirrus shards that do not run framework tests. This comparator is also +/// used when an internet connection is not available for contacting Gold. /// /// See also: /// -/// * [FlutterPostSubmitFileComparator], another [FlutterGoldenFileComparator] +/// * [FlutterGoldensRepositoryFileComparator], another +/// [FlutterGoldenFileComparator] that tests golden images using the +/// flutter/goldens repository. +/// * [FlutterSkiaGoldFileComparator], another [FlutterGoldenFileComparator] /// that tests golden images through Skia Gold. /// * [FlutterPreSubmitFileComparator], another /// [FlutterGoldenFileComparator] that tests golden images before changes are @@ -492,24 +462,24 @@ class _UnauthorizedFlutterPreSubmitComparator extends FlutterPreSubmitFileCompar /// * [FlutterLocalFileComparator], another /// [FlutterGoldenFileComparator] that tests golden images locally on your /// current machine. -class FlutterSkippingFileComparator extends FlutterGoldenFileComparator { - /// Creates a [FlutterSkippingFileComparator] that will skip tests that +class FlutterSkippingGoldenFileComparator extends FlutterGoldenFileComparator { + /// Creates a [FlutterSkippingGoldenFileComparator] that will skip tests that /// are not in the right environment for golden file testing. - FlutterSkippingFileComparator( + FlutterSkippingGoldenFileComparator( final Uri basedir, final SkiaGoldClient skiaClient, this.reason, ) : assert(reason != null), super(basedir, skiaClient); - /// Describes the reason for using the [FlutterSkippingFileComparator]. + /// Describes the reason for using the [FlutterSkippingGoldenFileComparator]. /// /// Cannot be null. final String reason; - /// Creates a new [FlutterSkippingFileComparator] that mirrors the + /// Creates a new [FlutterSkippingGoldenFileComparator] that mirrors the /// relative path resolution of the default [goldenFileComparator]. - static FlutterSkippingFileComparator fromDefaultComparator( + static FlutterSkippingGoldenFileComparator fromDefaultComparator( String reason, { LocalFileComparator defaultComparator, }) { @@ -517,7 +487,7 @@ class FlutterSkippingFileComparator extends FlutterGoldenFileComparator { const FileSystem fs = LocalFileSystem(); final Uri basedir = defaultComparator.basedir; final SkiaGoldClient skiaClient = SkiaGoldClient(fs.directory(basedir)); - return FlutterSkippingFileComparator(basedir, skiaClient, reason); + return FlutterSkippingGoldenFileComparator(basedir, skiaClient, reason); } @override @@ -531,11 +501,8 @@ class FlutterSkippingFileComparator extends FlutterGoldenFileComparator { @override Future update(Uri golden, Uint8List imageBytes) => null; - /// Decides, based on the current environment, if this comparator should be + /// Decides based on the current environment whether this comparator should be /// used. - /// - /// 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') || platform.environment.containsKey('CIRRUS_CI'); @@ -559,13 +526,13 @@ class FlutterSkippingFileComparator extends FlutterGoldenFileComparator { /// /// * [GoldenFileComparator], the abstract class that /// [FlutterGoldenFileComparator] implements. -/// * [FlutterPostSubmitFileComparator], another +/// * [FlutterSkiaGoldFileComparator], another /// [FlutterGoldenFileComparator] that uploads tests to the Skia Gold /// dashboard. /// * [FlutterPreSubmitFileComparator], another /// [FlutterGoldenFileComparator] that tests golden images before changes are /// merged into the master branch. -/// * [FlutterSkippingFileComparator], another +/// * [FlutterSkippingGoldenFileComparator], another /// [FlutterGoldenFileComparator] that controls post-submit testing /// conditions that do not execute golden file tests. class FlutterLocalFileComparator extends FlutterGoldenFileComparator with LocalComparisonOutput { @@ -613,14 +580,14 @@ class FlutterLocalFileComparator extends FlutterGoldenFileComparator with LocalC try { await goldens.getExpectations(); } on io.OSError catch (_) { - return FlutterSkippingFileComparator( + return FlutterSkippingGoldenFileComparator( baseDirectory.uri, goldens, 'OSError occurred, could not reach Gold. ' 'Switching to FlutterSkippingGoldenFileComparator.', ); } on io.SocketException catch (_) { - return FlutterSkippingFileComparator( + return FlutterSkippingGoldenFileComparator( baseDirectory.uri, goldens, 'SocketException occurred, could not reach Gold. ' diff --git a/packages/flutter_goldens/test/flutter_goldens_test.dart b/packages/flutter_goldens/test/flutter_goldens_test.dart index 1d4c42193ee..d3489a7fba1 100644 --- a/packages/flutter_goldens/test/flutter_goldens_test.dart +++ b/packages/flutter_goldens/test/flutter_goldens_test.dart @@ -108,7 +108,6 @@ void main() { process: process, platform: platform, httpClient: mockHttpClient, - ci: ContinuousIntegrationEnvironment.cirrus, ); when(process.run(any)) @@ -166,76 +165,6 @@ void main() { ); }); - test('correctly inits tryjob for luci', () async { - platform = FakePlatform( - environment: { - 'FLUTTER_ROOT': _kFlutterRoot, - 'GOLDCTL' : 'goldctl', - 'SWARMING_TASK_ID' : '4ae997b50dfd4d11', - 'LOGDOG_STREAM_PREFIX' : 'buildbucket/cr-buildbucket.appspot.com/8885996262141582672', - 'GOLD_TRYJOB' : 'refs/pull/49815/head', - }, - operatingSystem: 'macos' - ); - - skiaClient = SkiaGoldClient( - workDirectory, - fs: fs, - process: process, - platform: platform, - httpClient: mockHttpClient, - ci: ContinuousIntegrationEnvironment.luci, - ); - - final List ciArguments = skiaClient.getCIArguments(); - - expect( - ciArguments, - equals( - [ - '--changelist', '49815', - '--cis', 'buildbucket', - '--jobid', '8885996262141582672', - ], - ), - ); - }); - - test('correctly inits tryjob for cirrus', () async { - platform = FakePlatform( - environment: { - 'FLUTTER_ROOT': _kFlutterRoot, - 'GOLDCTL' : 'goldctl', - 'CIRRUS_CI' : 'true', - 'CIRRUS_TASK_ID' : '8885996262141582672', - 'CIRRUS_PR' : '49815', - }, - operatingSystem: 'macos' - ); - - skiaClient = SkiaGoldClient( - workDirectory, - fs: fs, - process: process, - platform: platform, - httpClient: mockHttpClient, - ci: ContinuousIntegrationEnvironment.cirrus, - ); - - final List ciArguments = skiaClient.getCIArguments(); - - expect( - ciArguments, - equals( - [ - '--changelist', '49815', - '--cis', 'cirrus', - '--jobid', '8885996262141582672', - ], - ), - ); - }); - group('Request Handling', () { String testName; String pullRequestNumber; @@ -527,12 +456,12 @@ void main() { }); group('FlutterGoldenFileComparator', () { - FlutterPostSubmitFileComparator comparator; + FlutterSkiaGoldFileComparator comparator; setUp(() { final Directory basedir = fs.directory('flutter/test/library/') ..createSync(recursive: true); - comparator = FlutterPostSubmitFileComparator( + comparator = FlutterSkiaGoldFileComparator( basedir.uri, MockSkiaGoldClient(), fs: fs, @@ -568,7 +497,7 @@ void main() { setUp(() { final Directory basedir = fs.directory('flutter/test/library/') ..createSync(recursive: true); - comparator = FlutterPostSubmitFileComparator( + comparator = FlutterSkiaGoldFileComparator( basedir.uri, mockSkiaClient, fs: fs, @@ -577,21 +506,7 @@ void main() { }); group('correctly determines testing environment', () { - test('returns true for Luci', () { - platform = FakePlatform( - environment: { - 'FLUTTER_ROOT': _kFlutterRoot, - 'SWARMING_TASK_ID' : '12345678990', - }, - operatingSystem: 'macos' - ); - expect( - FlutterPostSubmitFileComparator.isAvailableForEnvironment(platform), - isTrue, - ); - }); - - test('returns true for Cirrus', () { + test('returns true', () { platform = FakePlatform( environment: { 'FLUTTER_ROOT': _kFlutterRoot, @@ -603,7 +518,7 @@ void main() { operatingSystem: 'macos' ); expect( - FlutterPostSubmitFileComparator.isAvailableForEnvironment(platform), + FlutterSkiaGoldFileComparator.isAvailableForEnvironment(platform), isTrue, ); }); @@ -620,7 +535,7 @@ void main() { operatingSystem: 'macos' ); expect( - FlutterPostSubmitFileComparator.isAvailableForEnvironment(platform), + FlutterSkiaGoldFileComparator.isAvailableForEnvironment(platform), isFalse, ); }); @@ -636,7 +551,7 @@ void main() { operatingSystem: 'macos' ); expect( - FlutterPostSubmitFileComparator.isAvailableForEnvironment(platform), + FlutterSkiaGoldFileComparator.isAvailableForEnvironment(platform), isFalse, ); }); @@ -651,7 +566,7 @@ void main() { operatingSystem: 'macos' ); expect( - FlutterPostSubmitFileComparator.isAvailableForEnvironment(platform), + FlutterSkiaGoldFileComparator.isAvailableForEnvironment(platform), isFalse, ); }); @@ -668,7 +583,7 @@ void main() { operatingSystem: 'macos' ); expect( - FlutterPostSubmitFileComparator.isAvailableForEnvironment(platform), + FlutterSkiaGoldFileComparator.isAvailableForEnvironment(platform), isFalse, ); }); @@ -680,7 +595,7 @@ void main() { final MockSkiaGoldClient mockSkiaClient = MockSkiaGoldClient(); group('correctly determines testing environment', () { - test('returns true for Cirrus', () { + test('returns true', () { platform = FakePlatform( environment: { 'FLUTTER_ROOT': _kFlutterRoot, @@ -696,21 +611,6 @@ void main() { ); }); - test('returns true for Luci', () { - platform = FakePlatform( - environment: { - 'FLUTTER_ROOT': _kFlutterRoot, - 'SWARMING_TASK_ID' : '12345678990', - 'GOLD_TRYJOB' : 'git/ref/12345/head' - }, - operatingSystem: 'macos' - ); - expect( - FlutterPreSubmitFileComparator.isAvailableForEnvironment(platform), - isTrue, - ); - }); - test('returns false - no PR', () { platform = FakePlatform( environment: { @@ -742,7 +642,7 @@ void main() { ); }); - test('returns false - not on Cirrus or Luci', () { + test('returns false - not on Cirrus', () { platform = FakePlatform( environment: { 'FLUTTER_ROOT': _kFlutterRoot, @@ -865,7 +765,21 @@ void main() { group('Skipping', () { group('correctly determines testing environment', () { - test('returns true on Cirrus shards that don\'t run golden tests', () { + test('returns true on LUCI', () { + platform = FakePlatform( + environment: { + 'FLUTTER_ROOT': _kFlutterRoot, + 'SWARMING_TASK_ID' : '1234567890', + }, + operatingSystem: 'macos' + ); + expect( + FlutterSkippingGoldenFileComparator.isAvailableForEnvironment(platform), + isTrue, + ); + }); + + test('returns true on Cirrus', () { platform = FakePlatform( environment: { 'FLUTTER_ROOT': _kFlutterRoot, @@ -874,11 +788,10 @@ void main() { operatingSystem: 'macos' ); expect( - FlutterSkippingFileComparator.isAvailableForEnvironment(platform), + FlutterSkippingGoldenFileComparator.isAvailableForEnvironment(platform), isTrue, ); }); - test('returns false - no CI', () { platform = FakePlatform( environment: { @@ -887,7 +800,7 @@ void main() { operatingSystem: 'macos' ); expect( - FlutterSkippingFileComparator.isAvailableForEnvironment( + FlutterSkippingGoldenFileComparator.isAvailableForEnvironment( platform), isFalse, ); @@ -978,7 +891,7 @@ void main() { goldens: mockSkiaClient, baseDirectory: mockDirectory, ); - expect(comparator.runtimeType, FlutterSkippingFileComparator); + expect(comparator.runtimeType, FlutterSkippingGoldenFileComparator); when(mockSkiaClient.getExpectations()) .thenAnswer((_) => throw const SocketException("Can't reach Gold")); @@ -987,7 +900,7 @@ void main() { goldens: mockSkiaClient, baseDirectory: mockDirectory, ); - expect(comparator.runtimeType, FlutterSkippingFileComparator); + expect(comparator.runtimeType, FlutterSkippingGoldenFileComparator); }); }); }); diff --git a/packages/flutter_goldens_client/lib/skia_client.dart b/packages/flutter_goldens_client/lib/skia_client.dart index a6b911818d0..ab32613788f 100644 --- a/packages/flutter_goldens_client/lib/skia_client.dart +++ b/packages/flutter_goldens_client/lib/skia_client.dart @@ -21,12 +21,6 @@ const String _kGoldctlKey = 'GOLDCTL'; const String _kServiceAccountKey = 'GOLD_SERVICE_ACCOUNT'; const String _kTestBrowserKey = 'FLUTTER_TEST_BROWSER'; -/// Enum representing the supported CI environments used by flutter/flutter. -enum ContinuousIntegrationEnvironment { - luci, - cirrus, -} - /// A client for uploading image tests and making baseline requests to the /// Flutter Gold Dashboard. class SkiaGoldClient { @@ -35,7 +29,6 @@ class SkiaGoldClient { this.fs = const LocalFileSystem(), this.process = const LocalProcessManager(), this.platform = const LocalPlatform(), - this.ci, io.HttpClient httpClient, }) : assert(workDirectory != null), assert(fs != null), @@ -45,26 +38,23 @@ class SkiaGoldClient { /// The file system to use for storing the local clone of the repository. /// - /// This is useful in tests, where a local file system (the default) can be - /// replaced by a memory file system. + /// This is useful in tests, where a local file system (the default) can + /// be replaced by a memory file system. final FileSystem fs; /// A wrapper for the [dart:io.Platform] API. /// - /// This is useful in tests, where the system platform (the default) can be - /// replaced by a mock platform instance. + /// This is useful in tests, where the system platform (the default) can + /// be replaced by a mock platform instance. final Platform platform; /// A controller for launching sub-processes. /// - /// This is useful in tests, where the real process manager (the default) can - /// be replaced by a mock process manager that doesn't really create + /// This is useful in tests, where the real process manager (the default) + /// can be replaced by a mock process manager that doesn't really create /// sub-processes. final ProcessManager process; - /// What testing environment we may be in, like Cirrus or Luci. - final ContinuousIntegrationEnvironment ci; - /// A client for making Http requests to the Flutter Gold dashboard. final io.HttpClient httpClient; @@ -79,9 +69,9 @@ class SkiaGoldClient { /// A map of known golden file tests and their associated positive image /// hashes. /// - /// This is set and used by the [FlutterLocalFileComparator] and the - /// [_UnauthorizedFlutterPreSubmitComparator] to test against golden masters - /// maintained in the Flutter Gold dashboard. + /// This is set and used by the [FlutterLocalFileComparator] and + /// [FlutterPreSubmitFileComparator] to test against golden masters maintained + /// in the Flutter Gold dashboard. Map> get expectations => _expectations; Map> _expectations; @@ -104,58 +94,32 @@ class SkiaGoldClient { /// Prepares the local work space for golden file testing and calls the /// goldctl `auth` command. /// - /// This ensures that the goldctl tool is authorized and ready for testing. - /// Used by the [FlutterPostSubmitFileComparator] and the - /// [_AuthorizedFlutterPreSubmitComparator]. - /// - /// Based on the current environment, the goldctl tool may be authorized by - /// a service account provided by Cirrus, or through the context provided by a - /// luci environment. + /// This ensures that the goldctl tool is authorized and ready for testing. It + /// will only be called once for each instance of + /// [FlutterSkiaGoldFileComparator]. Future auth() async { if (await clientIsAuthorized()) return; - List authArguments; - String failureContext; - - switch (ci) { - case ContinuousIntegrationEnvironment.luci: - authArguments = [ - 'auth', - '--work-dir', workDirectory - .childDirectory('temp') - .path, - '--luci', - ]; - failureContext = - 'Luci environments authenticate using the file provided ' - 'by LUCI_CONTEXT. There may be an error with this file or Gold ' - 'authentication.'; - break; - case ContinuousIntegrationEnvironment.cirrus: - if (_serviceAccount.isEmpty) { - final StringBuffer buf = StringBuffer() - ..writeln('The Gold service account is unavailable.')..writeln( - 'Without a service account, Gold can not be authorized.')..writeln( - 'Please check your user permissions and current comparator.'); - throw Exception(buf.toString()); - } - - final File authorization = workDirectory.childFile('serviceAccount.json'); - await authorization.writeAsString(_serviceAccount); - authArguments = [ - 'auth', - '--service-account', authorization.path, - '--work-dir', workDirectory - .childDirectory('temp') - .path, - ]; - failureContext = 'This could be caused by incorrect user permissions on ' - 'Cirrus, if the debug information below contains ENCRYPTED, the wrong ' - 'comparator was chosen for the test case.'; - break; + if (_serviceAccount.isEmpty) { + final StringBuffer buf = StringBuffer() + ..writeln('The Gold service account is unavailable.') + ..writeln('Without a service account, Gold can not be authorized.') + ..writeln('Please check your user permissions and current comparator.'); + throw Exception(buf.toString()); } + final File authorization = workDirectory.childFile('serviceAccount.json'); + await authorization.writeAsString(_serviceAccount); + + final List authArguments = [ + 'auth', + '--service-account', authorization.path, + '--work-dir', workDirectory + .childDirectory('temp') + .path, + ]; + final io.ProcessResult result = await io.Process.run( _goldctl, authArguments, @@ -164,7 +128,10 @@ class SkiaGoldClient { if (result.exitCode != 0) { final StringBuffer buf = StringBuffer() ..writeln('Skia Gold authorization failed.') - ..writeln(failureContext) + ..writeln('This could be caused by incorrect user permissions, if the ') + ..writeln('debug information below contains ENCRYPTED, the wrong ') + ..writeln('comparator was chosen for the test case.') + ..writeln() ..writeln('Debug information for Gold:') ..writeln('stdout: ${result.stdout}') ..writeln('stderr: ${result.stderr}'); @@ -178,10 +145,6 @@ class SkiaGoldClient { /// It will only be called once for each instance of an /// [_UnauthorizedFlutterPreSubmitComparator]. Future emptyAuth() async { - // We only use emptyAuth when the service account cannot be decrypted on - // Cirrus. - assert(ci == ContinuousIntegrationEnvironment.cirrus); - final List authArguments = [ 'auth', '--work-dir', workDirectory @@ -208,8 +171,7 @@ class SkiaGoldClient { /// Executes the `imgtest init` command in the goldctl tool. /// /// The `imgtest` command collects and uploads test results to the Skia Gold - /// backend, the `init` argument initializes the current test. Used by the - /// [FlutterPostSubmitFileComparator]. + /// backend, the `init` argument initializes the current test. Future imgtestInit() async { final File keys = workDirectory.childFile('keys.json'); final File failures = workDirectory.childFile('failures.json'); @@ -265,7 +227,7 @@ class SkiaGoldClient { /// result. /// /// The [testName] and [goldenFile] parameters reference the current - /// comparison being evaluated by the [FlutterPostSubmitFileComparator]. + /// comparison being evaluated by the [FlutterSkiaGoldFileComparator]. Future imgtestAdd(String testName, File goldenFile) async { assert(testName != null); assert(goldenFile != null); @@ -298,8 +260,7 @@ class SkiaGoldClient { /// Executes the `imgtest init` command in the goldctl tool for tryjobs. /// /// The `imgtest` command collects and uploads test results to the Skia Gold - /// backend, the `init` argument initializes the current tryjob. Used by the - /// [_AuthorizedFlutterPreSubmitComparator]. + /// backend, the `init` argument initializes the current tryjob. Future tryjobInit() async { final File keys = workDirectory.childFile('keys.json'); final File failures = workDirectory.childFile('failures.json'); @@ -307,6 +268,9 @@ class SkiaGoldClient { await keys.writeAsString(_getKeysJSON()); await failures.create(); final String commitHash = await _getCurrentCommit(); + final String pullRequest = platform.environment['CIRRUS_PR']; + final String cirrusTaskID = platform.environment['CIRRUS_TASK_ID']; + final List imgtestInitArguments = [ 'imgtest', 'init', @@ -319,11 +283,12 @@ class SkiaGoldClient { '--failure-file', failures.path, '--passfail', '--crs', 'github', + '--changelist', pullRequest, + '--cis', 'cirrus', + '--jobid', cirrusTaskID, '--patchset_id', commitHash, ]; - imgtestInitArguments.addAll(getCIArguments()); - if (imgtestInitArguments.contains(null)) { final StringBuffer buf = StringBuffer() ..writeln('A null argument was provided for Skia Gold tryjob init.') @@ -487,12 +452,11 @@ class SkiaGoldClient { /// Returns a boolean value for whether or not the given test and current pull /// request are ignored on Flutter Gold. /// - /// This is only relevant when used by the - /// [_UnauthorizedFlutterPreSubmitComparator] when a golden file test fails. - /// In order to land a change to an existing golden file, an ignore must be - /// set up in Flutter Gold. This will serve as a flag to permit the change to - /// land, protect against any unwanted changes, and ensure that changes that - /// have landed are triaged. + /// This is only relevant when used by the [FlutterPreSubmitFileComparator] + /// when a golden file test fails. In order to land a change to an existing + /// golden file, an ignore must be set up in Flutter Gold. This will serve as + /// a flag to permit the change to land, protect against any unwanted changes, + /// and ensure that changes that have landed are triaged. Future testIsIgnoredForPullRequest(String pullRequest, String testName) async { bool ignoreIsActive = false; testName = cleanTestName(testName); @@ -610,7 +574,6 @@ class SkiaGoldClient { String _getKeysJSON() { final Map keys = { 'Platform' : platform.operatingSystem, - 'CI' : ci.toString().split('.').last, }; if (platform.environment[_kTestBrowserKey] != null) keys['Browser'] = platform.environment[_kTestBrowserKey]; @@ -638,34 +601,6 @@ class SkiaGoldClient { } return false; } - - /// Returns a list of arguments for initializing a tryjob based on the testing - /// environment. - List getCIArguments() { - String pullRequest; - String jobId; - String cis; - - switch (ci) { - case ContinuousIntegrationEnvironment.luci: - jobId = platform.environment['LOGDOG_STREAM_PREFIX'].split('/').last; - final List refs = platform.environment['GOLD_TRYJOB'].split('/'); - pullRequest = refs[refs.length - 2]; - cis = 'buildbucket'; - break; - case ContinuousIntegrationEnvironment.cirrus: - pullRequest = platform.environment['CIRRUS_PR']; - jobId = platform.environment['CIRRUS_TASK_ID']; - cis = 'cirrus'; - break; - } - - return [ - '--changelist', pullRequest, - '--cis', cis, - '--jobid', jobId, - ]; - } } /// Used to make HttpRequests during testing. @@ -688,10 +623,7 @@ class SkiaGoldDigest { return SkiaGoldDigest( imageHash: json['digest'] as String, paramSet: Map.from(json['paramset'] as Map ?? - >{ - 'Platform': [], - 'Browser' : [], - }), + >{'Platform': []}), testName: json['test'] as String, status: json['status'] as String, );