Skip to content
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

Merged

Conversation

matanlurey
Copy link
Contributor

Closes #157206.

I also added a prefix that will default to engine. 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).

@matanlurey matanlurey requested a review from jtmcdole December 19, 2024 00:11
@github-actions github-actions bot added the engine flutter/engine repository. See also e: labels. label Dec 19, 2024
Copy link
Member

@gaaclarke gaaclarke left a 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.

@Piinks
Copy link
Contributor

Piinks commented Dec 19, 2024

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.

@flutter-dashboard
Copy link

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 package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #160556 at sha 2564473

@flutter-dashboard flutter-dashboard bot added the will affect goldens Changes to golden files label Dec 20, 2024
Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@matanlurey matanlurey added this pull request to the merge queue Dec 20, 2024
@matanlurey matanlurey removed this pull request from the merge queue due to a manual request Dec 20, 2024
@matanlurey matanlurey added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 23, 2024
@auto-submit auto-submit bot added this pull request to the merge queue Dec 23, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 24, 2024
@flutter-dashboard flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Dec 24, 2024
Copy link
Contributor

@jtmcdole jtmcdole left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@matanlurey matanlurey added this pull request to the merge queue Dec 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 26, 2024
@matanlurey matanlurey added this pull request to the merge queue Dec 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 26, 2024
@matanlurey
Copy link
Contributor Author

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 flutter-gold check is not reporting anything.

It's worth noting I see the "double patchset" which in the past has meant bad state has occurred somehow:

Screenshot 2024-12-26 at 3 01 29 PM

@matanlurey matanlurey force-pushed the use-flutter-repo-for-engine-golds branch from 45cfe94 to 11f80eb Compare December 26, 2024 23:03
github-merge-queue bot pushed a commit that referenced this pull request Jan 7, 2025
… `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).
@matanlurey matanlurey added this pull request to the merge queue Jan 7, 2025
Merged via the queue into flutter:master with commit cda3515 Jan 7, 2025
176 of 179 checks passed
@matanlurey matanlurey deleted the use-flutter-repo-for-engine-golds branch January 7, 2025 04:13
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 8, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 8, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 8, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
engine flutter/engine repository. See also e: labels. will affect goldens Changes to golden files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change engine/testing/skia_gold_client to the flutter environment
5 participants