diff --git a/testing/skia_gold_client/lib/skia_gold_client.dart b/testing/skia_gold_client/lib/skia_gold_client.dart index e9d28ff504286..2543716b318ec 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..7e08a55164c87 100644 --- a/testing/skia_gold_client/lib/src/errors.dart +++ b/testing/skia_gold_client/lib/src/errors.dart @@ -51,3 +51,26 @@ 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".\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.'; +} 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();