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

Commit 8630c4b

Browse files
matanlureymatanlurey
authored
Implement .engine-release.version files for engine Skia Gold tests (#51739)
Work towards flutter/flutter#144835. Doc (_sorry, internal only_): [go/flutter-engine-goldens-workflow](http://goto.google.com/flutter-engine-goldens-workflow). This implements the majority of the proposed workflow, that is, optionally having a plain-text version at the root of the directory, and using it to apply a unique suffix we can review in release branches. As it stands, this is a NO-OP outside of tests (it will have no impact, and can be ignored). What's missing before using this feature in release branches: - Optimization work with the infra team (not sure if blocking or not): flutter/flutter#145842 - A dry-run of this with the release team to make sure it works as intended @gaaclarke As implemented, I _think_ we don't need anything special for [`dir_contents_diff`](https://github.com/flutter/engine/blob/b0d3663f439fd97c32bf4d46b9004c97841c43b8/tools/dir_contents_diff), but maybe I'm wrong - I think only the _test_ names are being changed, not the names on disk. /cc @zanderso as well.
1 parent 79f1530 commit 8630c4b

File tree

9 files changed

+483
-74
lines changed

9 files changed

+483
-74
lines changed

‎.engine-release.version

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
# Used to branch and track golden-file rendering of release branches.
2+
#
3+
# See ./testing/skia_gold_client/README.md#release-testing for more information.
4+
5+
# On the main branch, this should always read 'none'.
6+
# On release branches, this should be the release version such as '3.21'
7+
# (without the quotes).
8+
none

‎ci/licenses_golden/licenses_flutter

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41995,6 +41995,7 @@ ORIGIN: ../../../flutter/vulkan/vulkan_window.cc + ../../../flutter/LICENSE
4199541995
ORIGIN: ../../../flutter/vulkan/vulkan_window.h + ../../../flutter/LICENSE
4199641996
TYPE: LicenseType.bsd
4199741997
FILE: ../../../flutter/.clangd
41998+
FILE: ../../../flutter/.engine-release.version
4199841999
FILE: ../../../flutter/.pylintrc
4199942000
FILE: ../../../flutter/assets/asset_manager.cc
4200042001
FILE: ../../../flutter/assets/asset_manager.h

‎testing/litetest/lib/src/matchers.dart

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,11 @@ void throwsRangeError(dynamic d) {
8787
Expect.throwsRangeError(d as void Function());
8888
}
8989

90+
/// A [Matcher] that matches functions that throw a [RangeError] when invoked.
91+
void throwsFormatException(dynamic d) {
92+
Expect.throwsFormatException(d as void Function());
93+
}
94+
9095
/// Gives a [Matcher] that asserts that the value being matched is a [String]
9196
/// that contains `s` as a substring.
9297
Matcher contains(String s) => (dynamic d) {

‎testing/skia_gold_client/README.md

Lines changed: 96 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -1,70 +1,111 @@
1-
# skia_gold_client
1+
# Skia Gold Client
22

3-
This package allows to create a Skia gold client in the engine repo.
3+
This package interacts with [Skia Gold][] for uploading and comparing
4+
screenshots.
45

5-
The client uses the `goldctl` tool on LUCI builders to upload screenshots,
6-
and verify if a new screenshot matches the baseline screenshot.
6+
[skia gold]: https://skia.org/docs/dev/testing/skiagold/
77

8-
The web UI is available on https://flutter-engine-gold.skia.org/.
8+
The web UI for the engine is located at <https://flutter-engine-gold.skia.org/>.
99

10-
## Using the client
10+
## Usage
1111

12-
1. In `.ci.yaml`, ensure that the task has a dependency on `goldctl`:
12+
In the simplest case, import the package and establish a working directory:
1313

14-
```yaml
15-
dependencies: [{ "dependency": "goldctl" }]
16-
```
14+
```dart
15+
import 'dart:io' as io;
1716
18-
2. In the builder `.json` file, ensure the drone has a dependency on `goldctl`:
17+
import 'package:skia_gold_client/skia_gold_client.dart';
1918
20-
```yaml
21-
"dependencies": [
22-
{
23-
"dependency": "goldctl",
24-
"version": "git_revision:<sha>"
25-
}
26-
],
19+
void main() async {
20+
// Create a temporary working directory.
21+
final io.Directory tmpDirectory = io.Directory.systemTemp.createTempSync('skia_gold_wd');
22+
try {
23+
final SkiaGoldClient client = SkiaGoldClient(tmpDirectory);
24+
await client.auth();
25+
// ...
26+
} finally {
27+
tmpDirectory.deleteSync(recursive: true);
28+
}
29+
}
2730
```
2831

29-
3. Add dependency in `pubspec.yaml`:
32+
Once you have an authorized instance, use `addImg` to upload a screenshot:
3033

31-
```yaml
32-
dependencies:
33-
# needed for skia_gold_client to avoid a cache miss.
34-
engine_repo_tools:
35-
path: <relative-path>/tools/pkg/engine_repo_tools
36-
skia_gold_client:
37-
path: <relative-path>/testing/skia_gold_client
34+
```dart
35+
await client.addImg(
36+
'my-screenshot',
37+
io.File('path/to/screenshot.png'),
38+
screenshotSize: 400, // i.e. a 20x20 image
39+
);
3840
```
3941

40-
4. Use the client:
42+
## Configuring CI
4143

42-
```dart
43-
import 'package:skia_gold_client/skia_gold_client.dart';
44+
Currently[^1], the client is only available on Flutter Engine's CI platform, and
45+
will fail to authenticate if run elsewhere.
4446

45-
Future<void> main() {
46-
final Directory tmpDirectory = Directory.current.createTempSync('skia_gold_wd');
47-
final SkiaGoldClient client = SkiaGoldClient(
48-
tmpDirectory,
49-
dimensions: <String, String> {'<attribute-name>': '<attribute-value>'},
50-
);
51-
52-
try {
53-
if (SkiaGoldClient.isAvailable()) {
54-
await client.auth();
55-
56-
await client.addImg(
57-
'<file-name>',
58-
File('gold-file.png'),
59-
screenshotSize: 1024,
60-
);
61-
}
62-
} catch (error) {
63-
// Failed to authenticate or compare pixels.
64-
stderr.write(error.toString());
65-
rethrow;
66-
} finally {
67-
tmpDirectory.deleteSync(recursive: true);
68-
}
69-
}
70-
```
47+
To use the client in CI, you'll need to make two changes:
48+
49+
[^1]:
50+
The `flutter/flutter` repository has a workaround which downloads digests
51+
and does basic local image comparison, but because we have forked the
52+
client and not kept it up-to-date, we cannot use that workaround. Send
53+
a PR or file an issue if you'd like to see this fixed!
54+
55+
1. **Add a dependency on `goldctl`**
56+
57+
In your task's configuration in [`.ci.yaml`](../../.ci.yaml) file, add a
58+
dependency on `goldctl`:
59+
60+
```diff
61+
# This is just an example.
62+
targets:
63+
- name: Linux linux_android_emulator_tests
64+
properties:
65+
config_name: linux_android_emulator
66+
+ dependencies: >-
67+
+ [
68+
+ {"dependency": "goldctl", "version": "git_revision:720a542f6fe4f92922c3b8f0fdcc4d2ac6bb83cd"}
69+
+ ]
70+
```
71+
72+
2. **Ensure the builder (i.e. `config_name: {name}`) also has a dependency**
73+
74+
For example, for `linux_android_emulator`, modify
75+
[`ci/builders/linux_android_emulator.json`](../../ci/builders/linux_android_emulator.json):
76+
77+
```json
78+
"dependencies": [
79+
{
80+
"dependency": "goldctl",
81+
"version": "git_revision:720a542f6fe4f92922c3b8f0fdcc4d2ac6bb83cd"
82+
}
83+
]
84+
```
85+
86+
## Release Testing
87+
88+
> [!NOTE]
89+
> This workflow is a work in progress. Contact @matanlurey for more information.
90+
91+
When we create a release branch (i.e. for a beta or stable release), all
92+
golden-file tests will have to be regenerated for the new release. This is
93+
because it's possible that the rendering of the engine has changed in a way
94+
that affects the golden files (either due to a bug, or intentionally) as we
95+
apply cherry-picks and other changes to the release branch.
96+
97+
Fortunately this process is easy and mostly automated. Here's how it works:
98+
99+
1. Create your release branch, e.g. `flutter-3.21-candidate.1`.
100+
1. Edit [`.engine-release.verison`](../../.engine-release.version) to the new
101+
release version (e.g. `3.21`).
102+
1. Run all the tests, generating new golden files.
103+
1. Bulk triage all of the images as positive using the web UI.
104+
105+
![Screenshot](https://github.com/flutter/flutter/assets/168174/a327ffc0-95b3-4d3a-9d36-052e0607a1e5)
106+
107+
All of the tests will have a unique `_Release_{major}}_{minor}` suffix, so you
108+
can easily filter them in the web UI and they can diverge from the `main` branch
109+
as needed. As cherry-picks are applied to the release branch, the tests should
110+
continue to pass, and the golden files should either _not_ change, or change in
111+
a way that is expected (i.e. fixing a bug).

‎testing/skia_gold_client/lib/skia_gold_client.dart

Lines changed: 93 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import 'package:path/path.dart' as path;
1212
import 'package:process/process.dart';
1313

1414
import 'src/errors.dart';
15+
import 'src/release_version.dart';
1516

1617
export 'src/errors.dart' show SkiaGoldProcessError;
1718

@@ -22,25 +23,89 @@ const String _kLuciEnvName = 'LUCI_CONTEXT';
2223
const String _skiaGoldHost = 'https://flutter-engine-gold.skia.org';
2324
const String _instance = 'flutter-engine';
2425

25-
/// A client for uploading image tests and making baseline requests to the
26-
/// Flutter Gold Dashboard.
26+
/// Uploads images and makes baseline requests to Skia Gold.
27+
///
28+
/// For an example of how to use this class, see `tool/e2e_test.dart`.
2729
interface class SkiaGoldClient {
2830
/// Creates a [SkiaGoldClient] with the given [workDirectory].
2931
///
30-
/// [dimensions] allows to add attributes about the environment
31-
/// used to generate the screenshots.
32-
SkiaGoldClient(
32+
/// A set of [dimensions] can be provided to add attributes about the
33+
/// environment used to generate the screenshots, which are treated as keys
34+
/// for the image:
35+
///
36+
/// ```dart
37+
/// final SkiaGoldClient skiaGoldClient = SkiaGoldClient(
38+
/// someDir,
39+
/// dimensions: <String, String>{
40+
/// 'platform': 'linux',
41+
/// },
42+
/// );
43+
/// ```
44+
///
45+
/// The [verbose] flag is intended for use in debugging CI issues, and
46+
/// produces more detailed output that some may find useful, but would be too
47+
/// spammy for regular use.
48+
factory SkiaGoldClient(
49+
io.Directory workDirectory, {
50+
Map<String, String>? dimensions,
51+
bool verbose = false,
52+
}) {
53+
return SkiaGoldClient.forTesting(
54+
workDirectory,
55+
dimensions: dimensions,
56+
verbose: verbose,
57+
);
58+
}
59+
60+
/// Creates a [SkiaGoldClient] for testing.
61+
///
62+
/// Similar to the default constructor, but allows for dependency injection
63+
/// for testing purposes:
64+
///
65+
/// - [httpClient] makes requests to Skia Gold to fetch expectations.
66+
/// - [processManager] launches sub-processes.
67+
/// - [stderr] is where output is written for diagnostics.
68+
/// - [environment] is the environment variables for the currently running
69+
/// process, and is used to determine if Skia Gold is available, and whether
70+
/// the current environment is CI, and if so, if it's pre-submit or
71+
/// post-submit.
72+
/// - [engineRoot] is the root of the engine repository, which is used for
73+
/// finding the current commit hash, as well as the location of the
74+
/// `.engine-release.version` file.
75+
@visibleForTesting
76+
SkiaGoldClient.forTesting(
3377
this.workDirectory, {
3478
this.dimensions,
3579
this.verbose = false,
3680
io.HttpClient? httpClient,
3781
ProcessManager? processManager,
3882
StringSink? stderr,
3983
Map<String, String>? environment,
84+
Engine? engineRoot,
4085
}) : httpClient = httpClient ?? io.HttpClient(),
4186
process = processManager ?? const LocalProcessManager(),
4287
_stderr = stderr ?? io.stderr,
43-
_environment = environment ?? io.Platform.environment;
88+
_environment = environment ?? io.Platform.environment,
89+
_engineRoot = engineRoot ?? Engine.findWithin() {
90+
// Lookup the release version from the engine repository.
91+
final io.File releaseVersionFile = io.File(path.join(
92+
_engineRoot.flutterDir.path,
93+
'.engine-release.version',
94+
));
95+
96+
// If the file is not found or cannot be read, we are in an invalid state.
97+
try {
98+
_releaseVersion = ReleaseVersion.parse(releaseVersionFile.readAsStringSync());
99+
} on FormatException catch (error) {
100+
throw StateError('Failed to parse release version file: $error.');
101+
} on io.FileSystemException catch (error) {
102+
throw StateError('Failed to read release version file: $error.');
103+
}
104+
}
105+
106+
/// The root of the engine repository.
107+
final Engine _engineRoot;
108+
ReleaseVersion? _releaseVersion;
44109

45110
/// Whether the client is available and can be used in this environment.
46111
static bool isAvailable({
@@ -244,6 +309,14 @@ interface class SkiaGoldClient {
244309
/// determined by the [pixelColorDelta] parameter. It's in the range [0.0,
245310
/// 1.0] and defaults to 0.01. A value of 0.01 means that 1% of the pixels are
246311
/// allowed to be different.
312+
///
313+
/// ## Release Testing
314+
///
315+
/// In release branches, we add a unique test suffix to the test name. For
316+
/// example "testName" -> "testName_Release_3_21", based on the version in the
317+
/// `.engine-release.version` file at the root of the engine repository.
318+
///
319+
/// See <../README.md#release-testing> for more information.
247320
Future<void> addImg(
248321
String testName,
249322
io.File goldenFile, {
@@ -252,6 +325,17 @@ interface class SkiaGoldClient {
252325
required int screenshotSize,
253326
}) async {
254327
assert(_isPresubmit || _isPostsubmit);
328+
329+
// Clean the test name to remove the file extension.
330+
testName = path.basenameWithoutExtension(testName);
331+
332+
// In release branches, we add a unique test suffix to the test name.
333+
// For example "testName" -> "testName_Release_3_21".
334+
// See ../README.md#release-testing for more information.
335+
if (_releaseVersion case final ReleaseVersion v) {
336+
testName = '${testName}_Release_${v.major}_${v.minor}';
337+
}
338+
255339
if (_isPresubmit) {
256340
await _tryjobAdd(testName, goldenFile, screenshotSize, pixelColorDelta, differentPixelsRate);
257341
}
@@ -287,7 +371,7 @@ interface class SkiaGoldClient {
287371
'--work-dir',
288372
_tempPath,
289373
'--test-name',
290-
_cleanTestName(testName),
374+
testName,
291375
'--png-file',
292376
goldenFile.path,
293377
// Otherwise post submit will not fail.
@@ -391,7 +475,7 @@ interface class SkiaGoldClient {
391475
'--work-dir',
392476
_tempPath,
393477
'--test-name',
394-
_cleanTestName(testName),
478+
testName,
395479
'--png-file',
396480
goldenFile.path,
397481
..._getMatchingArguments(testName, screenshotSize, pixelDeltaThreshold, differentPixelsRate),
@@ -489,7 +573,7 @@ interface class SkiaGoldClient {
489573

490574
/// Returns the current commit hash of the engine repository.
491575
Future<String> _getCurrentCommit() async {
492-
final String engineCheckout = Engine.findWithin().flutterDir.path;
576+
final String engineCheckout = _engineRoot.flutterDir.path;
493577
final io.ProcessResult revParse = await process.run(
494578
<String>['git', 'rev-parse', 'HEAD'],
495579
workingDirectory: engineCheckout,
@@ -521,12 +605,6 @@ interface class SkiaGoldClient {
521605
return json.encode(_getKeys());
522606
}
523607

524-
/// Removes the file extension from the [fileName] to represent the test name
525-
/// properly.
526-
static String _cleanTestName(String fileName) {
527-
return path.basenameWithoutExtension(fileName);
528-
}
529-
530608
/// Returns a list of arguments for initializing a tryjob based on the testing
531609
/// environment.
532610
List<String> _getCIArguments() {

‎testing/skia_gold_client/lib/src/errors.dart

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,7 @@
1+
// Copyright 2013 The Flutter Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
15
/// Skia Gold errors thrown by intepreting process exits and [stdout]/[stderr].
26
final class SkiaGoldProcessError extends Error {
37
/// Creates a new [SkiaGoldProcessError] from the provided origin.

0 commit comments

Comments
 (0)