From d891112af2150e7808f5a6260b89456fc92742fd Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 26 Mar 2024 12:40:35 -0700 Subject: [PATCH 1/5] Fail pre-submit during a negative image digest. --- .../lib/skia_gold_client.dart | 48 ++++++++++++------ testing/skia_gold_client/lib/src/errors.dart | 19 +++++++ .../test/skia_gold_client_test.dart | 49 ++++++++++++++++--- 3 files changed, 94 insertions(+), 22 deletions(-) diff --git a/testing/skia_gold_client/lib/skia_gold_client.dart b/testing/skia_gold_client/lib/skia_gold_client.dart index e9d28ff504286..add3593578557 100644 --- a/testing/skia_gold_client/lib/skia_gold_client.dart +++ b/testing/skia_gold_client/lib/skia_gold_client.dart @@ -13,7 +13,7 @@ import 'package:process/process.dart'; import 'src/errors.dart'; -export 'src/errors.dart' show SkiaGoldProcessError; +export 'src/errors.dart' show SkiaGoldNegativeImageError, SkiaGoldProcessError; const String _kGoldctlKey = 'GOLDCTL'; const String _kPresubmitEnvName = 'GOLD_TRYJOB'; @@ -406,30 +406,46 @@ interface class SkiaGoldClient { _stderr.writeln('stderr:\n${result.stderr}'); } } else { - // Neither of these conditions are considered failures during tryjobs. - final bool isUntriaged = resultStdout.contains('Untriaged'); - final bool isNegative = resultStdout.contains('negative image'); - if (!isUntriaged && !isNegative) { - final StringBuffer buf = StringBuffer() - ..writeln('Unexpected Gold tryjobAdd failure.') - ..writeln('Tryjob execution for golden file test $testName failed for') - ..writeln('a reason unrelated to pixel comparison.'); - throw SkiaGoldProcessError( - command: tryjobCommand, - stdout: resultStdout, - stderr: result.stderr.toString(), - message: buf.toString(), - ); - } + // Untriaged images are not considered failures during tryjobs. // ... but we want to know about them anyway. // See https://github.com/flutter/flutter/issues/145219. // TODO(matanlurey): Update the documentation to reflect the new behavior. + final bool isUntriaged = resultStdout.contains('Untriaged'); if (isUntriaged) { _stderr ..writeln('NOTE: Untriaged image detected in tryjob.') ..writeln('Triage should be required by the "Flutter Gold" check') ..writeln('stdout:\n$resultStdout'); + return; + } + + // Negative images are considered failures during tryjobs. + // + // We don't actually use negative images as part of our workflow, but + // if they *are* used (i.e. someone accidentally marks a digest a + // negative image), we want to fail the tryjob - otherwise we just end up + // with a negative image in the tree (a post-submit failure). + final bool isNegative = resultStdout.contains('negative image'); + if (isNegative) { + throw SkiaGoldNegativeImageError( + testName: testName, + command: tryjobCommand, + stdout: resultStdout, + stderr: result.stderr.toString(), + ); } + + // If the tryjob failed for some other reason, throw an error. + final StringBuffer buf = StringBuffer() + ..writeln('Unexpected Gold tryjobAdd failure.') + ..writeln('Tryjob execution for golden file test $testName failed for') + ..writeln('a reason unrelated to pixel comparison.'); + throw SkiaGoldProcessError( + command: tryjobCommand, + stdout: resultStdout, + stderr: result.stderr.toString(), + message: buf.toString(), + ); } } diff --git a/testing/skia_gold_client/lib/src/errors.dart b/testing/skia_gold_client/lib/src/errors.dart index d5590091bf46d..d96dc5643a774 100644 --- a/testing/skia_gold_client/lib/src/errors.dart +++ b/testing/skia_gold_client/lib/src/errors.dart @@ -51,3 +51,22 @@ final class SkiaGoldProcessError extends Error { ].join('\n'); } } + +/// Error thrown when the Skia Gold process exits due to a negative image. +final class SkiaGoldNegativeImageError extends SkiaGoldProcessError { + /// Creates a new [SkiaGoldNegativeImageError] from the provided origin. + /// + /// See [SkiaGoldProcessError.new] for more information. + SkiaGoldNegativeImageError({ + required this.testName, + required super.command, + required super.stdout, + required super.stderr, + }); + + /// Name of the test that produced the negative image. + final String testName; + + @override + String get message => 'Negative image detected for test: "$testName"'; +} diff --git a/testing/skia_gold_client/test/skia_gold_client_test.dart b/testing/skia_gold_client/test/skia_gold_client_test.dart index 1f91d8a35231a..e473edc2fa26f 100644 --- a/testing/skia_gold_client/test/skia_gold_client_test.dart +++ b/testing/skia_gold_client/test/skia_gold_client_test.dart @@ -336,6 +336,41 @@ void main() { } }); + test('addImg [pre-submit] fails due to a negative image', () async { + final _TestFixture fixture = _TestFixture(); + try { + final SkiaGoldClient client = createClient( + fixture, + environment: presubmitEnv, + onRun: (List command) { + if (command case ['git', ...]) { + return io.ProcessResult(0, 0, mockCommitHash, ''); + } + if (command case ['python tools/goldctl.py', 'imgtest', 'init', ...]) { + return io.ProcessResult(0, 0, '', ''); + } + return io.ProcessResult(0, 1, 'negative image', 'stderr'); + }, + ); + + try { + await client.addImg( + 'test-name.foo', + io.File(p.join(fixture.workDirectory.path, 'temp', 'golden.png')), + screenshotSize: 1000, + ); + fail('addImg should fail due to a negative image'); + } on SkiaGoldNegativeImageError catch (error) { + expect(error.testName, 'test-name.foo'); + expect(error.stdout, 'negative image'); + expect(error.stderr, 'stderr'); + expect(error.command.join(' '), contains('imgtest add')); + } + } finally { + fixture.dispose(); + } + }); + test('addImg [pre-submit] fails due to an unexpected error', () async { final _TestFixture fixture = _TestFixture(); try { @@ -349,7 +384,7 @@ void main() { if (command case ['python tools/goldctl.py', 'imgtest', 'init', ...]) { return io.ProcessResult(0, 0, '', ''); } - return io.ProcessResult(1, 0, 'stdout-text', 'stderr-text'); + return io.ProcessResult(0, 1, 'stdout-text', 'stderr-text'); }, ); @@ -359,11 +394,12 @@ void main() { io.File(p.join(fixture.workDirectory.path, 'temp', 'golden.png')), screenshotSize: 1000, ); + fail('addImg should fail due to an unexpected error'); } on SkiaGoldProcessError catch (error) { - expect(error.message, contains('Skia Gold image test failed.')); + expect(error.message, contains('Unexpected Gold tryjobAdd failure.')); expect(error.stdout, 'stdout-text'); expect(error.stderr, 'stderr-text'); - expect(error.command, contains('imgtest add')); + expect(error.command.join(' '), contains('imgtest add')); } } finally { fixture.dispose(); @@ -478,7 +514,7 @@ void main() { if (command case ['python tools/goldctl.py', 'imgtest', 'init', ...]) { return io.ProcessResult(0, 0, '', ''); } - return io.ProcessResult(1, 0, 'stdout-text', 'stderr-text'); + return io.ProcessResult(0, 1, 'stdout-text', 'stderr-text'); }, ); @@ -488,11 +524,12 @@ void main() { io.File(p.join(fixture.workDirectory.path, 'temp', 'golden.png')), screenshotSize: 1000, ); + fail('addImg should fail due to an unapproved image'); } on SkiaGoldProcessError catch (error) { - expect(error.message, contains('Skia Gold image test failed.')); + expect(error.message, contains('Skia Gold received an unapproved image')); expect(error.stdout, 'stdout-text'); expect(error.stderr, 'stderr-text'); - expect(error.command, contains('imgtest add')); + expect(error.command.join(' '), contains('imgtest add')); } } finally { fixture.dispose(); From 448ac658eab8d1d049b89c858e694c45ad07b60c Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 26 Mar 2024 12:42:15 -0700 Subject: [PATCH 2/5] Add more to the message. --- testing/skia_gold_client/lib/src/errors.dart | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/testing/skia_gold_client/lib/src/errors.dart b/testing/skia_gold_client/lib/src/errors.dart index d96dc5643a774..7e08a55164c87 100644 --- a/testing/skia_gold_client/lib/src/errors.dart +++ b/testing/skia_gold_client/lib/src/errors.dart @@ -55,7 +55,7 @@ final class SkiaGoldProcessError extends Error { /// Error thrown when the Skia Gold process exits due to a negative image. final class SkiaGoldNegativeImageError extends SkiaGoldProcessError { /// Creates a new [SkiaGoldNegativeImageError] from the provided origin. - /// + /// /// See [SkiaGoldProcessError.new] for more information. SkiaGoldNegativeImageError({ required this.testName, @@ -68,5 +68,9 @@ final class SkiaGoldNegativeImageError extends SkiaGoldProcessError { final String testName; @override - String get message => 'Negative image detected for test: "$testName"'; + String get message => 'Negative image detected for test: "$testName".\n\n' + 'The flutter/engine workflow should never produce negative images; it is ' + 'possible that someone accidentally (or without knowing our policy) ' + 'marked a test as negative.\n\n' + 'See https://github.com/flutter/flutter/issues/145043 for details.'; } From 9880f80a5ee9aba7196b78baea6f19aaf8186246 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 26 Mar 2024 12:42:51 -0700 Subject: [PATCH 3/5] Format. --- testing/skia_gold_client/lib/skia_gold_client.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testing/skia_gold_client/lib/skia_gold_client.dart b/testing/skia_gold_client/lib/skia_gold_client.dart index add3593578557..2543716b318ec 100644 --- a/testing/skia_gold_client/lib/skia_gold_client.dart +++ b/testing/skia_gold_client/lib/skia_gold_client.dart @@ -418,7 +418,7 @@ interface class SkiaGoldClient { ..writeln('stdout:\n$resultStdout'); return; } - + // Negative images are considered failures during tryjobs. // // We don't actually use negative images as part of our workflow, but From 94b82a4551870d1801b47fe8dbd61fc563c48106 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 26 Mar 2024 13:02:09 -0700 Subject: [PATCH 4/5] Update testing/skia_gold_client/test/skia_gold_client_test.dart Co-authored-by: gaaclarke <30870216+gaaclarke@users.noreply.github.com> --- testing/skia_gold_client/test/skia_gold_client_test.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testing/skia_gold_client/test/skia_gold_client_test.dart b/testing/skia_gold_client/test/skia_gold_client_test.dart index e473edc2fa26f..87f1ce87e995d 100644 --- a/testing/skia_gold_client/test/skia_gold_client_test.dart +++ b/testing/skia_gold_client/test/skia_gold_client_test.dart @@ -526,7 +526,7 @@ void main() { ); fail('addImg should fail due to an unapproved image'); } on SkiaGoldProcessError catch (error) { - expect(error.message, contains('Skia Gold received an unapproved image')); + expect(error.message, contains('Skia Gold received an unapproved image.')); expect(error.stdout, 'stdout-text'); expect(error.stderr, 'stderr-text'); expect(error.command.join(' '), contains('imgtest add')); From 4ed1878f760e797cde89df2a5421d2a965be3e10 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 26 Mar 2024 13:44:38 -0700 Subject: [PATCH 5/5] ++ --- testing/skia_gold_client/test/skia_gold_client_test.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testing/skia_gold_client/test/skia_gold_client_test.dart b/testing/skia_gold_client/test/skia_gold_client_test.dart index 87f1ce87e995d..e473edc2fa26f 100644 --- a/testing/skia_gold_client/test/skia_gold_client_test.dart +++ b/testing/skia_gold_client/test/skia_gold_client_test.dart @@ -526,7 +526,7 @@ void main() { ); fail('addImg should fail due to an unapproved image'); } on SkiaGoldProcessError catch (error) { - expect(error.message, contains('Skia Gold received an unapproved image.')); + expect(error.message, contains('Skia Gold received an unapproved image')); expect(error.stdout, 'stdout-text'); expect(error.stderr, 'stderr-text'); expect(error.command.join(' '), contains('imgtest add'));