diff --git a/.engine-release.version b/.engine-release.version new file mode 100644 index 0000000000000..dd395bfd21045 --- /dev/null +++ b/.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 diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index e040b28546e07..7e4361ef38692 100644 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -41995,6 +41995,7 @@ ORIGIN: ../../../flutter/vulkan/vulkan_window.cc + ../../../flutter/LICENSE ORIGIN: ../../../flutter/vulkan/vulkan_window.h + ../../../flutter/LICENSE TYPE: LicenseType.bsd FILE: ../../../flutter/.clangd +FILE: ../../../flutter/.engine-release.version FILE: ../../../flutter/.pylintrc FILE: ../../../flutter/assets/asset_manager.cc FILE: ../../../flutter/assets/asset_manager.h diff --git a/testing/litetest/lib/src/matchers.dart b/testing/litetest/lib/src/matchers.dart index 5c40eacc093db..ba30a1620cf19 100644 --- a/testing/litetest/lib/src/matchers.dart +++ b/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) { diff --git a/testing/skia_gold_client/README.md b/testing/skia_gold_client/README.md index 8c4c68e19985c..e63486b13b038 100644 --- a/testing/skia_gold_client/README.md +++ b/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 . -## 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:" - } - ], +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: /tools/pkg/engine_repo_tools - skia_gold_client: - 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 main() { - final Directory tmpDirectory = Directory.current.createTempSync('skia_gold_wd'); - final SkiaGoldClient client = SkiaGoldClient( - tmpDirectory, - dimensions: {'': ''}, - ); - - try { - if (SkiaGoldClient.isAvailable()) { - await client.auth(); - - await client.addImg( - '', - 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). diff --git a/testing/skia_gold_client/lib/skia_gold_client.dart b/testing/skia_gold_client/lib/skia_gold_client.dart index e9d28ff504286..9d14d0116f074 100644 --- a/testing/skia_gold_client/lib/skia_gold_client.dart +++ b/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,14 +23,57 @@ 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: { + /// '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? 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, @@ -37,10 +81,31 @@ interface class SkiaGoldClient { ProcessManager? processManager, StringSink? stderr, Map? 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 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 _getCurrentCommit() async { - final String engineCheckout = Engine.findWithin().flutterDir.path; + final String engineCheckout = _engineRoot.flutterDir.path; final io.ProcessResult revParse = await process.run( ['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 _getCIArguments() { diff --git a/testing/skia_gold_client/lib/src/errors.dart b/testing/skia_gold_client/lib/src/errors.dart index d5590091bf46d..830555229d35b 100644 --- a/testing/skia_gold_client/lib/src/errors.dart +++ b/testing/skia_gold_client/lib/src/errors.dart @@ -1,3 +1,7 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + /// Skia Gold errors thrown by intepreting process exits and [stdout]/[stderr]. final class SkiaGoldProcessError extends Error { /// Creates a new [SkiaGoldProcessError] from the provided origin. diff --git a/testing/skia_gold_client/lib/src/release_version.dart b/testing/skia_gold_client/lib/src/release_version.dart new file mode 100644 index 0000000000000..a62953e349c60 --- /dev/null +++ b/testing/skia_gold_client/lib/src/release_version.dart @@ -0,0 +1,130 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:convert'; + +import 'package:meta/meta.dart'; + +/// Signals that the output of golden files may diverge from `main`. +/// +/// By default, Skia Gold will compare the output of a test to the golden file +/// in the `main` branch. During a release branch, the output of the test may +/// diverge (intentionally or accidentally, i.e. a bug) from the output in +/// `main`. +/// +/// This class signals to Skia Gold to use a unique test name for the release. +@immutable +final class ReleaseVersion { + /// Creates a [ReleaseVersion] with the given [major] and [minor] version. + /// + /// For example, for the `3.21` release: + /// ```dart + /// ReleaseVersion( + /// major: 3, + /// minor: 21, + /// ) + /// ``` + /// + /// Each number must be non-negative. + ReleaseVersion({ + required this.major, + required this.minor, + }) { + RangeError.checkNotNegative(major, 'major'); + RangeError.checkNotNegative(minor, 'minor'); + } + + /// Parses a [ReleaseVersion] from the contents of a `release.version` file. + /// + /// Returns `null` if and only if the version is parsed as the string `none`. + /// + /// The format of the file is plaintext, and any lines that are empty + /// (newline characters only) or start with `#` are ignored. The first line + /// that is not ignored must be in the format `major.minor`, where `major` and + /// `minor` are non-negative integers. + /// + /// For example, the following file contents: + /// ```txt + /// # This is a comment + /// + /// 3.21 + /// ``` + /// + /// ... would parse to `ReleaseVersion(major: 3, minior: 21)`. + /// + /// Throws a [FormatException] if the file contents are not in the expected + /// format (either empty/comments only, or missing a `major.minor` line in + /// the format described in [ReleaseVersion.new]). + static ReleaseVersion? parse(String fileContents) { + bool parsedNone = false; + ReleaseVersion? parsed; + for (final String line in LineSplitter.split(fileContents)) { + final String trimmed = line.trim(); + if (trimmed.isEmpty || trimmed.startsWith('#')) { + continue; + } + if (trimmed == 'none') { + parsedNone = true; + continue; + } + final List parts = trimmed.split('.'); + if (parts.length != 2) { + throw FormatException( + 'Expected "major.minor" format, got "$trimmed"', + fileContents, + ); + } + final int? major = int.tryParse(parts[0]); + final int? minor = int.tryParse(parts[1]); + if (major == null || minor == null) { + throw FormatException( + 'Expected "major.minor", each as an integer, got "$trimmed"', + fileContents, + ); + } + if (parsed != null) { + throw FormatException( + 'Multiple "major.minor" versions found', + fileContents, + ); + } + parsed = ReleaseVersion( + major: major, + minor: minor, + ); + } + if (parsed != null && parsedNone) { + throw FormatException( + 'Both "none" and a "major.minor" version found', + fileContents, + ); + } + if (parsed != null) { + return parsed; + } + if (parsedNone) { + return null; + } + throw FormatException('No "major.minor" version found', fileContents); + } + + /// The major version number. + final int major; + + /// The minor version number. + final int minor; + + @override + bool operator ==(Object other) { + return other is ReleaseVersion && + other.major == major && + other.minor == minor; + } + + @override + int get hashCode => Object.hash(major, minor); + + @override + String toString() => '$major.$minor'; +} diff --git a/testing/skia_gold_client/test/release_version_test.dart b/testing/skia_gold_client/test/release_version_test.dart new file mode 100644 index 0000000000000..72f7e73a31f53 --- /dev/null +++ b/testing/skia_gold_client/test/release_version_test.dart @@ -0,0 +1,68 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'package:litetest/litetest.dart'; +import 'package:skia_gold_client/src/release_version.dart'; + +void main() { + test('should accept a major and minor version', () { + final ReleaseVersion version = ReleaseVersion(major: 3, minor: 21); + expect(version.major, equals(3)); + expect(version.minor, equals(21)); + }); + + test('should accept the string "none"', () { + final ReleaseVersion? version = ReleaseVersion.parse('none'); + expect(version, isNull); + }); + + test('should fail on a negative major version', () { + expect(() => ReleaseVersion(major: -1, minor: 0), throwsRangeError); + }); + + test('should fail on a negative minor version', () { + expect(() => ReleaseVersion(major: 0, minor: -1), throwsRangeError); + }); + + test('should parse a release version from a file', () { + final ReleaseVersion version = ReleaseVersion.parse('3.21')!; + expect(version.major, equals(3)); + expect(version.minor, equals(21)); + }); + + test('should ignore comments and empty lines', () { + final ReleaseVersion version = ReleaseVersion.parse([ + '# This is a comment', + '', + '3.21', + '', + ].join('\n'))!; + expect(version.major, equals(3)); + expect(version.minor, equals(21)); + }); + + test('should fail on a missing version line', () { + expect(() => ReleaseVersion.parse(''), throwsFormatException); + }); + + test('should fail on a malformed version line', () { + expect(() => ReleaseVersion.parse('3.21.0'), throwsFormatException); + }); + + test('should fail on a non-integer major version', () { + expect(() => ReleaseVersion.parse('a.21'), throwsFormatException); + }); + + test('should fail on a non-integer minor version', () { + expect(() => ReleaseVersion.parse('3.b'), throwsFormatException); + }); + + test('should fail on multiple version lines', () { + expect(() => ReleaseVersion.parse('3.21\n3.22'), throwsFormatException); + }); + + test('should fail if both "none" and a version are present', () { + expect(() => ReleaseVersion.parse('none\n3.21'), throwsFormatException); + }); +} diff --git a/testing/skia_gold_client/test/skia_gold_client_test.dart b/testing/skia_gold_client/test/skia_gold_client_test.dart index 1f91d8a35231a..b3265866da3da 100644 --- a/testing/skia_gold_client/test/skia_gold_client_test.dart +++ b/testing/skia_gold_client/test/skia_gold_client_test.dart @@ -7,10 +7,12 @@ import 'dart:convert'; import 'dart:io' as io; import 'dart:typed_data'; +import 'package:engine_repo_tools/engine_repo_tools.dart'; import 'package:litetest/litetest.dart'; import 'package:path/path.dart' as p; import 'package:process_fakes/process_fakes.dart'; import 'package:skia_gold_client/skia_gold_client.dart'; +import 'package:skia_gold_client/src/release_version.dart'; void main() { /// A mock commit hash that is used to simulate a successful git call. @@ -44,13 +46,15 @@ void main() { SkiaGoldClient createClient( _TestFixture fixture, { required Map environment, + ReleaseVersion? engineVersion, Map? dimensions, bool verbose = false, io.ProcessResult Function(List command) onRun = _runUnhandled, }) { - return SkiaGoldClient( + return SkiaGoldClient.forTesting( fixture.workDirectory, dimensions: dimensions, + engineRoot: Engine.fromSrcPath(fixture.engineSrcDir.path), httpClient: fixture.httpClient, processManager: FakeProcessManager( onRun: onRun, @@ -237,6 +241,58 @@ void main() { } }); + test('addImg [pre-submit] executes successfully with a release version', () async { + // Adds a suffix of "_Release_3_21" to the test name. + final _TestFixture fixture = _TestFixture( + // Creates a file called "engine/src/fluter/.engine-release.version" with the contents "3.21". + engineVersion: ReleaseVersion( + major: 3, + minor: 21, + ), + ); + try { + final SkiaGoldClient client = createClient( + fixture, + environment: presubmitEnv, + onRun: (List command) { + if (command case ['git', ...]) { + return io.ProcessResult(0, 0, mockCommitHash, ''); + } + if (command case ['python tools/goldctl.py', 'imgtest', 'init', ...]) { + return io.ProcessResult(0, 0, '', ''); + } + expect(command, [ + 'python tools/goldctl.py', + 'imgtest', + 'add', + '--work-dir', + p.join(fixture.workDirectory.path, 'temp'), + '--test-name', + // This is the significant change. + 'test-name_Release_3_21', + '--png-file', + p.join(fixture.workDirectory.path, 'temp', 'golden.png'), + '--add-test-optional-key', + 'image_matching_algorithm:fuzzy', + '--add-test-optional-key', + 'fuzzy_max_different_pixels:10', + '--add-test-optional-key', + 'fuzzy_pixel_delta_threshold:0', + ]); + return io.ProcessResult(0, 0, '', ''); + }, + ); + + await client.addImg( + 'test-name.foo', + io.File(p.join(fixture.workDirectory.path, 'temp', 'golden.png')), + screenshotSize: 1000, + ); + } finally { + fixture.dispose(); + } + }); + test('addImg [pre-submit] executes successfully with verbose logging', () async { final _TestFixture fixture = _TestFixture(); try { @@ -536,14 +592,32 @@ void main() { } final class _TestFixture { - _TestFixture(); + _TestFixture({ + ReleaseVersion? engineVersion, + }) { + workDirectory = rootDirectory.createTempSync('working'); + + // Create the engine/src directory. + engineSrcDir = io.Directory(p.join(rootDirectory.path, 'engine', 'src')); + engineSrcDir.createSync(recursive: true); + + // Create a .engine-release.version file in the engine root. + final io.Directory flutterDir = io.Directory(p.join(engineSrcDir.path, 'flutter')); + flutterDir.createSync(recursive: true); + + final String version = engineVersion?.toString() ?? 'none'; + io.File(p.join(flutterDir.path, '.engine-release.version')).writeAsStringSync(version); + } + + final io.Directory rootDirectory = io.Directory.systemTemp.createTempSync('skia_gold_client_test'); + late final io.Directory workDirectory; + late final io.Directory engineSrcDir; - final io.Directory workDirectory = io.Directory.systemTemp.createTempSync('skia_gold_client_test'); final _FakeHttpClient httpClient = _FakeHttpClient(); final StringSink outputSink = StringBuffer(); void dispose() { - workDirectory.deleteSync(recursive: true); + rootDirectory.deleteSync(recursive: true); } }