This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Refactor golden_tests_harvester, throw when not --dry-run, add tests.
#51364
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gaaclarke
approved these changes
Mar 12, 2024
| ## Usage | ||
|
|
||
| This program assumes you've _already run_ a suite of golden tests that produce | ||
| a directory of images, of which the format, a JSON file named `digests.json`, is |
Member
There was a problem hiding this comment.
Suggested change
| a directory of images, of which the format, a JSON file named `digests.json`, is | |
| a directory of images with a JSON digest named `digests.json`, which is |
| /// } | ||
| /// ``` | ||
| /// | ||
| /// If [dryRun] is true, the images will _not_ be uploaded to Skia Gold, and |
Member
There was a problem hiding this comment.
dryRun is not apropos to harvest, I think you refactored this and didn't yet update the docstring.
Contributor
Author
There was a problem hiding this comment.
Right, thanks for catching the refactor!
| /// Other tools (perhaps implemented in other languages, like C++) can use this | ||
| /// format to communicate with the `golden_tests_harvester` tool without | ||
| /// relying on the tool directly (or the Dart SDK). | ||
| final class Digests { |
Member
There was a problem hiding this comment.
This code seems like the sort of thing we should be automating, can we use the dart json parser to do this? We would probably need custom builders which is a showstopper, eh?
Contributor
Author
There was a problem hiding this comment.
I am guessing (?) this format will virtually never change, or if it does, we'll catch it in a unit test.
I am not opposed to generating this, but it wouldn't be a priority for me.
| test('should require a file named "digests.json" in the working directory', () async { | ||
| await withTempDirectory((io.Directory tempDirectory) async { | ||
| final StringSink stderr = StringBuffer(); | ||
| try { |
Member
There was a problem hiding this comment.
You use this pattern a few times. You could factor this out to
expectThrow<io.FileSystemException>((){ await harvest(...);}, (io.FileExceptionException e) {
expect(e.path, contains('digest.json'));
});
Contributor
Author
There was a problem hiding this comment.
Reasonable suggestion, done!
matanlurey
added
the
autosubmit
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Mar 13, 2024
auto-submit bot
pushed a commit
to flutter/flutter
that referenced
this pull request
Mar 13, 2024
…145067) flutter/engine@285b9fb...2871c26 2024-03-13 [email protected] Add et lint command (flutter/engine#51238) 2024-03-13 [email protected] Refactor `golden_tests_harvester`, throw when not `--dry-run`, add tests. (flutter/engine#51364) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes flutter/flutter#144948.
I went a little farther, as I'm not looking forward to debugging this in the future without tests or examples.