-
Notifications
You must be signed in to change notification settings - Fork 28k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use flutter
repo for engine golds instead of flutter-engine
.
#160556
Use flutter
repo for engine golds instead of flutter-engine
.
#160556
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have to make this change? Isn't this going to invalidate all of our history? I don't know if there is any benefit of starting to upload our goldens to the flutter golden endpoint.
Engine golden files in flutter/flutter monorepo land will not be able to use the engine instance of Gold. Gold instances are correlated with a repo and track commits only from that repo. |
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR keeps failing in merge queue despite passing in presubmit. Example failure, 1 of ~200: stdout: Given image with hash 3f5895ca785071873070f39b6ee018d2 for test engine.canvas_test_toImage
Fetching most recent positive digest for trace with ID "d378d6587de23311f96d6cbdf6daa26f".
No recent positive digests for trace with ID "d378d6587de23311f96d6cbdf6daa26f". This probably means that the test was newly added.
Non-exact image comparison using algorithm "fuzzy" against most recent positive digest "".
Untriaged or negative image The same test in the presubmit reports the same failure: NOTE: Untriaged image detected in tryjob.
Triage should be required by the "Flutter Gold" check
stdout:
Given image with hash 43af40a9baa67b933cde8f2dac7f28ac for test engine.canvas_test_toImage
Expectation for test: 3f5895ca785071873070f39b6ee018d2 (positive)
Fetching most recent positive digest for trace with ID "3c059772c4ee1ae45c84a335b2849854".
No recent positive digests for trace with ID "3c059772c4ee1ae45c84a335b2849854". This probably means that the test was newly added.
Non-exact image comparison using algorithm "fuzzy" against most recent positive digest "".
Untriaged or negative image ... but the It's worth noting I see the "double patchset" which in the past has meant bad state has occurred somehow: |
45cfe94
to
11f80eb
Compare
… `master`. (#161187) Unblocks #160556. Currently the merge queue runs in post-submit mode, which looks for (and fails to find) digests for PRs that have not yet been submitted, blocking the engine goldens from being enabled/checked in #160556. This PR is a proposal to fix that by skipping the tests. We might decide instead that tests should not be running in the merge queue at all, in which case we will _not_ merge this PR and will change how the merge queue works instead. Otherwise this PR is a proof of concept of [aligning implementations with the framework](https://github.com/flutter/flutter/blob/a9b3f6c042d2c24d183a5d0cdc616dab00b8e700/packages/flutter_goldens/lib/flutter_goldens.dart#L338).
Closes #157206.
I also added a
prefix
that will default toengine.
to avoid accidentally stomping on golden names across repos./cc @gaaclarke for visibility, @Piinks for visibility.
(I would love to get rid of this "engine copy" of the client as part of longer-term mono repo deduplication).