From 72696f77ddc5d04712d14bef688a32e88de12f6b Mon Sep 17 00:00:00 2001 From: Kate Lovett Date: Thu, 8 Oct 2020 12:19:42 -0700 Subject: [PATCH] Remove Cirrus support for Gold (#67468) --- .cirrus.yml | 3 - dev/bots/download_goldctl.ps1 | 18 - dev/bots/download_goldctl.sh | 9 - dev/ci/docker_linux/Dockerfile | 5 - packages/flutter/test/widgets/basic_test.dart | 2 +- .../flutter_goldens/lib/flutter_goldens.dart | 183 +------ .../test/flutter_goldens_test.dart | 491 +++--------------- .../flutter_goldens/test/json_templates.dart | 31 -- .../lib/skia_client.dart | 132 +---- 9 files changed, 125 insertions(+), 749 deletions(-) delete mode 100644 dev/bots/download_goldctl.ps1 delete mode 100755 dev/bots/download_goldctl.sh diff --git a/.cirrus.yml b/.cirrus.yml index 0f0903c74e..2ad36c5e2e 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -89,7 +89,6 @@ task: # We use 3 CPUs because that's the minimum required to get framework_tests-widgets-linux # running fast enough that it is not the long pole, as of October 2019. CPU: 3 - GOLD_SERVICE_ACCOUNT: ENCRYPTED[3afeea5ac7201151c3d0dc9648862f0462b5e4f55dc600ca8b692319622f7c3eda3d577b1b16cc2ef0311b7314c1c095] script: - dart --enable-asserts ./dev/bots/test.dart @@ -100,7 +99,6 @@ task: # framework_tests-libraries-linux shard running fast enough that it is not the long pole, as # of October 2019. CPU: 3 - GOLD_SERVICE_ACCOUNT: ENCRYPTED[3afeea5ac7201151c3d0dc9648862f0462b5e4f55dc600ca8b692319622f7c3eda3d577b1b16cc2ef0311b7314c1c095] script: - dart --enable-asserts ./dev/bots/test.dart @@ -150,7 +148,6 @@ task: CPU: 2 MEMORY: 8G CHROME_NO_SANDBOX: true - GOLD_SERVICE_ACCOUNT: ENCRYPTED[3afeea5ac7201151c3d0dc9648862f0462b5e4f55dc600ca8b692319622f7c3eda3d577b1b16cc2ef0311b7314c1c095] script: - dart --enable-asserts ./dev/bots/test.dart diff --git a/dev/bots/download_goldctl.ps1 b/dev/bots/download_goldctl.ps1 deleted file mode 100644 index a53e40e156..0000000000 --- a/dev/bots/download_goldctl.ps1 +++ /dev/null @@ -1,18 +0,0 @@ -# Copyright 2014 The Flutter Authors. All rights reserved. -# Use of this source code is governed by a BSD-style license that can be -# found in the LICENSE file. - -$url= "https://storage.googleapis.com/chrome-infra/depot_tools.zip" -$zipPath = "C:\Windows\Temp\depot_tools.zip" -$path = "C:\Windows\Temp\depot_tools" -$gclient = "C:\Windows\Temp\depot_tools\gclient.bat" -$cipd = "C:\Windows\Temp\depot_tools\cipd.bat" -$ensureFile = "C:\Windows\Temp\depot_tools\ensure.txt" -$text = "# Ensure File`n`$ServiceURL https://chrome-infra-packages.appspot.com`n`n# Skia Gold Client goldctl`nskia/tools/goldctl/`${platform} git_revision:b57f561ad4ad624bd399b8b7b500aa1955276d41" - -(New-Object System.Net.WebClient).DownloadFile($url, $zipPath) -Expand-Archive -LiteralPath $zipPath -DestinationPath $path -cd $path -cmd.exe /C "$gclient" -$text | Out-File -filePath $ensureFile -encoding ascii -cmd.exe /C "$cipd ensure -ensure-file $ensureFile -root $path" diff --git a/dev/bots/download_goldctl.sh b/dev/bots/download_goldctl.sh deleted file mode 100755 index fce310dc86..0000000000 --- a/dev/bots/download_goldctl.sh +++ /dev/null @@ -1,9 +0,0 @@ -#!/usr/bin/env bash -# Copyright 2014 The Flutter Authors. All rights reserved. -# Use of this source code is governed by a BSD-style license that can be -# found in the LICENSE file. - -git clone --depth 1 https://chromium.googlesource.com/chromium/tools/depot_tools.git ./depot_tools -cd depot_tools -echo -e '# Ensure File\n$ServiceURL https://chrome-infra-packages.appspot.com\n\n# Skia Gold Client goldctl\nskia/tools/goldctl/${platform} git_revision:b57f561ad4ad624bd399b8b7b500aa1955276d41' > ensure.txt -./cipd ensure -ensure-file ./ensure.txt -root . diff --git a/dev/ci/docker_linux/Dockerfile b/dev/ci/docker_linux/Dockerfile index 05afc966f5..b6de029d87 100644 --- a/dev/ci/docker_linux/Dockerfile +++ b/dev/ci/docker_linux/Dockerfile @@ -123,8 +123,3 @@ COPY ci/docker_linux/Gemfile.lock /Gemfile.lock RUN bundle config set system 'true' && \ bundle install --system - -# Install goldctl, for Golden testing -COPY bots/download_goldctl.sh /download_goldctl.sh -ENV GOLDCTL '/depot_tools/goldctl' -RUN /download_goldctl.sh diff --git a/packages/flutter/test/widgets/basic_test.dart b/packages/flutter/test/widgets/basic_test.dart index d4a675b901..11a4b54e5d 100644 --- a/packages/flutter/test/widgets/basic_test.dart +++ b/packages/flutter/test/widgets/basic_test.dart @@ -443,7 +443,7 @@ void main() { // golden file can be approved at any time. await tester.pumpWidget(RepaintBoundary( child: Container( - color: const Color(0xFF42A5F5), + color: const Color(0xABCDABCD), ), )); diff --git a/packages/flutter_goldens/lib/flutter_goldens.dart b/packages/flutter_goldens/lib/flutter_goldens.dart index 4d9afb90c6..bc29e1a46a 100644 --- a/packages/flutter_goldens/lib/flutter_goldens.dart +++ b/packages/flutter_goldens/lib/flutter_goldens.dart @@ -35,7 +35,7 @@ Future testExecutable(FutureOr testMain()) async { goldenFileComparator = await FlutterPreSubmitFileComparator.fromDefaultComparator(platform); } else if (FlutterSkippingFileComparator.isAvailableForEnvironment(platform)) { goldenFileComparator = FlutterSkippingFileComparator.fromDefaultComparator( - 'Golden file testing is not executed on some Cirrus & Luci environments.' + 'Golden file testing is not executed on Cirrus, or LUCI environments outside of flutter/flutter.' ); } else { goldenFileComparator = await FlutterLocalFileComparator.fromDefaultComparator(platform); @@ -84,10 +84,10 @@ Future testExecutable(FutureOr testMain()) async { /// 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 and Luci environments do not execute golden file testing, and -/// as such do not require a comparator. This comparator is also used when an -/// internet connection is unavailable. +/// of the appropriate environments described above. Currently, some Luci +/// environments do not execute golden file testing, and as such do not require +/// a comparator. This comparator is also used when an internet connection is +/// unavailable. abstract class FlutterGoldenFileComparator extends GoldenFileComparator { /// Creates a [FlutterGoldenFileComparator] that will resolve golden file /// URIs relative to the specified [basedir], and retrieve golden baselines @@ -241,12 +241,7 @@ 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); @@ -264,30 +259,18 @@ class FlutterPostSubmitFileComparator extends FlutterGoldenFileComparator { /// Decides based on the current environment if goldens tests should be /// executed through 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') - && cirrusPR.isEmpty - && cirrusBranch == 'master' - && platform.environment.containsKey('GOLD_SERVICE_ACCOUNT'); - 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'); - return cirrusPostSubmit || luciPostSubmit; + return 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. +/// merged into the master branch. The comparator executes tryjobs using the +/// [SkiaGoldClient]. /// /// See also: /// @@ -339,80 +322,17 @@ 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) { - await goldens.auth(); - await goldens.tryjobInit(); - return _AuthorizedFlutterPreSubmitComparator( + await goldens.auth(); + await goldens.tryjobInit(); + return FlutterPreSubmitFileComparator( baseDirectory.uri, goldens, platform: platform, ); } - goldens.emptyAuth(); - return _UnauthorizedFlutterPreSubmitComparator( - baseDirectory.uri, - goldens, - platform: platform, - ); - } - - @override - Future compare(Uint8List imageBytes, Uri golden) async { - assert( - false, - 'The FlutterPreSubmitFileComparator has been used to execute a golden ' - 'file test; this should never happen. Presubmit golden file testing ' - 'should be executed by either the _AuthorizedFlutterPreSubmitComparator ' - 'or the _UnauthorizedFlutterPreSubmitComparator based on contributor ' - 'permissions.' - ); - return false; - } - - /// Decides based on the current environment if goldens tests should be - /// executed 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') - && cirrusPR.isNotEmpty - && platform.environment.containsKey('GOLD_SERVICE_ACCOUNT'); - - final bool luciPreSubmit = platform.environment.containsKey('SWARMING_TASK_ID') - && platform.environment.containsKey('GOLDCTL') - && platform.environment.containsKey('GOLD_TRYJOB'); - return cirrusPreSubmit || luciPreSubmit; - } -} - -class _AuthorizedFlutterPreSubmitComparator extends FlutterPreSubmitFileComparator { - _AuthorizedFlutterPreSubmitComparator( - final Uri basedir, - final SkiaGoldClient skiaClient, { - final FileSystem fs = const LocalFileSystem(), - final Platform platform = const LocalPlatform(), - }) : super( - basedir, - skiaClient, - fs: fs, - platform: platform, - ); - @override Future compare(Uint8List imageBytes, Uri golden) async { golden = _addPrefix(golden); @@ -425,74 +345,22 @@ class _AuthorizedFlutterPreSubmitComparator extends FlutterPreSubmitFileComparat // in pre-submit checks by the flutter-gold status check. return true; } -} -class _UnauthorizedFlutterPreSubmitComparator extends FlutterPreSubmitFileComparator { - _UnauthorizedFlutterPreSubmitComparator( - final Uri basedir, - final SkiaGoldClient skiaClient, { - final FileSystem fs = const LocalFileSystem(), - final Platform platform = const LocalPlatform(), - }) : super( - basedir, - skiaClient, - fs: fs, - platform: platform, - ); - - @override - Future compare(Uint8List imageBytes, Uri golden) async { - golden = _addPrefix(golden); - await update(golden, imageBytes); - final File goldenFile = getGoldenFile(golden); - - // Check for match to existing baseline. - if (await skiaClient.imgtestCheck(golden.path, goldenFile)) - return true; - - // We do not have a matching image hash, so we need to check manually. - final String testName = skiaClient.cleanTestName(golden.path); - final String? testExpectation = await skiaClient.getExpectationForTest(testName); - if (testExpectation == null) { - // This is a new test. - print('No expectations provided by Skia Gold for test: $golden. ' - 'This may be a new test. If this is an unexpected result, check ' - 'https://flutter-gold.skia.org.\n' - ); - return true; - } - - // Contributors without the proper permissions to execute a tryjob can make - // a golden file change through Gold's ignore feature instead. - String? pullRequest; - switch(skiaClient.ci) { - case ContinuousIntegrationEnvironment.cirrus: - pullRequest = platform.environment['CIRRUS_PR']!; - break; - case ContinuousIntegrationEnvironment.luci: - final List refs = platform.environment['GOLD_TRYJOB']!.split('/'); - pullRequest = refs[refs.length - 2]; - break; - case ContinuousIntegrationEnvironment.none: - pullRequest = ''; - break; - } - - final bool ignoreResult = await skiaClient.testIsIgnoredForPullRequest( - pullRequest, - golden.path, - ); - // If true, this is an intended change and is being handled on the Flutter - // Gold dashboard: https://flutter-gold.skia.org/ignores - return ignoreResult; + /// Decides based on the current environment if goldens tests should be + /// executed as pre-submit tests with Skia Gold. + static bool isAvailableForEnvironment(Platform platform) { + final bool luciPreSubmit = platform.environment.containsKey('SWARMING_TASK_ID') + && platform.environment.containsKey('GOLDCTL') + && platform.environment.containsKey('GOLD_TRYJOB'); + return luciPreSubmit; } } /// A [FlutterGoldenFileComparator] for testing conditions that do not execute /// golden file tests. /// -/// Currently, this comparator is used in some Cirrus test shards and Luci -/// environments. +/// Currently, this comparator is used on Cirrus, or in Luci environments when executing tests +/// outside of the flutter/flutter repository. /// /// See also: /// @@ -527,7 +395,7 @@ class FlutterSkippingFileComparator extends FlutterGoldenFileComparator { defaultComparator ??= goldenFileComparator as LocalFileComparator; const FileSystem fs = LocalFileSystem(); final Uri basedir = defaultComparator.basedir; - final SkiaGoldClient skiaClient = SkiaGoldClient(fs.directory(basedir), ci: ContinuousIntegrationEnvironment.none); + final SkiaGoldClient skiaClient = SkiaGoldClient(fs.directory(basedir)); return FlutterSkippingFileComparator(basedir, skiaClient, reason); } @@ -545,10 +413,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, LUCI or Cirrus, but are not using the other /// comparators, we skip. static bool isAvailableForEnvironment(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'); } } @@ -619,7 +488,7 @@ class FlutterLocalFileComparator extends FlutterGoldenFileComparator with LocalC baseDirectory.createSync(recursive: true); } - goldens ??= SkiaGoldClient(baseDirectory, ci: ContinuousIntegrationEnvironment.none); + goldens ??= SkiaGoldClient(baseDirectory); try { // Check if we can reach Gold. await goldens.getExpectationForTest(''); diff --git a/packages/flutter_goldens/test/flutter_goldens_test.dart b/packages/flutter_goldens/test/flutter_goldens_test.dart index 16fad995fe..bdb95676c0 100644 --- a/packages/flutter_goldens/test/flutter_goldens_test.dart +++ b/packages/flutter_goldens/test/flutter_goldens_test.dart @@ -3,8 +3,8 @@ // found in the LICENSE file. // @dart = 2.8 + import 'dart:async'; -import 'dart:convert'; import 'dart:core'; import 'dart:io'; import 'dart:typed_data'; @@ -76,7 +76,6 @@ void main() { process: process, platform: platform, httpClient: mockHttpClient, - ci: ContinuousIntegrationEnvironment.luci, ); }); @@ -121,7 +120,6 @@ void main() { process: process, platform: platform, httpClient: mockHttpClient, - ci: ContinuousIntegrationEnvironment.cirrus, ); when(process.run(any)) @@ -150,7 +148,6 @@ void main() { process: process, platform: platform, httpClient: mockHttpClient, - ci: ContinuousIntegrationEnvironment.luci ); when(process.run( @@ -198,7 +195,6 @@ void main() { process: process, platform: platform, httpClient: mockHttpClient, - ci: ContinuousIntegrationEnvironment.luci, ); final List ciArguments = skiaClient.getCIArguments(); @@ -215,73 +211,8 @@ void main() { ); }); - 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', - ], - ), - ); - }); - test('Creates traceID correctly', () { String traceID; - - // On Cirrus - 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, - ); - - traceID = skiaClient.getTraceID('flutter.golden.1'); - - expect( - traceID, - equals(',CI=cirrus,Platform=macos,name=flutter.golden.1,source_type=flutter,'), - ); - - // On Luci platform = FakePlatform( environment: { 'FLUTTER_ROOT': _kFlutterRoot, @@ -299,7 +230,6 @@ void main() { process: process, platform: platform, httpClient: mockHttpClient, - ci: ContinuousIntegrationEnvironment.luci, ); traceID = skiaClient.getTraceID('flutter.golden.1'); @@ -328,7 +258,6 @@ void main() { process: process, platform: platform, httpClient: mockHttpClient, - ci: ContinuousIntegrationEnvironment.luci, ); traceID = skiaClient.getTraceID('flutter.golden.1'); @@ -352,7 +281,6 @@ void main() { process: process, platform: platform, httpClient: mockHttpClient, - ci: ContinuousIntegrationEnvironment.luci, ); traceID = skiaClient.getTraceID('flutter.golden.1'); @@ -364,13 +292,9 @@ void main() { }); group('Request Handling', () { - String testName; - String pullRequestNumber; String expectation; setUp(() { - testName = 'flutter.golden_test.1.png'; - pullRequestNumber = '1234'; expectation = '55109a4bed52acc780530f7a9aeff6c0'; }); @@ -391,120 +315,6 @@ void main() { expect(masterBytes, equals(_kTestPngBytes)); }); - - group('ignores', () { - Uri url; - MockHttpClientRequest mockHttpRequest; - MockHttpClientResponse mockHttpResponse; - - setUp(() { - url = Uri.parse('https://flutter-gold.skia.org/json/v1/ignores'); - mockHttpRequest = MockHttpClientRequest(); - mockHttpResponse = MockHttpClientResponse(utf8.encode( - ignoreResponseTemplate( - pullRequestNumber: pullRequestNumber, - expires: DateTime.now() - .add(const Duration(days: 1)) - .toString(), - otherTestName: 'unrelatedTest.1' - ) - )); - when(mockHttpClient.getUrl(url)) - .thenAnswer((_) => Future.value(mockHttpRequest)); - when(mockHttpRequest.close()) - .thenAnswer((_) => Future.value(mockHttpResponse)); - }); - - test('returns true for ignored test and ignored pull request number', () async { - expect( - await skiaClient.testIsIgnoredForPullRequest( - pullRequestNumber, - testName, - ), - isTrue, - ); - }); - - test('returns true for ignored test and not ignored pull request number', () async { - expect( - await skiaClient.testIsIgnoredForPullRequest( - '5678', - testName, - ), - isTrue, - ); - }); - - test('returns false for not ignored test and ignored pull request number', () async { - expect( - await skiaClient.testIsIgnoredForPullRequest( - pullRequestNumber, - 'failure.png', - ), - isFalse, - ); - }); - - test('throws exception for expired ignore', () async { - mockHttpResponse = MockHttpClientResponse(utf8.encode( - ignoreResponseTemplate( - pullRequestNumber: pullRequestNumber, - ) - )); - when(mockHttpRequest.close()) - .thenAnswer((_) => Future.value(mockHttpResponse)); - final Future test = skiaClient.testIsIgnoredForPullRequest( - pullRequestNumber, - testName, - ); - expect( - test, - throwsException, - ); - }); - - test('throws exception for first expired ignore among multiple', () async { - mockHttpResponse = MockHttpClientResponse(utf8.encode( - ignoreResponseTemplate( - pullRequestNumber: pullRequestNumber, - otherExpires: DateTime.now() - .add(const Duration(days: 1)) - .toString(), - ) - )); - when(mockHttpRequest.close()) - .thenAnswer((_) => Future.value(mockHttpResponse)); - final Future test = skiaClient.testIsIgnoredForPullRequest( - pullRequestNumber, - testName, - ); - expect( - test, - throwsException, - ); - }); - - test('throws exception for later expired ignore among multiple', () async { - mockHttpResponse = MockHttpClientResponse(utf8.encode( - ignoreResponseTemplate( - pullRequestNumber: pullRequestNumber, - expires: DateTime.now() - .add(const Duration(days: 1)) - .toString(), - ) - )); - when(mockHttpRequest.close()) - .thenAnswer((_) => Future.value(mockHttpResponse)); - final Future test = skiaClient.testIsIgnoredForPullRequest( - pullRequestNumber, - testName, - ); - expect( - test, - throwsException, - ); - }); - }); }); }); @@ -559,7 +369,7 @@ void main() { }); group('correctly determines testing environment', () { - test('returns true for Luci', () { + test('returns true for configured Luci', () { platform = FakePlatform( environment: { 'FLUTTER_ROOT': _kFlutterRoot, @@ -574,31 +384,11 @@ void main() { ); }); - test('returns true for Cirrus', () { + test('returns false - GOLDCTL not present', () { platform = FakePlatform( environment: { 'FLUTTER_ROOT': _kFlutterRoot, - 'CIRRUS_CI': 'true', - 'CIRRUS_PR': '', - 'CIRRUS_BRANCH': 'master', - 'GOLD_SERVICE_ACCOUNT': 'service account...', - }, - operatingSystem: 'macos' - ); - expect( - FlutterPostSubmitFileComparator.isAvailableForEnvironment(platform), - isTrue, - ); - }); - - test('returns false - PR active', () { - platform = FakePlatform( - environment: { - 'FLUTTER_ROOT': _kFlutterRoot, - 'CIRRUS_CI': 'true', - 'CIRRUS_PR': '1234', - 'CIRRUS_BRANCH': 'master', - 'GOLD_SERVICE_ACCOUNT': 'service account...', + 'SWARMING_TASK_ID' : '12345678990', }, operatingSystem: 'macos' ); @@ -608,44 +398,29 @@ void main() { ); }); - test('returns false - no service account', () { + test('returns false - GOLD_TRYJOB active', () { + platform = FakePlatform( + environment: { + 'FLUTTER_ROOT': _kFlutterRoot, + 'SWARMING_TASK_ID' : '12345678990', + 'GOLDCTL' : 'goldctl', + 'GOLD_TRYJOB' : 'git/ref/12345/head' + }, + operatingSystem: 'macos' + ); + expect( + FlutterPostSubmitFileComparator.isAvailableForEnvironment(platform), + isFalse, + ); + }); + + test('returns false - on Cirrus', () { platform = FakePlatform( environment: { 'FLUTTER_ROOT': _kFlutterRoot, 'CIRRUS_CI': 'true', 'CIRRUS_PR': '', 'CIRRUS_BRANCH': 'master', - }, - operatingSystem: 'macos' - ); - expect( - FlutterPostSubmitFileComparator.isAvailableForEnvironment(platform), - isFalse, - ); - }); - - test('returns false - not on cirrus', () { - platform = FakePlatform( - environment: { - 'FLUTTER_ROOT': _kFlutterRoot, - 'SWARMING_ID' : '1234567890', - 'GOLD_SERVICE_ACCOUNT': 'service account...' - }, - operatingSystem: 'macos' - ); - expect( - FlutterPostSubmitFileComparator.isAvailableForEnvironment(platform), - isFalse, - ); - }); - - test('returns false - not on master', () { - platform = FakePlatform( - environment: { - 'FLUTTER_ROOT': _kFlutterRoot, - 'CIRRUS_CI': 'true', - 'CIRRUS_PR': '', - 'CIRRUS_BRANCH': 'hotfix', 'GOLD_SERVICE_ACCOUNT': 'service account...' }, operatingSystem: 'macos' @@ -659,26 +434,7 @@ void main() { }); group('Pre-Submit', () { - FlutterGoldenFileComparator comparator; - final MockSkiaGoldClient mockSkiaClient = MockSkiaGoldClient(); - group('correctly determines testing environment', () { - test('returns true for Cirrus', () { - platform = FakePlatform( - environment: { - 'FLUTTER_ROOT': _kFlutterRoot, - 'CIRRUS_CI': 'true', - 'CIRRUS_PR': '1234', - 'GOLD_SERVICE_ACCOUNT' : 'service account...', - }, - operatingSystem: 'macos' - ); - expect( - FlutterPreSubmitFileComparator.isAvailableForEnvironment(platform), - isTrue, - ); - }); - test('returns true for Luci', () { platform = FakePlatform( environment: { @@ -695,166 +451,71 @@ void main() { ); }); - test('returns false - no PR', () { + test('returns false - not on Luci', () { + platform = FakePlatform( + environment: { + 'FLUTTER_ROOT': _kFlutterRoot, + }, + operatingSystem: 'macos' + ); + expect( + FlutterPreSubmitFileComparator.isAvailableForEnvironment(platform), + isFalse, + ); + }); + + test('returns false - GOLDCTL missing', () { + platform = FakePlatform( + environment: { + 'FLUTTER_ROOT': _kFlutterRoot, + 'SWARMING_TASK_ID' : '12345678990', + 'GOLD_TRYJOB' : 'git/ref/12345/head' + }, + operatingSystem: 'macos' + ); + expect( + FlutterPreSubmitFileComparator.isAvailableForEnvironment(platform), + isFalse, + ); + }); + + test('returns false - GOLD_TRYJOB missing', () { + platform = FakePlatform( + environment: { + 'FLUTTER_ROOT': _kFlutterRoot, + 'SWARMING_TASK_ID' : '12345678990', + 'GOLDCTL' : 'goldctl', + }, + operatingSystem: 'macos' + ); + expect( + FlutterPreSubmitFileComparator.isAvailableForEnvironment(platform), + isFalse, + ); + }); + + test('returns false - on Cirrus', () { platform = FakePlatform( environment: { 'FLUTTER_ROOT': _kFlutterRoot, 'CIRRUS_CI': 'true', 'CIRRUS_PR': '', - 'GOLD_SERVICE_ACCOUNT' : 'service account...', + 'CIRRUS_BRANCH': 'master', + 'GOLD_SERVICE_ACCOUNT': 'service account...' }, operatingSystem: 'macos' ); expect( - FlutterPreSubmitFileComparator.isAvailableForEnvironment(platform), + FlutterPostSubmitFileComparator.isAvailableForEnvironment(platform), isFalse, ); }); - - test('returns false - no service account', () { - platform = FakePlatform( - environment: { - 'FLUTTER_ROOT': _kFlutterRoot, - 'CIRRUS_CI': 'true', - 'CIRRUS_PR': '1234', - }, - operatingSystem: 'macos' - ); - expect( - FlutterPreSubmitFileComparator.isAvailableForEnvironment(platform), - isFalse, - ); - }); - - test('returns false - not on Cirrus or Luci', () { - platform = FakePlatform( - environment: { - 'FLUTTER_ROOT': _kFlutterRoot, - }, - operatingSystem: 'macos' - ); - expect( - FlutterPreSubmitFileComparator.isAvailableForEnvironment(platform), - isFalse, - ); - }); - }); - - group('_Authorized', () { - setUp(() async { - final Directory basedir = fs.directory('flutter/test/library/') - ..createSync(recursive: true); - comparator = await FlutterPreSubmitFileComparator.fromDefaultComparator( - FakePlatform( - environment: { - 'FLUTTER_ROOT': _kFlutterRoot, - 'CIRRUS_CI' : 'true', - 'CIRRUS_PR' : '1234', - 'GOLD_SERVICE_ACCOUNT' : 'service account...', - 'CIRRUS_USER_PERMISSION' : 'admin', - }, - operatingSystem: 'macos' - ), - goldens: mockSkiaClient, - testBasedir: basedir, - ); - }); - - test('fromDefaultComparator chooses correct comparator', () async { - expect( - comparator.runtimeType.toString(), - '_AuthorizedFlutterPreSubmitComparator', - ); - }); - }); - - group('_UnAuthorized', () { - setUp(() async { - final Directory basedir = fs.directory('flutter/test/library/') - ..createSync(recursive: true); - comparator = await FlutterPreSubmitFileComparator.fromDefaultComparator( - FakePlatform( - environment: { - 'FLUTTER_ROOT': _kFlutterRoot, - 'CIRRUS_CI' : 'true', - 'CIRRUS_PR' : '1234', - 'GOLD_SERVICE_ACCOUNT' : 'ENCRYPTED[...]', - 'CIRRUS_USER_PERMISSION' : 'none', - }, - operatingSystem: 'macos' - ), - goldens: mockSkiaClient, - testBasedir: basedir, - ); - when(mockSkiaClient.cleanTestName('library.flutter.golden_test.1.png')) - .thenReturn('flutter.golden_test.1'); - }); - - test('fromDefaultComparator chooses correct comparator', () async { - expect( - comparator.runtimeType.toString(), - '_UnauthorizedFlutterPreSubmitComparator', - ); - }); - - test('comparison passes test that is ignored for this PR', () async { - when(mockSkiaClient.imgtestCheck(any, any)) - .thenAnswer((_) => Future.value(false)); - when(mockSkiaClient.getExpectationForTest('flutter.golden_test.1')) - .thenAnswer((_) => Future.value('123456789abc')); - when(mockSkiaClient.ci).thenReturn(ContinuousIntegrationEnvironment.cirrus); - when(mockSkiaClient.testIsIgnoredForPullRequest( - '1234', - 'library.flutter.golden_test.1.png', - )) - .thenAnswer((_) => Future.value(true)); - expect( - await comparator.compare( - Uint8List.fromList(_kFailPngBytes), - Uri.parse('flutter.golden_test.1.png'), - ), - isTrue, - ); - }); - - test('fails test that is not ignored', () async { - when(mockSkiaClient.imgtestCheck(any, any)) - .thenAnswer((_) => Future.value(false)); - when(mockSkiaClient.getExpectationForTest('flutter.golden_test.1')) - .thenAnswer((_) => Future.value('123456789abc')); - when(mockSkiaClient.ci).thenReturn(ContinuousIntegrationEnvironment.cirrus); - when(mockSkiaClient.testIsIgnoredForPullRequest( - '1234', - 'library.flutter.golden_test.1.png', - )) - .thenAnswer((_) => Future.value(false)); - expect( - await comparator.compare( - Uint8List.fromList(_kFailPngBytes), - Uri.parse('flutter.golden_test.1.png'), - ), - isFalse, - ); - }); - - testWithOutput('passes non-existent baseline for new test', () async { - when(mockSkiaClient.cleanTestName('library.flutter.new_golden_test.1.png')) - .thenReturn('flutter.new_golden_test.1'); - expect( - await comparator.compare( - Uint8List.fromList(_kFailPngBytes), - Uri.parse('flutter.new_golden_test.1.png'), - ), - isTrue, - ); - }, 'No expectations provided by Skia Gold for test: library.flutter.new_golden_test.1.png. ' - 'This may be a new test. If this is an unexpected result, check https://flutter-gold.skia.org.\n'); }); }); group('Skipping', () { group('correctly determines testing environment', () { - test('returns true on Cirrus shards that don\'t run golden tests', () { + test('returns true on Cirrus builds', () { platform = FakePlatform( environment: { 'FLUTTER_ROOT': _kFlutterRoot, @@ -868,6 +529,20 @@ void main() { ); }); + test('returns true on irrelevant LUCI builds', () { + platform = FakePlatform( + environment: { + 'FLUTTER_ROOT': _kFlutterRoot, + 'SWARMING_TASK_ID' : '1234567890', + }, + operatingSystem: 'macos' + ); + expect( + FlutterSkippingFileComparator.isAvailableForEnvironment(platform), + isTrue, + ); + }); + test('returns false - no CI', () { platform = FakePlatform( environment: { diff --git a/packages/flutter_goldens/test/json_templates.dart b/packages/flutter_goldens/test/json_templates.dart index 1285656607..2791a23c0a 100644 --- a/packages/flutter_goldens/test/json_templates.dart +++ b/packages/flutter_goldens/test/json_templates.dart @@ -16,37 +16,6 @@ String authTemplate({ '''; } -/// Json response template for Skia Gold ignore request: -/// https://flutter-gold.skia.org/json/v1/ignores -String ignoreResponseTemplate({ - String pullRequestNumber = '0000', - String testName = 'flutter.golden_test.1', - String otherTestName = 'flutter.golden_test.1', - String expires = '2019-09-06T21:28:18.815336Z', - String otherExpires = '2019-09-06T21:28:18.815336Z', -}) { - return ''' - [ - { - "id": "7579425228619212078", - "name": "contributor@getMail.com", - "updatedBy": "contributor@getMail.com", - "expires": "$expires", - "query": "ext=png&name=$testName", - "note": "https://github.com/flutter/flutter/pull/$pullRequestNumber" - }, - { - "id": "7579425228619212078", - "name": "contributor@getMail.com", - "updatedBy": "contributor@getMail.com", - "expires": "$otherExpires", - "query": "ext=png&name=$otherTestName", - "note": "https://github.com/flutter/flutter/pull/99999" - } - ] - '''; -} - /// Json response template for Skia Gold image request: /// https://flutter-gold.skia.org/img/images/[imageHash].png List> imageResponseTemplate() { diff --git a/packages/flutter_goldens_client/lib/skia_client.dart b/packages/flutter_goldens_client/lib/skia_client.dart index dd87fc2c94..65f96d75dd 100644 --- a/packages/flutter_goldens_client/lib/skia_client.dart +++ b/packages/flutter_goldens_client/lib/skia_client.dart @@ -17,16 +17,8 @@ import 'package:process/process.dart'; const String _kFlutterRootKey = 'FLUTTER_ROOT'; 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, - none, -} - /// A client for uploading image tests and making baseline requests to the /// Flutter Gold Dashboard. class SkiaGoldClient { @@ -35,7 +27,6 @@ class SkiaGoldClient { this.fs = const LocalFileSystem(), this.process = const LocalProcessManager(), this.platform = const LocalPlatform(), - required this.ci, io.HttpClient? httpClient, }) : httpClient = httpClient ?? io.HttpClient(); @@ -58,9 +49,6 @@ class SkiaGoldClient { /// 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; @@ -82,12 +70,6 @@ class SkiaGoldClient { /// Uses the [platform] environment in this implementation. String get _goldctl => platform.environment[_kGoldctlKey]!; - /// The path to the local [Directory] where the service account key is - /// hosted. - /// - /// Uses the [platform] environment in this implementation. - String get _serviceAccount => platform.environment[_kServiceAccountKey]!; - /// Prepares the local work space for golden file testing and calls the /// goldctl `auth` command. /// @@ -101,81 +83,12 @@ class SkiaGoldClient { 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; - case ContinuousIntegrationEnvironment.none: - return; - } - - final io.ProcessResult result = await io.Process.run( - _goldctl, - authArguments, - ); - - if (result.exitCode != 0) { - final StringBuffer buf = StringBuffer() - ..writeln('Skia Gold authorization failed.') - ..writeln(failureContext) - ..writeln('Debug information for Gold:') - ..writeln('stdout: ${result.stdout}') - ..writeln('stderr: ${result.stderr}'); - throw Exception(buf.toString()); - } - } - - /// Prepares the local work space for an unauthorized client to lookup golden - /// file expectations using [imgtestCheck]. - /// - /// 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 .childDirectory('temp') .path, + '--luci', ]; final io.ProcessResult result = await io.Process.run( @@ -185,8 +98,10 @@ class SkiaGoldClient { if (result.exitCode != 0) { final StringBuffer buf = StringBuffer() - ..writeln('Skia Gold emptyAuth failed.') - ..writeln() + ..writeln('Skia Gold authorization failed.') + ..writeln('Luci environments authenticate using the file provided ' + 'by LUCI_CONTEXT. There may be an error with this file or Gold ' + 'authentication.') ..writeln('Debug information for Gold:') ..writeln('stdout: ${result.stdout}') ..writeln('stderr: ${result.stderr}'); @@ -306,10 +221,9 @@ class SkiaGoldClient { '--passfail', '--crs', 'github', '--patchset_id', commitHash, + ...getCIArguments(), ]; - imgtestInitArguments.addAll(getCIArguments()); - if (imgtestInitArguments.contains(null)) { final StringBuffer buf = StringBuffer() ..writeln('A null argument was provided for Skia Gold tryjob init.') @@ -558,10 +472,12 @@ class SkiaGoldClient { String _getKeysJSON() { final Map keys = { 'Platform' : platform.operatingSystem, - 'CI' : ci.toString().split('.').last, + 'CI' : 'luci', }; - if (platform.environment[_kTestBrowserKey] != null) + if (platform.environment[_kTestBrowserKey] != null) { keys['Browser'] = platform.environment[_kTestBrowserKey]; + keys['Platform'] = keys['Platform'] + '-browser'; + } return json.encode(keys); } @@ -590,29 +506,13 @@ class SkiaGoldClient { /// 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; - case ContinuousIntegrationEnvironment.none: - return []; - } + final String jobId = platform.environment['LOGDOG_STREAM_PREFIX']!.split('/').last; + final List refs = platform.environment['GOLD_TRYJOB']!.split('/'); + final String pullRequest = refs[refs.length - 2]; return [ '--changelist', pullRequest, - '--cis', cis, + '--cis', 'buildbucket', '--jobid', jobId, ]; } @@ -627,14 +527,12 @@ class SkiaGoldClient { /// Example TraceID for Flutter Gold: /// ',CI=cirrus,Platform=linux,name=cupertino.activityIndicator.inprogress.1.0,source_type=flutter,' String getTraceID(String testName) { - // If we are not in a CI environment, fallback on luci. return '${platform.environment[_kTestBrowserKey] == null ? ',' : ',Browser=${platform.environment[_kTestBrowserKey]},'}' - 'CI=${ci == ContinuousIntegrationEnvironment.none ? 'luci' : ci.toString().split('.').last},' + 'CI=luci,' 'Platform=${platform.operatingSystem},' 'name=$testName,' 'source_type=flutter,'; } - } /// Used to make HttpRequests during testing.