Don't send images to Gold on release branches #139706

Piinks · 2023-12-07T00:18:52Z

Part of fixing #139673, it will need to be cherry picked into the stable branch to fully resolve.

Originally I tried to come at this from ci.yaml, but the syntax does not exist to either conditionally include a dependency based on the branch we are on, or to disable a shard for a given branch (as opposed to enabling it which is supported). I could double every CI shard that uses Gold to try to serve my purpose, but it is already a very large and cumbersome file to keep up to date. Doubling it does not feel like the best solution. Using a RegEx is not my favorite, but I am using the same one we use in our CI, and left a note there to update it if it should ever change. Since there is already a whole infra built around it, I feel it is pretty safe so we can fix the stable tree.

We already had mitigated Gold affecting release branches in the past through flutter/cocoon (#58814), but #139673 exposed a rather rare edge case. A change was CP'd into the stable branch that introduced golden file image changes. Typically this would not cause an issue since any change that has landed on the master branch has golden files accounted for. In this case, the CP'd change on master has generated a different image on canvaskit due to another change that was not on stable. So when the CP landed, it generated a new image Gold had never seen before. Gold only tracks the master branch, so we cannot approve the image, and so cannot fix the stable tree.

This would disable the failing check on release branches and fix the tree.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

CaseyHillers

Thanks for the quick fix! I left a comment for something to consider, otherwise LGTM

CaseyHillers · 2023-12-07T18:12:18Z

packages/flutter_goldens/lib/flutter_goldens.dart

@@ -19,6 +19,7 @@ export 'package:flutter_goldens_client/skia_client.dart';
 // https://github.com/flutter/flutter/wiki/Writing-a-golden-file-test-for-package%3Aflutter

 const String _kFlutterRootKey = 'FLUTTER_ROOT';
+final RegExp _kReleaseBranch = RegExp(r'flutter-\d+\.\d+-candidate\.\d+');


Instead of thinking this as release branches, what if we change it to only look at master|main? It seems unlikely we want to run goldens anywhere that isn't HEAD.

Unfortunately Luci does not give us that kind of info. From this place in the test execution, I only have the environment that Luci has set up to work with.
I could try targeting master/stable/beta in ci.yaml, but it would double the amount of recipes, which feels super brittle for maintaining which one is which and how its configured.

To clarify, I mean checking ENV['GIT_BRANCH'] == RegEx('master|main')

Oo, let me check a PR to see if this works. I think in presubmit checks it will reflect whatever the contributor named the branch.

TIL! Looks like in post and presubmit the branch reflects master, that is way better - thank you!

Part of fixing flutter#139673, it will need to be cherry picked into the stable branch to fully resolve. Originally I tried to come at this from `ci.yaml`, but the syntax does not exist to either conditionally include a dependency based on the branch we are on, or to disable a shard for a given branch (as opposed to enabling it which is supported). I could double every CI shard that uses Gold to try to serve my purpose, but it is already a very large and cumbersome file to keep up to date. Doubling it does not feel like the best solution. Using a RegEx is not my favorite, but I am using the same one we use in our CI, and left a note there to update it if it should ever change. Since there is already a whole infra built around it, I feel it is pretty safe so we can fix the stable tree. We already had mitigated Gold affecting release branches in the past through flutter/cocoon (flutter#58814), but flutter#139673 exposed a rather rare edge case. A change was CP'd into the stable branch that introduced golden file image changes. Typically this would not cause an issue since any change that has landed on the master branch has golden files accounted for. In this case, the CP'd change on master has generated a different image on canvaskit due to another change that was not on stable. So when the CP landed, it generated a new image Gold had never seen before. Gold only tracks the master branch, so we cannot approve the image, and so cannot fix the stable tree. This would disable the failing check on release branches and fix the tree.

Fixes #139673 Cherry picks #139706 to the stable branch to fix the tree.

…r#139706)

Hixie · 2023-12-08T22:21:30Z

packages/flutter_goldens/lib/flutter_goldens.dart

-      || platform.environment.containsKey('CIRRUS_CI');
+      || platform.environment.containsKey('CIRRUS_CI'))
+      // If we are in CI, skip on branches that are not main.
+      && !_kMainBranch.hasMatch(platform.environment['GIT_BRANCH'] ?? '');


this doesn't seem right? it makes us use the local file comparator on the main branch on Cirrus, for example, whereas before we used the skipping one.

(i'll fix it in my PR, let me know if i misunderstood this)

Oh. Well gosh. Yeah that's wrong, and obviously not covered by a test.
We don't actually run these tests on Cirrus at all anymore. I had the thought to clean everything Cirrus related out while I was here, but didn't want to make merging with your PR even worse.

This reverts commit 91ea024.

* [CP] Gold fix for stable branch (flutter#139764) Fixes flutter#139673 Cherry picks flutter#139706 to the stable branch to fix the tree. * [macOS] Suppress Xcode 15 createItemModels warning (flutter#138243) (flutter#139782) As of Xcode 15 on macOS Sonoma, the following message is (repeatedly) output to stderr during builds (repros on non-Flutter apps). It is supppressed in xcode itself, but not when run from the command-line. ``` 2023-11-10 10:44:58.031 xcodebuild[61115:1017566] [MT] DVTAssertions: Warning in /System/Volumes/Data/SWE/Apps/DT/BuildRoots/BuildRoot11/ActiveBuildRoot/Library/Caches/com.apple.xbs/Sources/IDEFrameworks/IDEFrameworks-22267/IDEFoundation/Provisioning/Capabilities Infrastructure/IDECapabilityQuerySelection.swift:103 Details: createItemModels creation requirements should not create capability item model for a capability item model that already exists. Function: createItemModels(for:itemModelSource:) Thread: <_NSMainThread: 0x6000027c0280>{number = 1, name = main} Please file a bug at https://feedbackassistant.apple.com with this warning message and any useful information you can provide. ``` This suppresses this message from stderr in our macOS build logs. Issue: flutter#135277 Cherry-pick: flutter#139284 https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [Features we expect every widget to implement]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat *Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.* *List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.* *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* * [CP] have `Java.version` return null if `java --version` fails or cannot be run (flutter#139698) cherry-picks changes from flutter#139614 onto the stable channel * [CP] Catch error for missing directory in `FontConfigManager` (flutter#138496) (flutter#139743) Closes: - flutter#138434 We will catch any errors while attempting to clear the temp directories that don't exist for the `FontConfigManager` class --------- Co-authored-by: Kate Lovett <[email protected]> Co-authored-by: Chris Bracken <[email protected]> Co-authored-by: Andrew Kolos <[email protected]> Co-authored-by: Elias Yishak <[email protected]>

…r#139706)

Don't run Gold on releases

327288d

Piinks requested a review from CaseyHillers December 7, 2023 00:18

github-actions bot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. labels Dec 7, 2023

CaseyHillers approved these changes Dec 7, 2023

View reviewed changes

Switch to main|master instead

612cdbe

Piinks requested a review from CaseyHillers December 7, 2023 19:05

CaseyHillers approved these changes Dec 7, 2023

View reviewed changes

Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 7, 2023

auto-submit bot merged commit 91ea024 into flutter:master Dec 7, 2023
62 checks passed

Piinks mentioned this pull request Dec 7, 2023

[CP] Gold fix for stable branch #139764

Merged

8 tasks

auto-submit bot pushed a commit that referenced this pull request Dec 7, 2023

[CP] Gold fix for stable branch (#139764)

54f1b88

Fixes #139673 Cherry picks #139706 to the stable branch to fix the tree.

engine-flutter-autoroll mentioned this pull request Dec 8, 2023

Roll Flutter from f35d049e0d8e to c67416140413 (85 revisions) flutter/packages#5616

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 8, 2023

91ea024 Don't send images to Gold on release branches (flutter/flutte…

dd349b1

…r#139706)

engine-flutter-autoroll mentioned this pull request Dec 8, 2023

Roll Flutter from f35d049e0d8e to e070417afeb6 (86 revisions) flutter/packages#5617

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 8, 2023

91ea024 Don't send images to Gold on release branches (flutter/flutte…

b605af2

…r#139706)

engine-flutter-autoroll mentioned this pull request Dec 8, 2023

Roll Flutter from f35d049e0d8e to 6142e7cbdbc1 (87 revisions) flutter/packages#5618

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 8, 2023

91ea024 Don't send images to Gold on release branches (flutter/flutte…

0f5d564

…r#139706)

engine-flutter-autoroll mentioned this pull request Dec 8, 2023

Roll Flutter from f35d049e0d8e to 0c0b77f34274 (88 revisions) flutter/packages#5620

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 8, 2023

91ea024 Don't send images to Gold on release branches (flutter/flutte…

b1bc503

…r#139706)

engine-flutter-autoroll mentioned this pull request Dec 8, 2023

Roll Flutter from f35d049e0d8e to 82bc9ca3273b (89 revisions) flutter/packages#5621

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 8, 2023

91ea024 Don't send images to Gold on release branches (flutter/flutte…

3190bc6

…r#139706)

engine-flutter-autoroll mentioned this pull request Dec 8, 2023

Roll Flutter from f35d049e0d8e to 3df9d4423348 (90 revisions) flutter/packages#5622

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 8, 2023

91ea024 Don't send images to Gold on release branches (flutter/flutte…

cde640c

…r#139706)

engine-flutter-autoroll mentioned this pull request Dec 8, 2023

Roll Flutter from f35d049e0d8e to a5acec1b8d30 (91 revisions) flutter/packages#5624

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 8, 2023

91ea024 Don't send images to Gold on release branches (flutter/flutte…

f6a588c

…r#139706)

engine-flutter-autoroll mentioned this pull request Dec 8, 2023

Roll Flutter from f35d049e0d8e to c642f4e012f4 (92 revisions) flutter/packages#5626

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 8, 2023

91ea024 Don't send images to Gold on release branches (flutter/flutte…

f07f044

…r#139706)

Piinks mentioned this pull request Dec 8, 2023

Make tests more resilient to Skia gold failures and refactor flutter_goldens for extensive technical debt removal #139549

Merged

Hixie reviewed Dec 8, 2023

View reviewed changes

Hixie added a commit to Hixie/flutter that referenced this pull request Dec 12, 2023

Revert "Don't send images to Gold on release branches (flutter#139706)"

b2ad1b7

This reverts commit 91ea024.

Hixie added a commit to Hixie/flutter that referenced this pull request Dec 12, 2023

Merge in flutter#139706

b7e1a39

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2024

91ea024 Don't send images to Gold on release branches (flutter/flutte…

a231fdd

…r#139706)

Piinks mentioned this pull request Mar 22, 2024

Fix skipping golden comparator for CI environments #145619

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't send images to Gold on release branches #139706

Don't send images to Gold on release branches #139706

Piinks commented Dec 7, 2023

CaseyHillers left a comment

CaseyHillers Dec 7, 2023

Piinks Dec 7, 2023

CaseyHillers Dec 7, 2023

Piinks Dec 7, 2023

Piinks Dec 7, 2023

Hixie Dec 8, 2023

Hixie Dec 8, 2023

Piinks Dec 8, 2023

Don't send images to Gold on release branches #139706

Don't send images to Gold on release branches #139706

Conversation

Piinks commented Dec 7, 2023

Pre-launch Checklist

CaseyHillers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment