From 26d09f1ab454dc8042386c4fbca7e30942da5d8a Mon Sep 17 00:00:00 2001 From: Kate Lovett Date: Thu, 31 Oct 2019 11:44:07 -0700 Subject: [PATCH] Gold Performance improvements (#43748) --- .../flutter_goldens/lib/flutter_goldens.dart | 143 ++++---- .../test/flutter_goldens_test.dart | 307 +++++++++++++++--- .../flutter_goldens/test/json_templates.dart | 13 +- .../lib/skia_client.dart | 56 +++- 4 files changed, 387 insertions(+), 132 deletions(-) diff --git a/packages/flutter_goldens/lib/flutter_goldens.dart b/packages/flutter_goldens/lib/flutter_goldens.dart index e0c05276f45..3b7463a8826 100644 --- a/packages/flutter_goldens/lib/flutter_goldens.dart +++ b/packages/flutter_goldens/lib/flutter_goldens.dart @@ -233,14 +233,12 @@ class FlutterSkiaGoldFileComparator extends FlutterGoldenFileComparator { /// Decides based on the current environment whether goldens tests should be /// performed against Skia Gold. static bool isAvailableForEnvironment(Platform platform) { - final String cirrusCI = platform.environment['CIRRUS_CI'] ?? ''; final String cirrusPR = platform.environment['CIRRUS_PR'] ?? ''; final String cirrusBranch = platform.environment['CIRRUS_BRANCH'] ?? ''; - final String goldServiceAccount = platform.environment['GOLD_SERVICE_ACCOUNT'] ?? ''; - return cirrusCI.isNotEmpty + return platform.environment.containsKey('CIRRUS_CI') && cirrusPR.isEmpty && cirrusBranch == 'master' - && goldServiceAccount.isNotEmpty; + && platform.environment.containsKey('GOLD_SERVICE_ACCOUNT'); } } @@ -340,9 +338,69 @@ class FlutterPreSubmitFileComparator extends FlutterGoldenFileComparator { /// 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 cirrusCI = platform.environment['CIRRUS_CI'] ?? ''; final String cirrusPR = platform.environment['CIRRUS_PR'] ?? ''; - return cirrusCI.isNotEmpty && cirrusPR.isNotEmpty; + return platform.environment.containsKey('CIRRUS_CI') + && cirrusPR.isNotEmpty + && platform.environment.containsKey('GOLD_SERVICE_ACCOUNT'); + } +} + +/// A [FlutterGoldenFileComparator] for controlling post-submit testing +/// conditions that do not execute golden file tests. +/// +/// Currently, this comparator is used in post-submit checks on LUCI and with +/// some Cirrus shards that do not run framework tests. +/// +/// See also: +/// +/// * [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 +/// merged into the master branch. +/// * [FlutterLocalFileComparator], another +/// [FlutterGoldenFileComparator] that tests golden images locally on your +/// current machine. +class FlutterSkippingGoldenFileComparator extends FlutterGoldenFileComparator { + /// Creates a [FlutterSkippingGoldenFileComparator] that will skip tests that + /// are not in the right environment for golden file testing. + FlutterSkippingGoldenFileComparator( + final Uri basedir, + final SkiaGoldClient skiaClient, + ) : super(basedir, skiaClient); + + /// Creates a new [FlutterSkippingGoldenFileComparator] that mirrors the + /// relative path resolution of the default [goldenFileComparator]. + static FlutterSkippingGoldenFileComparator fromDefaultComparator({ + LocalFileComparator defaultComparator, + }) { + defaultComparator ??= goldenFileComparator; + const FileSystem fs = LocalFileSystem(); + final Uri basedir = defaultComparator.basedir; + final SkiaGoldClient skiaClient = SkiaGoldClient(fs.directory(basedir)); + return FlutterSkippingGoldenFileComparator(basedir, skiaClient); + } + + @override + Future compare(Uint8List imageBytes, Uri golden) async { + print( + 'Skipping "$golden" test : Golden file testing is unavailable on LUCI and ' + 'some Cirrus shards.' + ); + return true; + } + + @override + Future update(Uri golden, Uint8List imageBytes) => null; + + /// Decides based on the current environment whether this comparator should be + /// used. + static bool isAvailableForEnvironment(Platform platform) { + return platform.environment.containsKey('SWARMING_TASK_ID') + || platform.environment.containsKey('CIRRUS_CI'); } } @@ -350,11 +408,14 @@ class FlutterPreSubmitFileComparator extends FlutterGoldenFileComparator { /// current machine. /// /// This comparator utilizes the [SkiaGoldClient] to request baseline images for -/// the given device under test for comparison. This comparator is only -/// initialized when running tests locally, and is intended to serve as a smoke -/// test during development. As such, it will not be able to detect unintended -/// changes on other machines until it they are tested using the -/// [FlutterPreSubmitFileComparator]. +/// the given device under test for comparison. This comparator is initialized +/// when conditions for all other [FlutterGoldenFileComparators] have not been +/// met, see the `isAvailableForEnvironment` method for each one listed below. +/// +/// The [FlutterLocalFileComparator] is intended to run on local machines and +/// serve as a smoke test during development. As such, it will not be able to +/// detect unintended changes on environments other than the currently executing +/// machine, until they are tested using the [FlutterPreSubmitFileComparator]. /// /// See also: /// @@ -366,6 +427,9 @@ class FlutterPreSubmitFileComparator extends FlutterGoldenFileComparator { /// * [FlutterPreSubmitFileComparator], another /// [FlutterGoldenFileComparator] that tests golden images before changes are /// merged into the master branch. +/// * [FlutterSkippingGoldenFileComparator], another +/// [FlutterGoldenFileComparator] that controls post-submit testing +/// conditions that do not execute golden file tests. class FlutterLocalFileComparator extends FlutterGoldenFileComparator with LocalComparisonOutput { /// Creates a [FlutterLocalFileComparator] that will test golden file /// images against baselines requested from Flutter Gold. @@ -449,60 +513,3 @@ class FlutterLocalFileComparator extends FlutterGoldenFileComparator with LocalC return false; } } - -/// A [FlutterGoldenFileComparator] for skipping golden image tests when the -/// current environment is not supported. This comparator is used in post-submit -/// checks on LUCI. -/// -/// See also: -/// -/// * [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 -/// merged into the master branch. -/// * [FlutterLocalFileComparator], another -/// [FlutterGoldenFileComparator] that tests golden images locally on your -/// current machine. -class FlutterSkippingGoldenFileComparator extends FlutterGoldenFileComparator { - /// Creates a [FlutterSkippingGoldenFileComparator] that will skip tests that - /// are not in the right environment for golden file testing. - FlutterSkippingGoldenFileComparator( - final Uri basedir, - final SkiaGoldClient skiaClient, - ) : super(basedir, skiaClient); - - /// Creates a new [FlutterSkippingGoldenFileComparator] that mirrors the - /// relative path resolution of the default [goldenFileComparator]. - static FlutterSkippingGoldenFileComparator fromDefaultComparator({ - LocalFileComparator defaultComparator, - }) { - defaultComparator ??= goldenFileComparator; - const FileSystem fs = LocalFileSystem(); - final Uri basedir = defaultComparator.basedir; - final SkiaGoldClient skiaClient = SkiaGoldClient(fs.directory(basedir)); - return FlutterSkippingGoldenFileComparator(basedir, skiaClient); - } - - @override - Future compare(Uint8List imageBytes, Uri golden) async { - print( - 'Skipping "$golden" test : Golden file testing is unavailble in LUCI' - 'environment.' - ); - return true; - } - - @override - Future update(Uri golden, Uint8List imageBytes) => null; - - /// Decides based on the current environment whether goldens tests should be - /// skipped. - static bool isAvailableForEnvironment(Platform platform) { - final String luci = platform.environment['SWARMING_TASK_ID'] ?? ''; - return luci.isNotEmpty; - } -} diff --git a/packages/flutter_goldens/test/flutter_goldens_test.dart b/packages/flutter_goldens/test/flutter_goldens_test.dart index 898aa11f984..ceec993712f 100644 --- a/packages/flutter_goldens/test/flutter_goldens_test.dart +++ b/packages/flutter_goldens/test/flutter_goldens_test.dart @@ -221,7 +221,13 @@ void main() { url = Uri.parse('https://flutter-gold.skia.org/json/ignores'); mockHttpRequest = MockHttpClientRequest(); mockHttpResponse = MockHttpClientResponse(utf8.encode( - ignoreResponseTemplate(pullRequestNumber: pullRequestNumber) + ignoreResponseTemplate( + pullRequestNumber: pullRequestNumber, + expires: DateTime.now() + .add(const Duration(days: 1)) + .toString(), + otherTestName: 'unrelatedTest.1' + ) )); when(mockHttpClient.getUrl(url)) .thenAnswer((_) => Future.value(mockHttpRequest)); @@ -239,17 +245,17 @@ void main() { ); }); - test('returns false for not ignored test and ignored pull request number', () async { + test('returns true for ignored test and not ignored pull request number', () async { expect( await skiaClient.testIsIgnoredForPullRequest( '5678', testName, ), - isFalse, + isTrue, ); }); - test('returns false for ignored test and not ignored pull request number', () async { + test('returns false for not ignored test and ignored pull request number', () async { expect( await skiaClient.testIsIgnoredForPullRequest( pullRequestNumber, @@ -258,6 +264,66 @@ void main() { 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, + ); + }); }); group('digest parsing', () { @@ -355,21 +421,88 @@ void main() { ); }); - test('correctly determines testing environment', () { - platform = FakePlatform( - environment: { - 'FLUTTER_ROOT': _kFlutterRoot, - 'CIRRUS_CI' : 'true', - 'CIRRUS_PR' : '', - 'CIRRUS_BRANCH' : 'master', - 'GOLD_SERVICE_ACCOUNT' : 'service account...', - }, - operatingSystem: 'macos' - ); - expect( - FlutterSkiaGoldFileComparator.isAvailableForEnvironment(platform), - isTrue, - ); + group('correctly determines testing environment', () { + test('returns true', () { + platform = FakePlatform( + environment: { + 'FLUTTER_ROOT': _kFlutterRoot, + 'CIRRUS_CI': 'true', + 'CIRRUS_PR': '', + 'CIRRUS_BRANCH': 'master', + 'GOLD_SERVICE_ACCOUNT': 'service account...', + }, + operatingSystem: 'macos' + ); + expect( + FlutterSkiaGoldFileComparator.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...', + }, + operatingSystem: 'macos' + ); + expect( + FlutterSkiaGoldFileComparator.isAvailableForEnvironment(platform), + isFalse, + ); + }); + + test('returns false - no service account', () { + platform = FakePlatform( + environment: { + 'FLUTTER_ROOT': _kFlutterRoot, + 'CIRRUS_CI': 'true', + 'CIRRUS_PR': '', + 'CIRRUS_BRANCH': 'master', + }, + operatingSystem: 'macos' + ); + expect( + FlutterSkiaGoldFileComparator.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( + FlutterSkiaGoldFileComparator.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' + ); + expect( + FlutterSkiaGoldFileComparator.isAvailableForEnvironment(platform), + isFalse, + ); + }); }); }); @@ -389,6 +522,7 @@ void main() { 'FLUTTER_ROOT': _kFlutterRoot, 'CIRRUS_CI' : 'true', 'CIRRUS_PR' : '1234', + 'GOLD_SERVICE_ACCOUNT' : 'service account...' }, operatingSystem: 'macos' ), @@ -406,20 +540,66 @@ void main() { )) .thenAnswer((_) => Future.value(false)); }); + group('correctly determines testing environment', () { + test('returns true', () { + 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('correctly determines testing environment', () { - platform = FakePlatform( - environment: { - 'FLUTTER_ROOT': _kFlutterRoot, - 'CIRRUS_CI' : 'true', - 'CIRRUS_PR' : '1234', - }, - operatingSystem: 'macos' - ); - expect( - FlutterPreSubmitFileComparator.isAvailableForEnvironment(platform), - isTrue, - ); + test('returns false - no PR', () { + platform = FakePlatform( + environment: { + 'FLUTTER_ROOT': _kFlutterRoot, + 'CIRRUS_CI': 'true', + 'CIRRUS_PR': '', + 'GOLD_SERVICE_ACCOUNT' : 'service account...', + }, + operatingSystem: 'macos' + ); + expect( + FlutterPreSubmitFileComparator.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', () { + platform = FakePlatform( + environment: { + 'FLUTTER_ROOT': _kFlutterRoot, + }, + operatingSystem: 'macos' + ); + expect( + FlutterPreSubmitFileComparator.isAvailableForEnvironment(platform), + isFalse, + ); + }); }); test('comparison passes test that is ignored for this PR', () async { @@ -439,7 +619,7 @@ void main() { ); }); - test('fails test that is not ignored for this PR', () async { + test('fails test that is not ignored', () async { when(mockSkiaClient.getImageBytes('55109a4bed52acc780530f7a9aeff6c0')) .thenAnswer((_) => Future>.value(_kTestPngBytes)); when(mockSkiaClient.testIsIgnoredForPullRequest( @@ -467,6 +647,51 @@ void main() { }); }); + group('Skipping', () { + group('correctly determines testing environment', () { + 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, + 'CIRRUS_CI' : 'yep', + }, + operatingSystem: 'macos' + ); + expect( + FlutterSkippingGoldenFileComparator.isAvailableForEnvironment(platform), + isTrue, + ); + }); + test('returns false', () { + platform = FakePlatform( + environment: { + 'FLUTTER_ROOT': _kFlutterRoot, + }, + operatingSystem: 'macos' + ); + expect( + FlutterSkippingGoldenFileComparator.isAvailableForEnvironment( + platform), + isFalse, + ); + }); + }); + }); + group('Local', () { FlutterLocalFileComparator comparator; final MockSkiaGoldClient mockSkiaClient = MockSkiaGoldClient(); @@ -517,22 +742,6 @@ void main() { ); }); }); - - group('Skipping', () { - test('correctly determines testing environment', () { - platform = FakePlatform( - environment: { - 'FLUTTER_ROOT': _kFlutterRoot, - 'SWARMING_TASK_ID' : '1234567890', - }, - operatingSystem: 'macos' - ); - expect( - FlutterSkippingGoldenFileComparator.isAvailableForEnvironment(platform), - isTrue, - ); - }); - }); }); } diff --git a/packages/flutter_goldens/test/json_templates.dart b/packages/flutter_goldens/test/json_templates.dart index f33a398f6f5..70725276267 100644 --- a/packages/flutter_goldens/test/json_templates.dart +++ b/packages/flutter_goldens/test/json_templates.dart @@ -174,6 +174,9 @@ String digestResponseTemplate({ 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 ''' [ @@ -181,9 +184,17 @@ String ignoreResponseTemplate({ "id": "7579425228619212078", "name": "contributor@getMail.com", "updatedBy": "contributor@getMail.com", - "expires": "2019-09-06T21:28:18.815336Z", + "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" } ] '''; diff --git a/packages/flutter_goldens_client/lib/skia_client.dart b/packages/flutter_goldens_client/lib/skia_client.dart index 4a78f252efe..38ff6f79569 100644 --- a/packages/flutter_goldens_client/lib/skia_client.dart +++ b/packages/flutter_goldens_client/lib/skia_client.dart @@ -249,10 +249,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 [FlutterPreSubmitFileComparator]. - /// In order to land a change to an exiting golden file, an ignore must be set - /// up in Flutter Gold. This will serve as a flag to permit the change to - /// land, and protect against any unwanted changes. + /// 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); @@ -270,16 +271,37 @@ class SkiaGoldClient { for(Map ignore in ignores) { final List ignoredQueries = ignore['query'].split('&'); final String ignoredPullRequest = ignore['note'].split('/').last; - if (ignoredQueries.contains('name=$testName') && - ignoredPullRequest == pullRequest) { - ignoreIsActive = true; - break; + final DateTime expiration = DateTime.parse(ignore['expires']); + // The currently failing test is in the process of modification. + if (ignoredQueries.contains('name=$testName')) { + if (expiration.isAfter(DateTime.now())) { + ignoreIsActive = true; + } else { + // If any ignore is expired for the given test, throw with + // guidance. + final StringBuffer buf = StringBuffer() + ..writeln('This test has an expired ignore in place, and the') + ..writeln('change has not been triaged.') + ..writeln('The associated pull request is:') + ..writeln('https://github.com/flutter/flutter/pull/$ignoredPullRequest'); + throw NonZeroExitCode(1, buf.toString()); + } } } } on FormatException catch(_) { - print('Formatting error detected requesting ignores from Flutter Gold.\n' - 'rawResponse: $rawResponse'); - rethrow; + if (rawResponse.contains('stream timeout')) { + final StringBuffer buf = StringBuffer() + ..writeln('Stream timeout on /ignores api.') + ..writeln('This may be caused by a failure to triage a change.') + ..writeln('Check https://flutter-gold.skia.org/ignores, or') + ..writeln('https://flutter-gold.skia.org/?query=source_type%3Dflutter') + ..writeln('for untriaged golden files.'); + throw NonZeroExitCode(1, buf.toString()); + } else { + print('Formatting error detected requesting /ignores from Flutter Gold.' + '\nrawResponse: $rawResponse'); + rethrow; + } } }, SkiaGoldHttpOverrides(), @@ -309,9 +331,15 @@ class SkiaGoldClient { isValid = digest.isValid(platform, testName, expectation); } on FormatException catch(_) { - print('Formatting error detected requesting digest from Flutter Gold.\n' - 'rawResponse: $rawResponse'); - rethrow; + if (rawResponse.contains('stream timeout')) { + final StringBuffer buf = StringBuffer() + ..writeln('Stream timeout on /details api.'); + throw NonZeroExitCode(1, buf.toString()); + } else { + print('Formatting error detected requesting /ignores from Flutter Gold.' + '\nrawResponse: $rawResponse'); + rethrow; + } } }, SkiaGoldHttpOverrides(),