Pre-Submit Tryjobs for Flutter Gold (#44474)
diff --git a/packages/flutter/lib/src/animation/animation_controller.dart b/packages/flutter/lib/src/animation/animation_controller.dart
index f27dea9..d2fa734 100644
--- a/packages/flutter/lib/src/animation/animation_controller.dart
+++ b/packages/flutter/lib/src/animation/animation_controller.dart
@@ -41,16 +41,17 @@
distance: 0.01,
);
-/// Configures how an [AnimationController] behaves when animations are disabled.
+/// Configures how an [AnimationController] behaves when animations are
+/// disabled.
///
/// When [AccessibilityFeatures.disableAnimations] is true, the device is asking
/// Flutter to reduce or disable animations as much as possible. To honor this,
-/// we reduce the duration and the corresponding number of frames for animations.
-/// This enum is used to allow certain [AnimationController]s to opt out of this
-/// behavior.
+/// we reduce the duration and the corresponding number of frames for
+/// animations. This enum is used to allow certain [AnimationController]s to opt
+/// out of this behavior.
///
/// For example, the [AnimationController] which controls the physics simulation
-/// for a scrollable list will have [AnimationBehavior.preserve] so that when
+/// for a scrollable list will have [AnimationBehavior.preserve], so that when
/// a user attempts to scroll it does not jump to the end/beginning too quickly.
enum AnimationBehavior {
/// The [AnimationController] will reduce its duration when
diff --git a/packages/flutter_goldens/lib/flutter_goldens.dart b/packages/flutter_goldens/lib/flutter_goldens.dart
index defb030..ceaf2f7 100644
--- a/packages/flutter_goldens/lib/flutter_goldens.dart
+++ b/packages/flutter_goldens/lib/flutter_goldens.dart
@@ -129,6 +129,11 @@
Uri getTestUri(Uri key, int version) => key;
/// Calculate the appropriate basedir for the current test context.
+ ///
+ /// The optional [suffix] argument is used by the
+ /// [FlutterSkiaGoldFileComparator] and the [FlutterPreSubmitFileComparator].
+ /// These [FlutterGoldenFileComparators] randomize their base directories to
+ /// maintain thread safety while using the `goldctl` tool.
@protected
@visibleForTesting
static Directory getBaseDirectory(LocalFileComparator defaultComparator, Platform platform, {String suffix = ''}) {
@@ -217,10 +222,7 @@
platform,
suffix: '${math.Random().nextInt(10000)}',
);
-
- if(!baseDirectory.existsSync()) {
- baseDirectory.createSync(recursive: true);
- }
+ baseDirectory.createSync(recursive: true);
goldens ??= SkiaGoldClient(baseDirectory);
await goldens.auth();
@@ -299,47 +301,23 @@
final Directory baseDirectory = FlutterGoldenFileComparator.getBaseDirectory(
defaultComparator,
platform,
+ suffix: '${math.Random().nextInt(10000)}',
);
-
- if(!baseDirectory.existsSync()) {
- baseDirectory.createSync(recursive: true);
- }
+ baseDirectory.createSync(recursive: true);
goldens ??= SkiaGoldClient(baseDirectory);
- await goldens.getExpectations();
-
+ await goldens.auth();
+ await goldens.tryjobInit();
return FlutterPreSubmitFileComparator(baseDirectory.uri, goldens);
}
@override
Future<bool> compare(Uint8List imageBytes, Uri golden) async {
golden = _addPrefix(golden);
- final String testName = skiaClient.cleanTestName(golden.path);
- final List<String> testExpectations = skiaClient.expectations[testName];
+ await update(golden, imageBytes);
+ final File goldenFile = getGoldenFile(golden);
- if (testExpectations == null) {
- // There is no baseline for this test
- return true;
- }
-
- ComparisonResult result;
- for (String expectation in testExpectations) {
- final List<int> goldenBytes = await skiaClient.getImageBytes(expectation);
-
- result = GoldenFileComparator.compareLists(
- imageBytes,
- goldenBytes,
- );
-
- if (result.passed) {
- return true;
- }
- }
-
- return skiaClient.testIsIgnoredForPullRequest(
- platform.environment['CIRRUS_PR'] ?? '',
- golden.path,
- );
+ return skiaClient.tryjobAdd(golden.path, goldenFile);
}
/// Decides based on the current environment whether goldens tests should be
diff --git a/packages/flutter_goldens/test/flutter_goldens_test.dart b/packages/flutter_goldens/test/flutter_goldens_test.dart
index 1f8c566..a120f3f 100644
--- a/packages/flutter_goldens/test/flutter_goldens_test.dart
+++ b/packages/flutter_goldens/test/flutter_goldens_test.dart
@@ -84,14 +84,12 @@
group('Request Handling', () {
String testName;
- String pullRequestNumber;
String expectation;
Uri url;
MockHttpClientRequest mockHttpRequest;
setUp(() {
testName = 'flutter.golden_test.1.png';
- pullRequestNumber = '1234';
expectation = '55109a4bed52acc780530f7a9aeff6c0';
mockHttpRequest = MockHttpClientRequest();
});
@@ -212,120 +210,6 @@
expect(masterBytes, equals(_kTestPngBytes));
});
- group('ignores', () {
- Uri url;
- MockHttpClientRequest mockHttpRequest;
- MockHttpClientResponse mockHttpResponse;
-
- setUp(() {
- url = Uri.parse('https://flutter-gold.skia.org/json/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<MockHttpClientRequest>.value(mockHttpRequest));
- when(mockHttpRequest.close())
- .thenAnswer((_) => Future<MockHttpClientResponse>.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<MockHttpClientResponse>.value(mockHttpResponse));
- final Future<bool> 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<MockHttpClientResponse>.value(mockHttpResponse));
- final Future<bool> 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<MockHttpClientResponse>.value(mockHttpResponse));
- final Future<bool> test = skiaClient.testIsIgnoredForPullRequest(
- pullRequestNumber,
- testName,
- );
- expect(
- test,
- throwsException,
- );
- });
- });
-
group('digest parsing', () {
Uri url;
MockHttpClientRequest mockHttpRequest;
@@ -507,39 +391,6 @@
});
group('Pre-Submit', () {
- FlutterPreSubmitFileComparator comparator;
- final MockSkiaGoldClient mockSkiaClient = MockSkiaGoldClient();
-
- setUp(() {
- final Directory basedir = fs.directory('flutter/test/library/')
- ..createSync(recursive: true);
- comparator = FlutterPreSubmitFileComparator(
- basedir.uri,
- mockSkiaClient,
- fs: fs,
- platform: FakePlatform(
- environment: <String, String>{
- 'FLUTTER_ROOT': _kFlutterRoot,
- 'CIRRUS_CI' : 'true',
- 'CIRRUS_PR' : '1234',
- 'GOLD_SERVICE_ACCOUNT' : 'service account...'
- },
- operatingSystem: 'macos'
- ),
- );
-
- when(mockSkiaClient.getImageBytes('55109a4bed52acc780530f7a9aeff6c0'))
- .thenAnswer((_) => Future<List<int>>.value(_kTestPngBytes));
- when(mockSkiaClient.expectations)
- .thenReturn(expectationsTemplate());
- when(mockSkiaClient.cleanTestName('library.flutter.golden_test.1.png'))
- .thenReturn('flutter.golden_test.1');
- when(mockSkiaClient.isValidDigestForExpectation(
- '55109a4bed52acc780530f7a9aeff6c0',
- 'library.flutter.golden_test.1.png',
- ))
- .thenAnswer((_) => Future<bool>.value(false));
- });
group('correctly determines testing environment', () {
test('returns true', () {
platform = FakePlatform(
@@ -601,50 +452,6 @@
);
});
});
-
- test('comparison passes test that is ignored for this PR', () async {
- when(mockSkiaClient.getImageBytes('55109a4bed52acc780530f7a9aeff6c0'))
- .thenAnswer((_) => Future<List<int>>.value(_kTestPngBytes));
- when(mockSkiaClient.testIsIgnoredForPullRequest(
- '1234',
- 'library.flutter.golden_test.1.png',
- ))
- .thenAnswer((_) => Future<bool>.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.getImageBytes('55109a4bed52acc780530f7a9aeff6c0'))
- .thenAnswer((_) => Future<List<int>>.value(_kTestPngBytes));
- when(mockSkiaClient.testIsIgnoredForPullRequest(
- '1234',
- 'library.flutter.golden_test.1.png',
- ))
- .thenAnswer((_) => Future<bool>.value(false));
- expect(
- await comparator.compare(
- Uint8List.fromList(_kFailPngBytes),
- Uri.parse('flutter.golden_test.1.png'),
- ),
- isFalse,
- );
- });
-
- test('passes non-existent baseline for new test', () async {
- expect(
- await comparator.compare(
- Uint8List.fromList(_kFailPngBytes),
- Uri.parse('flutter.new_golden_test.1.png'),
- ),
- isTrue,
- );
- });
});
group('Skipping', () {
diff --git a/packages/flutter_goldens/test/json_templates.dart b/packages/flutter_goldens/test/json_templates.dart
index 427bb4e..c74792a 100644
--- a/packages/flutter_goldens/test/json_templates.dart
+++ b/packages/flutter_goldens/test/json_templates.dart
@@ -169,37 +169,6 @@
''';
}
-/// Json response template for Skia Gold ignore request:
-/// https://flutter-gold.skia.org/json/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<List<int>> imageResponseTemplate() {
diff --git a/packages/flutter_goldens_client/lib/skia_client.dart b/packages/flutter_goldens_client/lib/skia_client.dart
index aa111bf..b89ddd1 100644
--- a/packages/flutter_goldens_client/lib/skia_client.dart
+++ b/packages/flutter_goldens_client/lib/skia_client.dart
@@ -209,6 +209,96 @@
return true;
}
+ /// 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.
+ Future<void> tryjobInit() async {
+ final File keys = workDirectory.childFile('keys.json');
+ final File failures = workDirectory.childFile('failures.json');
+
+ 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<String> imgtestInitArguments = <String>[
+ 'imgtest', 'init',
+ '--instance', 'flutter',
+ '--work-dir', workDirectory
+ .childDirectory('temp')
+ .path,
+ '--commit', commitHash,
+ '--keys-file', keys.path,
+ '--failure-file', failures.path,
+ '--passfail',
+ '--crs', 'github',
+ '--changelist', pullRequest,
+ '--cis', 'cirrus',
+ '--jobid', cirrusTaskID,
+ '--patchset_id', commitHash,
+ ];
+
+ if (imgtestInitArguments.contains(null)) {
+ final StringBuffer buf = StringBuffer()
+ ..writeln('Null argument for Skia Gold tryjobInit:');
+ imgtestInitArguments.forEach(buf.writeln);
+ throw NonZeroExitCode(1, buf.toString());
+ }
+
+ final io.ProcessResult result = await io.Process.run(
+ _goldctl,
+ imgtestInitArguments,
+ );
+
+ if (result.exitCode != 0) {
+ final StringBuffer buf = StringBuffer()
+ ..writeln('Skia Gold tryjobInit failure.')
+ ..writeln('stdout: ${result.stdout}')
+ ..writeln('stderr: ${result.stderr}');
+ throw NonZeroExitCode(1, buf.toString());
+ }
+ }
+
+ /// Executes the `imgtest add` command in the goldctl tool for tryjobs.
+ ///
+ /// The `imgtest` command collects and uploads test results to the Skia Gold
+ /// backend, the `add` argument uploads the current image test. A response is
+ /// returned from the invocation of this command that indicates a pass or fail
+ /// result for the tryjob.
+ ///
+ /// The testName and goldenFile parameters reference the current comparison
+ /// being evaluated by the [FlutterSkiaGoldFileComparator].
+ Future<bool> tryjobAdd(String testName, File goldenFile) async {
+ assert(testName != null);
+ assert(goldenFile != null);
+
+ final List<String> imgtestArguments = <String>[
+ 'imgtest', 'add',
+ '--work-dir', workDirectory
+ .childDirectory('temp')
+ .path,
+ '--test-name', cleanTestName(testName),
+ '--png-file', goldenFile.path,
+ ];
+
+ final io.ProcessResult result = await io.Process.run(
+ _goldctl,
+ imgtestArguments,
+ );
+
+ if (result.exitCode != 0) {
+ final StringBuffer buf = StringBuffer()
+ ..writeln('Skia Gold tryjobAdd failure.')
+ ..writeln('stdout: ${result.stdout}')
+ ..writeln('stderr: ${result.stderr}\n');
+ throw NonZeroExitCode(1, buf.toString());
+ }
+ return result.exitCode == 0;
+ }
+
/// Requests and sets the [_expectations] known to Flutter Gold at head.
Future<void> getExpectations() async {
_expectations = <String, List<String>>{};
@@ -262,69 +352,6 @@
return imageBytes;
}
- /// 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]
- /// 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<bool> testIsIgnoredForPullRequest(String pullRequest, String testName) async {
- bool ignoreIsActive = false;
- testName = cleanTestName(testName);
- String rawResponse;
- await io.HttpOverrides.runWithHttpOverrides<Future<void>>(() async {
- final Uri requestForIgnores = Uri.parse(
- 'https://flutter-gold.skia.org/json/ignores'
- );
-
- try {
- final io.HttpClientRequest request = await httpClient.getUrl(requestForIgnores);
- final io.HttpClientResponse response = await request.close();
- rawResponse = await utf8.decodeStream(response);
- final List<dynamic> ignores = json.decode(rawResponse) as List<dynamic>;
- for(dynamic ignore in ignores) {
- final List<String> ignoredQueries = (ignore['query'] as String).split('&');
- final String ignoredPullRequest = (ignore['note'] as String).split('/').last;
- final DateTime expiration = DateTime.parse(ignore['expires'] as String);
- // 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(_) {
- 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(),
- );
- return ignoreIsActive;
- }
-
/// The [_expectations] retrieved from Flutter Gold do not include the
/// parameters of the given test. This function queries the Flutter Gold
/// details api to determine if the given expectation for a test matches the
