WIP - Gold testing #10753
WIP - Gold testing #10753
Conversation
github-actions
bot
added
p: two_dimensional_scrollables
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new internal package, flutter_goldens, to enable Skia Gold testing for packages within the flutter/packages repository. It adds the necessary client logic, environment-aware comparators, and test configurations, with two_dimensional_scrollables serving as the initial implementation example. My review has identified some critical and high-severity issues, including a syntax error that will prevent compilation, incorrect Skia Gold configuration, and fragile logic for naming golden files. I have also noted several medium-severity issues related to code duplication, debugging artifacts, and dependency management that should be addressed to improve maintainability. Overall, this is a significant and valuable addition, and my feedback aims to ensure its correctness and robustness.
| return Uri.parse( | ||
| <String>[ | ||
| ?namePrefix, | ||
| basedir.pathSegments[1], // ~/packages/<package-name> | ||
| golden.toString(), | ||
| ].join('.'), | ||
| ); |
There was a problem hiding this comment.
The _addPrefix method has a syntax error and fragile logic.
- Syntax Error:
?namePrefixon line 237 is not valid Dart syntax inside a list literal and will cause a compilation failure. You likely intended to use a collection-if:if (namePrefix != null) namePrefix!. - Fragile/Redundant Logic: The method uses
basedir.pathSegments[1]to derive the package name, which is brittle and depends on the execution environment. It also includesnamePrefix, which is already set to the package name influtter_test_config.dart, leading to a duplicated name (e.g.,my_package.my_package.my_golden.png).
A more robust implementation would rely solely on the namePrefix.
return Uri.parse(
<String>[
if (namePrefix != null) namePrefix!,
golden.toString(),
].join('.'),
);| 'imgtest', | ||
| 'init', | ||
| '--instance', | ||
| 'flutter', |
There was a problem hiding this comment.
The Skia Gold instance is hardcoded as 'flutter'. Since this client is for the flutter/packages repository and targets the flutter-packages-gold.skia.org dashboard, the instance name should likely be flutter-packages. Using the wrong instance will cause golden files to be uploaded to the incorrect dashboard.
'flutter-packages',| 'imgtest', | ||
| 'init', | ||
| '--instance', | ||
| 'flutter', |
There was a problem hiding this comment.
The Skia Gold instance is hardcoded as 'flutter'. Since this client is for the flutter/packages repository and targets the flutter-packages-gold.skia.org dashboard, the instance name should likely be flutter-packages. Using the wrong instance will cause golden files to be uploaded to the incorrect dashboard.
'flutter-packages',| dependencies: >- | ||
| [ | ||
| {"dependency": "goldctl", "version": "git_revision:2387d6fff449587eecbb7e45b2692ca0710b63b9"} | ||
| ] |
There was a problem hiding this comment.
The dependencies block for goldctl is duplicated across many targets in this file. To improve maintainability and make future updates to the goldctl version easier, consider using YAML anchors to define this block once and reuse it where needed. Note that this is contingent on the CI system's support for YAML anchors.
Example:
x-gold-dependency: &gold-dependency
dependencies: >-
[
{"dependency": "goldctl", "version": "git_revision:2387d6fff449587eecbb7e45b2692ca0710b63b9"}
]
targets:
- name: Linux analyze
# ...
properties:
<<: *gold-dependency
# ...| infrastructure for tracking golden image tests. | ||
|
|
||
| See also: | ||
|  |
There was a problem hiding this comment.
This line contains an unexpected character  which appears to be a broken link or artifact. Please remove it.
| @protected | ||
| File getGoldenFile(Uri uri) { | ||
| final File goldenFile = fs.directory(basedir).childFile(fs.file(uri).path); | ||
| print(goldenFile); |
There was a problem hiding this comment.
This file contains several print() statements which appear to be for debugging. Please remove them before merging.
| await keys.writeAsString(_getKeysJSON()); | ||
| await failures.create(); | ||
| final String commitHash = await _getCurrentCommit(); | ||
| print(commitHash); |
There was a problem hiding this comment.
This file contains print() statements that appear to be for debugging. Please remove them before merging.
| ..writeln('testing. Golden file images in flutter/flutter are triaged ') | ||
| ..writeln('in pre-submit during code review for the given PR.') | ||
| ..writeln() | ||
| ..writeln('Visit https://flutter-gold.skia.org/ to view and approve ') |
There was a problem hiding this comment.
The error message for unapproved images directs users to https://flutter-gold.skia.org/. Since this client is for flutter/packages, this URL should point to https://flutter-packages-gold.skia.org/.
..writeln('Visit https://flutter-packages-gold.skia.org/ to view and approve ')| dependencies: | ||
| crypto: any | ||
| file: any | ||
| flutter: | ||
| sdk: flutter | ||
| flutter_test: | ||
| sdk: flutter | ||
| path: any | ||
| platform: any | ||
| process: any |
There was a problem hiding this comment.
Using any for dependencies is discouraged as it can pull in breaking changes unexpectedly. Please specify version constraints for your dependencies to ensure reproducible builds.
dependencies:
crypto: ^3.0.3
file: ^7.0.0
flutter:
sdk: flutter
flutter_test:
sdk: flutter
path: ^1.9.0
platform: ^3.1.4
process: ^5.0.21 participant
WIP
The flutter/packages instance of Gold does not have the frontend up yet. I need to validate the backend can receive first.
Still to do here:
Pre-Review Checklist
[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3