Implement .engine-release.version files for engine Skia Gold tests

.engine-release.version

@@ -0,0 +1,8 @@
+# Used to branch and track golden-file rendering of release branches.
+#
+# See ./testing/skia_gold_client/README.md#release-testing for more information.
+
+# On the main branch, this should always read 'none'.
+# On release branches, this should be the release version such as '3.21'
+# (without the quotes).
+none

ci/licenses_golden/licenses_flutter

@@ -41992,6 +41992,7 @@ ORIGIN: ../../../flutter/vulkan/vulkan_utilities.h + ../../../flutter/LICENSE
 ORIGIN: ../../../flutter/vulkan/vulkan_window.cc + ../../../flutter/LICENSE
 ORIGIN: ../../../flutter/vulkan/vulkan_window.h + ../../../flutter/LICENSE
 TYPE: LicenseType.bsd
+FILE: ../../../flutter/.engine-release.version
 FILE: ../../../flutter/.pylintrc
 FILE: ../../../flutter/assets/asset_manager.cc
 FILE: ../../../flutter/assets/asset_manager.h

testing/litetest/lib/src/matchers.dart

@@ -87,6 +87,11 @@ void throwsRangeError(dynamic d) {
   Expect.throwsRangeError(d as void Function());
 }
 
+/// A [Matcher] that matches functions that throw a [RangeError] when invoked.
+void throwsFormatException(dynamic d) {
+  Expect.throwsFormatException(d as void Function());
+}
+
 /// Gives a [Matcher] that asserts that the value being matched is a [String]
 /// that contains `s` as a substring.
 Matcher contains(String s) => (dynamic d) {

testing/skia_gold_client/README.md

@@ -1,70 +1,111 @@
-# skia_gold_client
+# Skia Gold Client
 
-This package allows to create a Skia gold client in the engine repo.
+This package interacts with [Skia Gold][] for uploading and comparing
+screenshots.
 
-The client uses the `goldctl` tool on LUCI builders to upload screenshots,
-and verify if a new screenshot matches the baseline screenshot.
+[skia gold]: https://skia.org/docs/dev/testing/skiagold/
 
-The web UI is available on https://flutter-engine-gold.skia.org/.
+The web UI for the engine is located at <https://flutter-engine-gold.skia.org/>.
 
-## Using the client
+## Usage
 
-1. In `.ci.yaml`, ensure that the task has a dependency on `goldctl`:
+In the simplest case, import the package and establish a working directory:
 
-```yaml
-dependencies: [{ "dependency": "goldctl" }]
-```
+```dart
+import 'dart:io' as io;
 
-2. In the builder `.json` file, ensure the drone has a dependency on `goldctl`:
+import 'package:skia_gold_client/skia_gold_client.dart';
 
-```yaml
-            "dependencies": [
-                {
-                    "dependency": "goldctl",
-                    "version": "git_revision:<sha>"
-                }
-            ],
+void main() async {
+  // Create a temporary working directory.
+  final io.Directory tmpDirectory = io.Directory.systemTemp.createTempSync('skia_gold_wd');
+  try {
+    final SkiaGoldClient client = SkiaGoldClient(tmpDirectory);
+    await client.auth();
+    // ...
+  } finally {
+    tmpDirectory.deleteSync(recursive: true);
+  }
+}
 ```
 
-3. Add dependency in `pubspec.yaml`:
+Once you have an authorized instance, use `addImg` to upload a screenshot:
 
-```yaml
-dependencies:
-  # needed for skia_gold_client to avoid a cache miss.
-  engine_repo_tools:
-    path: <relative-path>/tools/pkg/engine_repo_tools
-  skia_gold_client:
-    path: <relative-path>/testing/skia_gold_client
+```dart
+await client.addImg(
+  'my-screenshot',
+  io.File('path/to/screenshot.png'),
+  screenshotSize: 400, // i.e. a 20x20 image
+);
 ```
 
-4. Use the client:
+## Configuring CI
 
-```dart
-import 'package:skia_gold_client/skia_gold_client.dart';
+Currently[^1], the client is only available on Flutter Engine's CI platform, and
+will fail to authenticate if run elsewhere.
 
-Future<void> main() {
-    final Directory tmpDirectory = Directory.current.createTempSync('skia_gold_wd');
-    final SkiaGoldClient client = SkiaGoldClient(
-        tmpDirectory,
-        dimensions: <String, String> {'<attribute-name>': '<attribute-value>'},
-    );
-
-    try {
-        if (SkiaGoldClient.isAvailable()) {
-            await client.auth();
-
-            await client.addImg(
-                '<file-name>',
-                File('gold-file.png'),
-                screenshotSize: 1024,
-            );
-        }
-    } catch (error) {
-        // Failed to authenticate or compare pixels.
-        stderr.write(error.toString());
-        rethrow;
-    } finally {
-        tmpDirectory.deleteSync(recursive: true);
-    }
-}
-```
+To use the client in CI, you'll need to make two changes:
+
+[^1]:
+    The `flutter/flutter` repository has a workaround which downloads digests
+    and does basic local image comparison, but because we have forked the
+    client and not kept it up-to-date, we cannot use that workaround. Send
+    a PR or file an issue if you'd like to see this fixed!
+
+1. **Add a dependency on `goldctl`**
+
+   In your task's configuration in [`.ci.yaml`](../../.ci.yaml) file, add a
+   dependency on `goldctl`:
+
+   ```diff
+   # This is just an example.
+   targets:
+     - name: Linux linux_android_emulator_tests
+       properties:
+         config_name: linux_android_emulator
+         + dependencies: >-
+         +   [
+         +     {"dependency": "goldctl", "version": "git_revision:720a542f6fe4f92922c3b8f0fdcc4d2ac6bb83cd"}
+         +   ]
+   ```
+
+2. **Ensure the builder (i.e. `config_name: {name}`) also has a dependency**
+
+   For example, for `linux_android_emulator`, modify
+   [`ci/builders/linux_android_emulator.json`](../../ci/builders/linux_android_emulator.json):
+
+   ```json
+   "dependencies": [
+     {
+       "dependency": "goldctl",
+       "version": "git_revision:720a542f6fe4f92922c3b8f0fdcc4d2ac6bb83cd"
+     }
+   ]
+   ```
+
+## Release Testing
+
+> [!NOTE]
+> This workflow is a work in progress. Contact @matanlurey for more information.
+
+When we create a release branch (i.e. for a beta or stable release), all
+golden-file tests will have to be regenerated for the new release. This is
+because it's possible that the rendering of the engine has changed in a way
+that affects the golden files (either due to a bug, or intentionally) as we
+apply cherry-picks and other changes to the release branch.
+
+Fortunately this process is easy and mostly automated. Here's how it works:
+
+1. Create your release branch, e.g. `flutter-3.21-candidate.1`.
+1. Edit [`.engine-release.verison`](../../.engine-release.version) to the new
+   release version (e.g. `3.21`).
+1. Run all the tests, generating new golden files.
+1. Bulk triage all of the images as positive using the web UI.
+
+   ![Screenshot](https://github.com/flutter/flutter/assets/168174/a327ffc0-95b3-4d3a-9d36-052e0607a1e5)
+
+All of the tests will have a unique `_Release_{major}}_{minor}` suffix, so you
+can easily filter them in the web UI and they can diverge from the `main` branch
+as needed. As cherry-picks are applied to the release branch, the tests should
+continue to pass, and the golden files should either _not_ change, or change in
+a way that is expected (i.e. fixing a bug).

testing/skia_gold_client/lib/skia_gold_client.dart

@@ -12,6 +12,7 @@ import 'package:path/path.dart' as path;
 import 'package:process/process.dart';
 
 import 'src/errors.dart';
+import 'src/release_version.dart';
 
 export 'src/errors.dart' show SkiaGoldProcessError;
 
@@ -22,25 +23,89 @@ const String _kLuciEnvName = 'LUCI_CONTEXT';
 const String _skiaGoldHost = 'https://flutter-engine-gold.skia.org';
 const String _instance = 'flutter-engine';
 
-/// A client for uploading image tests and making baseline requests to the
-/// Flutter Gold Dashboard.
+/// Uploads images and makes baseline requests to Skia Gold.
+///
+/// For an example of how to use this class, see `tool/e2e_test.dart`.
 interface class SkiaGoldClient {
   /// Creates a [SkiaGoldClient] with the given [workDirectory].
   ///
-  /// [dimensions] allows to add attributes about the environment
-  /// used to generate the screenshots.
-  SkiaGoldClient(
+  /// A set of [dimensions] can be provided to add attributes about the
+  /// environment used to generate the screenshots, which are treated as keys
+  /// for the image:
+  ///
+  /// ```dart
+  /// final SkiaGoldClient skiaGoldClient = SkiaGoldClient(
+  ///   someDir,
+  ///   dimensions: <String, String>{
+  ///     'platform': 'linux',
+  ///   },
+  /// );
+  /// ```
+  ///
+  /// The [verbose] flag is intended for use in debugging CI issues, and
+  /// produces more detailed output that some may find useful, but would be too
+  /// spammy for regular use.
+  factory SkiaGoldClient(
+    io.Directory workDirectory, {
+    Map<String, String>? dimensions,
+    bool verbose = false,
+  }) {
+    return SkiaGoldClient.forTesting(
+      workDirectory,
+      dimensions: dimensions,
+      verbose: verbose,
+    );
+  }
+
+  /// Creates a [SkiaGoldClient] for testing.
+  ///
+  /// Similar to the default constructor, but allows for dependency injection
+  /// for testing purposes:
+  ///
+  /// - [httpClient] makes requests to Skia Gold to fetch expectations.
+  /// - [processManager] launches sub-processes.
+  /// - [stderr] is where output is written for diagnostics.
+  /// - [environment] is the environment variables for the currently running
+  ///   process, and is used to determine if Skia Gold is available, and whether
+  ///   the current environment is CI, and if so, if it's pre-submit or
+  ///   post-submit.
+  /// - [engineRoot] is the root of the engine repository, which is used for
+  ///   finding the current commit hash, as well as the location of the
+  ///   `.engine-release.version` file.
+  @visibleForTesting
+  SkiaGoldClient.forTesting(
     this.workDirectory, {
     this.dimensions,
     this.verbose = false,
     io.HttpClient? httpClient,
     ProcessManager? processManager,
     StringSink? stderr,
     Map<String, String>? environment,
+    Engine? engineRoot,
   }) : httpClient = httpClient ?? io.HttpClient(),
        process = processManager ?? const LocalProcessManager(),
        _stderr = stderr ?? io.stderr,
-        _environment = environment ?? io.Platform.environment;
+       _environment = environment ?? io.Platform.environment,
+       _engineRoot = engineRoot ?? Engine.findWithin() {
+    // Lookup the release version from the engine repository.
+    final io.File releaseVersionFile = io.File(path.join(
+      _engineRoot.flutterDir.path,
+      '.engine-release.version',
+    ));
+
+    // If the file is not found or cannot be read, we are in an invalid state.
+    try {
+      _releaseVersion = ReleaseVersion.parse(releaseVersionFile.readAsStringSync());
+    } on FormatException catch (error) {
+      throw StateError('Failed to parse release version file: $error.');
+    } on io.FileSystemException catch (error) {
+      throw StateError('Failed to read release version file: $error.');
+    }
+  }
+
+  /// The root of the engine repository.
+  final Engine _engineRoot;
+  ReleaseVersion? _releaseVersion;
 
   /// Whether the client is available and can be used in this environment.
   static bool isAvailable({
@@ -244,6 +309,14 @@ interface class SkiaGoldClient {
   /// determined by the [pixelColorDelta] parameter. It's in the range [0.0,
   /// 1.0] and defaults to 0.01. A value of 0.01 means that 1% of the pixels are
   /// allowed to be different.
+  ///
+  /// ## Release Testing
+  ///
+  /// In release branches, we add a unique test suffix to the test name. For
+  /// example "testName" -> "testName_Release_3_21", based on the version in the
+  /// `.engine-release.version` file at the root of the engine repository.
+  ///
+  /// See <../README.md#release-testing> for more information.
   Future<void> addImg(
     String testName,
     io.File goldenFile, {
@@ -252,6 +325,17 @@ interface class SkiaGoldClient {
     required int screenshotSize,
   }) async {
     assert(_isPresubmit || _isPostsubmit);
+
+    // Clean the test name to remove the file extension.
+    testName = path.basenameWithoutExtension(testName);
+
+    // In release branches, we add a unique test suffix to the test name.
+    // For example "testName" -> "testName_Release_3_21".
+    // See ../README.md#release-testing for more information.
+    if (_releaseVersion case final ReleaseVersion v) {
+      testName = '${testName}_Release_${v.major}_${v.minor}';
+    }
+
     if (_isPresubmit) {
       await _tryjobAdd(testName, goldenFile, screenshotSize, pixelColorDelta, differentPixelsRate);
     }
@@ -287,7 +371,7 @@ interface class SkiaGoldClient {
       '--work-dir',
       _tempPath,
       '--test-name',
-      _cleanTestName(testName),
+      testName,
       '--png-file',
       goldenFile.path,
       // Otherwise post submit will not fail.
@@ -391,7 +475,7 @@ interface class SkiaGoldClient {
       '--work-dir',
       _tempPath,
       '--test-name',
-      _cleanTestName(testName),
+      testName,
       '--png-file',
       goldenFile.path,
       ..._getMatchingArguments(testName, screenshotSize, pixelDeltaThreshold, differentPixelsRate),
@@ -489,7 +573,7 @@ interface class SkiaGoldClient {
 
   /// Returns the current commit hash of the engine repository.
   Future<String> _getCurrentCommit() async {
-    final String engineCheckout = Engine.findWithin().flutterDir.path;
+    final String engineCheckout = _engineRoot.flutterDir.path;
     final io.ProcessResult revParse = await process.run(
       <String>['git', 'rev-parse', 'HEAD'],
       workingDirectory: engineCheckout,
@@ -521,12 +605,6 @@ interface class SkiaGoldClient {
     return json.encode(_getKeys());
   }
 
-  /// Removes the file extension from the [fileName] to represent the test name
-  /// properly.
-  static String _cleanTestName(String fileName) {
-    return path.basenameWithoutExtension(fileName);
-  }
-
   /// Returns a list of arguments for initializing a tryjob based on the testing
   /// environment.
   List<String> _getCIArguments() {