Reduce code-duplication a bit and add more error context across SkiaGoldClient. (#51426)

- Replaced manual `StringBuffer()..writeln('stdout: ...')` with a single
`SkiaGoldProcessError` constructor.
- Updated tests to make sure it's working.

_/cc @dnfield @jonahwilliams FYI only._

Author: matanlurey

testing/skia_gold_client/lib/skia_gold_client.dart

@@ -11,6 +11,10 @@ import 'package:meta/meta.dart';
 import 'package:path/path.dart' as path;
 import 'package:process/process.dart';
 
+import 'src/errors.dart';
+
+export 'src/errors.dart' show SkiaGoldProcessError;
+
 const String _kGoldctlKey = 'GOLDCTL';
 const String _kPresubmitEnvName = 'GOLD_TRYJOB';
 const String _kLuciEnvName = 'LUCI_CONTEXT';
@@ -154,11 +158,13 @@ interface class SkiaGoldClient {
         ..writeln('Skia Gold authorization failed.')
         ..writeln('Luci environments authenticate using the file provided '
           'by LUCI_CONTEXT. There may be an error with this file or Gold '
-          'authentication.')
-        ..writeln('Debug information for Gold:')
-        ..writeln('stdout: ${result.stdout}')
-        ..writeln('stderr: ${result.stderr}');
-      throw Exception(buf.toString());
+          'authentication.');
+      throw SkiaGoldProcessError(
+        command: authCommand,
+        stdout: result.stdout.toString(),
+        stderr: result.stderr.toString(),
+        message: buf.toString(),
+      );
     } else if (verbose) {
       _stderr.writeln('stdout:\n${result.stdout}');
       _stderr.writeln('stderr:\n${result.stderr}');
@@ -193,27 +199,19 @@ interface class SkiaGoldClient {
       '--passfail',
     ];
 
-    if (imgtestInitCommand.contains(null)) {
-      final StringBuffer buf = StringBuffer()
-        ..writeln('A null argument was provided for Skia Gold imgtest init.')
-        ..writeln('Please confirm the settings of your golden file test.')
-        ..writeln('Arguments provided:');
-      imgtestInitCommand.forEach(buf.writeln);
-      throw Exception(buf.toString());
-    }
-
     final io.ProcessResult result = await _runCommand(imgtestInitCommand);
 
     if (result.exitCode != 0) {
       final StringBuffer buf = StringBuffer()
         ..writeln('Skia Gold imgtest init failed.')
         ..writeln('An error occurred when initializing golden file test with ')
-        ..writeln('goldctl.')
-        ..writeln()
-        ..writeln('Debug information for Gold:')
-        ..writeln('stdout: ${result.stdout}')
-        ..writeln('stderr: ${result.stderr}');
-      throw Exception(buf.toString());
+        ..writeln('goldctl.');
+      throw SkiaGoldProcessError(
+        command: imgtestInitCommand,
+        stdout: result.stdout.toString(),
+        stderr: result.stderr.toString(),
+        message: buf.toString(),
+      );
     } else if (verbose) {
       _stderr.writeln('stdout:\n${result.stdout}');
       _stderr.writeln('stderr:\n${result.stderr}');
@@ -308,12 +306,13 @@ interface class SkiaGoldClient {
         ..writeln('Visit https://flutter-engine-gold.skia.org/ to view and approve ')
         ..writeln('the image(s), or revert the associated change. For more ')
         ..writeln('information, visit the wiki: ')
-        ..writeln('https://github.com/flutter/flutter/wiki/Writing-a-golden-file-test-for-package:flutter')
-        ..writeln()
-        ..writeln('Debug information for Gold --------------------------------')
-        ..writeln('stdout: ${result.stdout}')
-        ..writeln('stderr: ${result.stderr}');
-      throw Exception(buf.toString());
+        ..writeln('https://github.com/flutter/flutter/wiki/Writing-a-golden-file-test-for-package:flutter');
+      throw SkiaGoldProcessError(
+        command: imgtestCommand,
+        stdout: result.stdout.toString(),
+        stderr: result.stderr.toString(),
+        message: buf.toString(),
+      );
     } else if (verbose) {
       _stderr.writeln('stdout:\n${result.stdout}');
       _stderr.writeln('stderr:\n${result.stderr}');
@@ -347,27 +346,19 @@ interface class SkiaGoldClient {
       ..._getCIArguments(),
     ];
 
-    if (tryjobInitCommand.contains(null)) {
-      final StringBuffer buf = StringBuffer()
-        ..writeln('A null argument was provided for Skia Gold tryjob init.')
-        ..writeln('Please confirm the settings of your golden file test.')
-        ..writeln('Arguments provided:');
-      tryjobInitCommand.forEach(buf.writeln);
-      throw Exception(buf.toString());
-    }
-
     final io.ProcessResult result = await _runCommand(tryjobInitCommand);
 
     if (result.exitCode != 0) {
       final StringBuffer buf = StringBuffer()
         ..writeln('Skia Gold tryjobInit failure.')
         ..writeln('An error occurred when initializing golden file tryjob with ')
-        ..writeln('goldctl.')
-        ..writeln()
-        ..writeln('Debug information for Gold:')
-        ..writeln('stdout: ${result.stdout}')
-        ..writeln('stderr: ${result.stderr}');
-      throw Exception(buf.toString());
+        ..writeln('goldctl.');
+      throw SkiaGoldProcessError(
+        command: tryjobInitCommand,
+        stdout: result.stdout.toString(),
+        stderr: result.stderr.toString(),
+        message: buf.toString(),
+      );
     } else if (verbose) {
       _stderr.writeln('stdout:\n${result.stdout}');
       _stderr.writeln('stderr:\n${result.stderr}');
@@ -414,13 +405,13 @@ interface class SkiaGoldClient {
       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.')
-        ..writeln()
-        ..writeln('Debug information for Gold:')
-        ..writeln('stdout: ${result.stdout}')
-        ..writeln('stderr: ${result.stderr}')
-        ..writeln();
-      throw Exception(buf.toString());
+        ..writeln('a reason unrelated to pixel comparison.');
+      throw SkiaGoldProcessError(
+        command: tryjobCommand,
+        stdout: resultStdout,
+        stderr: result.stderr.toString(),
+        message: buf.toString(),
+      );
     } else if (verbose) {
       _stderr.writeln('stdout:\n${result.stdout}');
       _stderr.writeln('stderr:\n${result.stderr}');
@@ -489,7 +480,7 @@ interface class SkiaGoldClient {
       workingDirectory: engineCheckout,
     );
     if (revParse.exitCode != 0) {
-      throw Exception('Current commit of the engine can not be found from path $engineCheckout.');
+      throw StateError('Current commit of the engine can not be found from path $engineCheckout.');
     }
     return (revParse.stdout as String).trim();
   }

testing/skia_gold_client/lib/src/errors.dart

@@ -0,0 +1,53 @@
+/// Skia Gold errors thrown by intepreting process exits and [stdout]/[stderr].
+final class SkiaGoldProcessError extends Error {
+  /// Creates a new [SkiaGoldProcessError] from the provided origin.
+  ///
+  /// - [command] is the command that was executed.
+  /// - [stdout] is the result of the process's standard output.
+  /// - [stderr] is the result of the process's standard error.
+  ///
+  /// Optionally, [message] as context for the error.
+  ///
+  /// ## Example
+  ///
+  /// ```dart
+  /// final io.ProcessResult result = await _runCommand(someCommand);
+  /// if (result.exitCode != 0) {
+  ///   throw SkiaGoldProcessError(
+  ///     command: someCommand,
+  ///     stdout: result.stdout.toString(),
+  ///     stderr: result.stderr.toString(),
+  ///     message: 'Authentication failed <or whatever we were doing>',
+  ///   );
+  /// }
+  /// ```
+  SkiaGoldProcessError({
+    required Iterable<String> command,
+    required this.stdout,
+    required this.stderr,
+    this.message,
+  }) : command = List<String>.unmodifiable(command);
+
+  /// Optional message to include as context for the error.
+  final String? message;
+
+  /// Command that was executed.
+  final List<String> command;
+
+  /// The result of the process's standard output.
+  final String stdout;
+
+  /// The result of the process's standard error.
+  final String stderr;
+
+  @override
+  String toString() {
+    return <String>[
+      'Error when running Skia Gold: ${command.join(' ')}',
+      if (message != null) message!,
+      '',
+      'stdout: $stdout',
+      'stderr: $stderr',
+    ].join('\n');
+  }
+}

testing/skia_gold_client/test/skia_gold_client_test.dart

@@ -80,7 +80,7 @@ void main() {
       try {
         await client.auth();
         fail('auth should fail if GOLDCTL is not set');
-      } catch (error) {
+      } on StateError catch (error) {
         expect('$error', contains('GOLDCTL is not set'));
       }
     } finally {
@@ -176,15 +176,17 @@ void main() {
         fixture,
         environment: presubmitEnv,
         onRun: (List<String> command) {
-          return io.ProcessResult(1, 0, '', 'error-text');
+          return io.ProcessResult(1, 0, 'stdout-text', 'stderr-text');
         },
       );
 
       try {
         await client.auth();
-      } catch (error) {
-        expect('$error', contains('Skia Gold authorization failed.'));
-        expect('$error', contains('error-text'));
+      } on SkiaGoldProcessError catch (error) {
+        expect(error.command, contains('auth'));
+        expect(error.stdout, 'stdout-text');
+        expect(error.stderr, 'stderr-text');
+        expect(error.message, contains('Skia Gold authorization failed'));
       }
     } finally {
       fixture.dispose();
@@ -343,7 +345,7 @@ void main() {
           if (command case ['python tools/goldctl.py', 'imgtest', 'init', ...]) {
             return io.ProcessResult(0, 0, '', '');
           }
-          return io.ProcessResult(1, 0, '', 'error-text');
+          return io.ProcessResult(1, 0, 'stdout-text', 'stderr-text');
         },
       );
 
@@ -353,9 +355,11 @@ void main() {
           io.File(p.join(fixture.workDirectory.path, 'temp', 'golden.png')),
           screenshotSize: 1000,
         );
-      } catch (error) {
-        expect('$error', contains('Skia Gold image test failed.'));
-        expect('$error', contains('error-text'));
+      } on SkiaGoldProcessError catch (error) {
+        expect(error.message, contains('Skia Gold image test failed.'));
+        expect(error.stdout, 'stdout-text');
+        expect(error.stderr, 'stderr-text');
+        expect(error.command, contains('imgtest add'));
       }
     } finally {
       fixture.dispose();
@@ -470,7 +474,7 @@ void main() {
           if (command case ['python tools/goldctl.py', 'imgtest', 'init', ...]) {
             return io.ProcessResult(0, 0, '', '');
           }
-          return io.ProcessResult(1, 0, '', 'error-text');
+          return io.ProcessResult(1, 0, 'stdout-text', 'stderr-text');
         },
       );
 
@@ -480,9 +484,11 @@ void main() {
           io.File(p.join(fixture.workDirectory.path, 'temp', 'golden.png')),
           screenshotSize: 1000,
         );
-      } catch (error) {
-        expect('$error', contains('Skia Gold image test failed.'));
-        expect('$error', contains('error-text'));
+      } on SkiaGoldProcessError catch (error) {
+        expect(error.message, contains('Skia Gold image test failed.'));
+        expect(error.stdout, 'stdout-text');
+        expect(error.stderr, 'stderr-text');
+        expect(error.command, contains('imgtest add'));
       }
     } finally {
       fixture.dispose();