Fail pre-submit if a negative image is encountered as part of goldctl imgtest add. (#51685)

`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).

Author: matanlurey

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

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

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<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 {
@@ -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();