Reverts "Fail pre-submit if a negative image is encountered as part o…

…f `goldctl imgtest add`. (#51685)" (#51718)

Reverts: #51685
Initiated by: matanlurey
Reason for reverting: goldctl does not disambiguate negatives from untriaged images (see https://github.com/google/skia-buildbot/blob/9b9adad0805e6da96c4939cbd3d3855ab59998ee/gold-client/cmd/goldctl/cmd_imgtest_test.go#L325).
Original PR Author: matanlurey

Reviewed By: {mdebbar, gaaclarke}

This change reverts the following previous change:
`flutter/engine`-side fix for flutter/flutter#145043.

- Before this PR, if a negative image was encountered, we'd silently pass pre-submit, merge, and turn the tree red.
- After this PR, a negative image both makes pre and post-submit red.

Added tests, and fixed up some unrelated tests that were accidentally setting `pid` instead of `exitCode`. Oops!

/cc @zanderso and @eyebrowsoffire (current engine sheriff).

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(),
-      );
     }
   }
 

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.';
-}

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<String> 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();