Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

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

Merged
matanlurey merged 1 commit into from
Mar 14, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 40 additions & 49 deletions testing/skia_gold_client/lib/skia_gold_client.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';

@gaaclarke gaaclarke Mar 14, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these uri's have the package in them instead of being relative paths?

@matanlurey matanlurey Mar 14, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel strongly either way (would prefer a lint).

It appears we're intentionally opting out. I'll file a follow-up issue to suggest opting back in?

Screenshot 2024-03-14 at 1 26 25 PM


export 'src/errors.dart' show SkiaGoldProcessError;

const String _kGoldctlKey = 'GOLDCTL';
const String _kPresubmitEnvName = 'GOLD_TRYJOB';
const String _kLuciEnvName = 'LUCI_CONTEXT';
Expand Down Expand Up @@ -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}');
Expand Down Expand Up @@ -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}');
Expand Down Expand Up @@ -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}');
Expand Down Expand Up @@ -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}');
Expand Down Expand Up @@ -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}');
Expand Down Expand Up @@ -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();
}
Expand Down
53 changes: 53 additions & 0 deletions testing/skia_gold_client/lib/src/errors.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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');
}
}
32 changes: 19 additions & 13 deletions testing/skia_gold_client/test/skia_gold_client_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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');
},
);

Expand All @@ -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();
Expand Down Expand Up @@ -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');
},
);

Expand All @@ -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();
Expand Down