Skip to content

Commit

Permalink
[golden_test_harvester]: Put back sending the dimensions to the SkiaG…
Browse files Browse the repository at this point in the history 
…oldClient (flutter#51536)

fixes flutter/flutter#145413

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide] and the [C++,
Objective-C, Java style guides].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I added new tests to check the change I am making or feature I am
adding, or the PR is [test-exempt]. See [testing the engine] for
instructions on writing and running engine tests.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I signed the [CLA].
- [x] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[C++, Objective-C, Java style guides]:
https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[testing the engine]:
https://github.com/flutter/flutter/wiki/Testing-the-engine
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
  • Loading branch information
@gaaclarke
gaaclarke authored Mar 20, 2024
1 parent 98cfd92 commit 912c61f
Show file tree
Hide file tree
Showing 3 changed files with 262 additions and 118 deletions.
23 changes: 9 additions & 14 deletions tools/golden_tests_harvester/bin/golden_tests_harvester.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,23 +41,18 @@ Future<void> main(List<String> args) async {
}

final io.Directory workDirectory = io.Directory(rest.single);
final AddImageToSkiaGold addImg;
final bool dryRun = results['dry-run'] as bool;
if (dryRun) {
final bool isDryRun = results['dry-run'] as bool;
final Harvester harvester;
if (isDryRun) {
io.stderr.writeln('=== DRY RUN. Results not submitted to Skia Gold. ===');
addImg = _dryRunAddImg;
harvester =
await Harvester.create(workDirectory, io.stderr,
addImageToSkiaGold: _dryRunAddImg);
} else {
// If GOLDCTL is not configured (i.e. on CI), this will throw.
final SkiaGoldClient client = SkiaGoldClient(workDirectory);
await client.auth();
addImg = client.addImg;
harvester =
await Harvester.create(workDirectory, io.stderr);
}

await harvest(
workDirectory: workDirectory,
addImg: addImg,
stderr: io.stderr,
);
await harvest(harvester);
}

Future<void> _dryRunAddImg(
Expand Down
219 changes: 159 additions & 60 deletions tools/golden_tests_harvester/lib/golden_tests_harvester.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,77 +5,176 @@
import 'dart:io' as io;

import 'package:path/path.dart' as p;
import 'package:skia_gold_client/skia_gold_client.dart';

import 'src/digests_json_format.dart';

/// Uploads the images of digests in [workDirectory] to Skia Gold.
///
/// The directory is expected to match the following structure:
/// ```txt
/// workDirectory/
/// - digest.json
/// - test_name_1.png
/// - test_name_2.png
/// - ...
/// ```
///
/// The format of `digest.json` is expected to match the following:
/// ```jsonc
/// {
/// "dimensions": {
/// // Key-value pairs of dimensions to provide to Skia Gold.
/// // For example:
/// "platform": "linux",
/// },
/// "entries": [
/// // Each entry is a test-run with the following format:
/// {
/// // Path must be a direct sibling of digest.json.
/// "filename": "test_name_1.png",
///
/// // Called `screenshotSize` in Skia Gold (width * height).
/// "width": 100,
/// "height": 100,
///
/// // Called `differentPixelsRate` in Skia Gold.
/// "maxDiffPixelsPercent": 0.01,
///
/// // Called `pixelColorDelta` in Skia Gold.
/// "maxColorDelta": 0
/// }
/// ]
/// }
/// ```
Future<void> harvest({
required io.Directory workDirectory,
required AddImageToSkiaGold addImg,
required StringSink stderr,
}) async {
final io.File file = io.File(p.join(workDirectory.path, 'digest.json'));
if (!file.existsSync()) {
// Check if the directory exists or if the file is just missing.
if (!workDirectory.existsSync()) {
throw ArgumentError('Directory not found: ${workDirectory.path}.');
/// Used by [harvest] to process a directory for Skia Gold upload.
abstract class Harvester {
/// Creates a new [Harvester] from the directory at [workDirectory].
///
/// The directory is expected to match the following structure:
/// ```txt
/// workDirectory/
/// - digest.json
/// - test_name_1.png
/// - test_name_2.png
/// - ...
/// ```
///
/// The format of `digest.json` is expected to match the following:
/// ```jsonc
/// {
/// "dimensions": {
/// // Key-value pairs of dimensions to provide to Skia Gold.
/// // For example:
/// "platform": "linux",
/// },
/// "entries": [
/// // Each entry is a test-run with the following format:
/// {
/// // Path must be a direct sibling of digest.json.
/// "filename": "test_name_1.png",
///
/// // Called `screenshotSize` in Skia Gold (width * height).
/// "width": 100,
/// "height": 100,
///
/// // Called `differentPixelsRate` in Skia Gold.
/// "maxDiffPixelsPercent": 0.01,
///
/// // Called `pixelColorDelta` in Skia Gold.
/// "maxColorDelta": 0
/// }
/// ]
/// }
/// ```
static Future<Harvester> create(
io.Directory workDirectory, StringSink stderr,
{AddImageToSkiaGold? addImageToSkiaGold}) async {
final io.File file = io.File(p.join(workDirectory.path, 'digest.json'));
if (!file.existsSync()) {
// Check if the directory exists or if the file is just missing.
if (!workDirectory.existsSync()) {
throw ArgumentError('Directory not found: ${workDirectory.path}.');
}
// Lookup sibling files to help the user understand what's missing.
final List<io.FileSystemEntity> files = workDirectory.listSync();
throw StateError(
'File "digest.json" not found in ${workDirectory.path}.\n\n'
'Found files: ${files.map((io.FileSystemEntity e) => p.basename(e.path)).join(', ')}',
);
}
final Digests digests = Digests.parse(file.readAsStringSync());

if (addImageToSkiaGold != null) {
return _DryRunHarvester(digests, stderr, workDirectory, addImageToSkiaGold);
} else {
return SkiaGoldHarvester._create(digests, stderr, workDirectory);
}
// Lookup sibling files to help the user understand what's missing.
final List<io.FileSystemEntity> files = workDirectory.listSync();
throw StateError(
'File "digest.json" not found in ${workDirectory.path}.\n\n'
'Found files: ${files.map((io.FileSystemEntity e) => p.basename(e.path)).join(', ')}',
);
}
final Digests digests = Digests.parse(file.readAsStringSync());

Future<void> _addImg(
String testName,
io.File goldenFile, {
double differentPixelsRate,
int pixelColorDelta,
required int screenshotSize,
});

Future<void> _auth();

Digests get _digests;
StringSink get _stderr;
io.Directory get _workDirectory;
}

/// A [Harvester] that communicates with a real [SkiaGoldClient].
class SkiaGoldHarvester implements Harvester {
SkiaGoldHarvester._init(
this._digests, this._stderr, this._workDirectory, this.client);

@override
final Digests _digests;
@override
final StringSink _stderr;
@override
final io.Directory _workDirectory;
/// The [SkiaGoldClient] that will be used for harvesting.
final SkiaGoldClient client;

static Future<SkiaGoldHarvester> _create(
Digests digests, StringSink stderr, io.Directory workDirectory) async {
final SkiaGoldClient client =
SkiaGoldClient(workDirectory, dimensions: digests.dimensions);
return SkiaGoldHarvester._init(digests, stderr, workDirectory, client);
}

@override
Future<void> _addImg(String testName, io.File goldenFile,
{double differentPixelsRate = 0.01,
int pixelColorDelta = 0,
required int screenshotSize}) async {
return client.addImg(testName, goldenFile,
differentPixelsRate: differentPixelsRate,
pixelColorDelta: pixelColorDelta,
screenshotSize: screenshotSize);
}

@override
Future<void> _auth() {
return client.auth();
}
}

/// A [Harvester] that doesn't harvest, just calls a callback.
class _DryRunHarvester implements Harvester {
_DryRunHarvester(
this._digests, this._stderr, this._workDirectory,
this._addImageToSkiaGold);

@override
final Digests _digests;
@override
final StringSink _stderr;
@override
final io.Directory _workDirectory;
final AddImageToSkiaGold _addImageToSkiaGold;

@override
Future<void> _addImg(String testName, io.File goldenFile,
{double differentPixelsRate = 0.01,
int pixelColorDelta = 0,
required int screenshotSize}) async {
return _addImageToSkiaGold(testName, goldenFile,
differentPixelsRate: differentPixelsRate,
pixelColorDelta: pixelColorDelta,
screenshotSize: screenshotSize);
}

@override
Future<void> _auth() async {
_stderr.writeln('using dimensions: ${_digests.dimensions}');
}
}

/// Uploads the images of digests in [harvester] to Skia Gold.
Future<void> harvest(Harvester harvester) async {
await harvester._auth();
final List<Future<void>> pendingComparisons = <Future<void>>[];
for (final DigestEntry entry in digests.entries) {
final io.File goldenFile = io.File(p.join(workDirectory.path, entry.filename));
final Future<void> future = addImg(
for (final DigestEntry entry in harvester._digests.entries) {
final io.File goldenFile =
io.File(p.join(harvester._workDirectory.path, entry.filename));
final Future<void> future = harvester
._addImg(
entry.filename,
goldenFile,
screenshotSize: entry.width * entry.height,
differentPixelsRate: entry.maxDiffPixelsPercent,
pixelColorDelta: entry.maxColorDelta,
).catchError((Object e) {
stderr.writeln('Failed to add image to Skia Gold: $e');
)
.catchError((Object e) {
harvester._stderr.writeln('Failed to add image to Skia Gold: $e');
throw FailedComparisonException(entry.filename);
});
pendingComparisons.add(future);
Expand Down
Loading

0 comments on commit 912c61f

Please sign in to comment.