diff --git a/testing/skia_gold_client/lib/skia_gold_client.dart b/testing/skia_gold_client/lib/skia_gold_client.dart index 2543716b318ec..e9d28ff504286 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 SkiaGoldNegativeImageError, SkiaGoldProcessError; +export 'src/errors.dart' show SkiaGoldProcessError; const String _kGoldctlKey = 'GOLDCTL'; const String _kPresubmitEnvName = 'GOLD_TRYJOB'; @@ -406,46 +406,30 @@ interface class SkiaGoldClient { _stderr.writeln('stderr:\n${result.stderr}'); } } else { - // Untriaged images are not considered failures during tryjobs. + // 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(), + ); + } // ... 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 7e08a55164c87..d5590091bf46d 100644 --- a/testing/skia_gold_client/lib/src/errors.dart +++ b/testing/skia_gold_client/lib/src/errors.dart @@ -51,26 +51,3 @@ 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 e473edc2fa26f..1f91d8a35231a 100644 --- a/testing/skia_gold_client/test/skia_gold_client_test.dart +++ b/testing/skia_gold_client/test/skia_gold_client_test.dart @@ -336,41 +336,6 @@ 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 { @@ -384,7 +349,7 @@ void main() { if (command case ['python tools/goldctl.py', 'imgtest', 'init', ...]) { return io.ProcessResult(0, 0, '', ''); } - return io.ProcessResult(0, 1, 'stdout-text', 'stderr-text'); + return io.ProcessResult(1, 0, 'stdout-text', 'stderr-text'); }, ); @@ -394,12 +359,11 @@ 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('Unexpected Gold tryjobAdd failure.')); + expect(error.message, contains('Skia Gold image test failed.')); expect(error.stdout, 'stdout-text'); expect(error.stderr, 'stderr-text'); - expect(error.command.join(' '), contains('imgtest add')); + expect(error.command, contains('imgtest add')); } } finally { fixture.dispose(); @@ -514,7 +478,7 @@ void main() { if (command case ['python tools/goldctl.py', 'imgtest', 'init', ...]) { return io.ProcessResult(0, 0, '', ''); } - return io.ProcessResult(0, 1, 'stdout-text', 'stderr-text'); + return io.ProcessResult(1, 0, 'stdout-text', 'stderr-text'); }, ); @@ -524,12 +488,11 @@ 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 received an unapproved image')); + expect(error.message, contains('Skia Gold image test failed.')); expect(error.stdout, 'stdout-text'); expect(error.stderr, 'stderr-text'); - expect(error.command.join(' '), contains('imgtest add')); + expect(error.command, contains('imgtest add')); } } finally { fixture.dispose();