Treat `flutter-gold` failures in `flutter/engine` as failures. #3487

matanlurey · 2024-02-13T05:06:41Z

In practice this reverts the functionality in flutter/flutter#123979 (#2585). Sorry for the back-and-forth, we tried this model but ended up with (currently) ~170 untriaged failures, so we want to go back to treating these as failures and improve our tools/process if needed instead.

/cc @jonahwilliams and @Piinks

Piinks

Definitely check with @mdebbar, I am less familiar here.

I think if the expectation that the engine sheriff will triage images is not working, then this is a good option. It is how we treat the same situation in the framework.

Alternatively, I think Gold has an endpoint to see "are there unapproved images at head?" If we wanted to try making ownership by the sheriff work, we could have a separate check or alert that notifies them. IIRC that endpoint had rate limitations though. @kjlubick may be able to expand more on this. :)

kjlubick · 2024-02-13T14:29:19Z

https://flutter-engine-gold.skia.org/search is the best way to see all unapproved images at head. It should be reasonably performant (as well as the underlying RPC call, but please try to avoid calling those directly, since we may break you).

I think the rate limitation thing @Piinks was thinking of was related to the changes for a particular PR - we use some extra caching for HEAD that isn't there for PRs.

mdebbar · 2024-02-13T14:41:26Z

I'm not familiar with the recent developments in the engine golden story. But one thing that came to mind a couple days ago as I was looking into some golden problems was this:

cocoon/app_dart/lib/src/request_handlers/push_gold_status_to_github.dart

Lines 122 to 127 in f163e34

    
           // Framework shards run framework goldens 
        
           // Web shards run web version of framework goldens 
        
           // Misc shard runs API docs goldens 
        
           if (name.contains('framework') || name.contains('web engine') || name.contains('misc')) { 
        
             runsGoldenFileTests = true; 
        
           }

I think that if condition is stale and we might be skipping the golden checks on some PRs that DO introduce golden changes. Thoughts on removing the condition and doing golden checks on ALL PRs? (cc @Piinks and @kjlubick in case this breaks some Gold API quota?)

kjlubick · 2024-02-13T14:44:19Z

I would like a quick write up on which RPCs would be affected on the Gold side before making a call about quotas.

Piinks · 2024-02-13T16:08:00Z

I would like a quick write up on which RPCs would be affected on the Gold side before making a call about quotas.

@kjlubick This doc lists all the RPCs we use, where and how. It is linked here in the wiki: https://github.com/flutter/flutter/wiki/Writing-a-golden-file-test-for-package:flutter and cane be used to write this up if we want to make changes.

I think that if condition is stale and we might be skipping the golden checks on some PRs that DO introduce golden changes. Thoughts on removing the condition and doing golden checks on ALL PRs? (cc @Piinks and @kjlubick in case this breaks some Gold API quota?)

@mdebbar That's super intentional (#672 for context).

Contributors complained that they did not want to be bothered with a flutter-gold check on their PR if the change had nothing to do with goldens. And I mean, they felt really really strongly about this.
By not running the flutter-gold check on PRs that do not execute golden file tests, we were able to reduce the amount of calls to Gold significantly. Changing that back would greatly increase traffic.

If the engine has deviated from the expected pattern for naming test shards that execute golden file tests, then that's something we can easily fix.

matanlurey · 2024-02-13T17:06:17Z

Just so I understand correctly @Piinks, the configuration pointed out by @mdebbar is flutter/flutter specific, right?

matanlurey · 2024-02-13T17:08:18Z

Also while I appreciate the chat here, I'd love to still get this PR approved/landed unless I'm missing something?

ricardoamador · 2024-02-13T17:37:29Z

auto_submit/lib/validations/ci_successful.dart

  }
-
-  // Treat any GitHub check run as stale if created over [Config.kGitHubCheckStaleThreshold] hours ago.
-  bool isStale(DateTime dateTime) {


I am not sure about the removal of this here. @keyonghan what do you think?

If I recall correctly we are trying to flag PRs that have checkruns over 2 hours old so I believe this code needs to stay if you are only removing stuff related to flutter-gold.

This was my bad, I had branched from a stale PR. Reverted.

ricardoamador · 2024-02-13T17:39:15Z

auto_submit/lib/validations/ci_successful.dart

      final String? name = status.context;

-      // If the account author is a roller account do not block merge on flutter-gold check.
-      if (isToEngineRoller(author, slug) && name == 'flutter-gold') {


I think this is the only code that needs to be removed here to avoid skipping and enforcing the failure.

Ah thinks Ricardo, this is my bad - it looks like I had an older branch. Let me redo this fix and tag you.

ricardoamador · 2024-02-13T17:47:40Z

Overall I think this is good but I would like to get @keyonghan to look over the stale checkRun flagging as I am not sure if he is tracking metrics there or we have alerting around that.

matanlurey · 2024-02-13T17:53:08Z

Overall I think this is good but I would like to get @keyonghan to look over the stale checkRun flagging as I am not sure if he is tracking metrics there or we have alerting around that.

Thanks @ricardoamador. I actually made a mistake and branched from an older main locally.

The change to ci_successful.dart should be ~1-line-ish, re-upped.

Piinks · 2024-02-13T17:54:49Z

Just so I understand correctly @Piinks, the configuration pointed out by @mdebbar is flutter/flutter specific, right?

No. That whole file is the flutter-gold implementation intended to cover flutter/flutter and flutter/engine.

ricardoamador

LGTM, thanks @matanlurey !

yjbanov · 2024-02-13T20:44:01Z

A note about the 170 untriaged web goldens. They all come from Safari + HTML renderer. All of them exhibit bimodality. There are exactly 2 golden variants per test. One hypothesis from @eyebrowsoffire is that our macOS fleet is made of Arm and x64 machines, and our tests get one of them randomly. If Safari is sensitive to Arm vs x64, that would lead to this behavior. Essentially, you send a PR, triage goldens, then submit. However, what you just did is triage a random combination of Arm and x64 goldens. After the PR is submitted, the post-submit instance of LUCI will again randomize the bots that run web tests, and report different golden outcomes in post-submit.

matanlurey · 2024-02-13T20:46:21Z

Thanks for explaining! I'm happy to hold this PR until we have consensus on how to handle those diffs.

jonahwilliams · 2024-02-13T21:01:45Z

Goldens are allowed to have multiple images per configuration, so if it was truly bimodal (only), then you'd eventually triage all of them.

matanlurey · 2024-02-16T22:52:41Z

I don't have consensus on how to land this yet, so holding off until I chat more with the teams that will be impacted (I'll loop in infra as well).

Treat flutter-gold failures in flutter/engine as failures.

66d506f

matanlurey requested a review from zanderso February 13, 2024 05:06

matanlurey requested a review from ricardoamador as a code owner February 13, 2024 05:06

Piinks reviewed Feb 13, 2024

View reviewed changes

ricardoamador reviewed Feb 13, 2024

View reviewed changes

++

51ff793

++

8637c07

ricardoamador approved these changes Feb 13, 2024

View reviewed changes

matanlurey added 2 commits February 13, 2024 10:41

Dartfmt.

a700748

++

f43689c

matanlurey requested a review from mdebbar February 13, 2024 19:18

matanlurey mentioned this pull request Feb 16, 2024

Engine's Skia Gold has 100+ untriaged results every day flutter/flutter#143591

Closed

mdebbar approved these changes Feb 20, 2024

View reviewed changes

matanlurey added the autosubmit Merge PR when tree becomes green via auto submit App. label Feb 20, 2024

auto-submit bot merged commit eff005a into flutter:main Feb 20, 2024

Treat flutter-gold failures in flutter/engine as failures. #3487

Treat flutter-gold failures in flutter/engine as failures. #3487

Uh oh!

Conversation

matanlurey commented Feb 13, 2024

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

kjlubick commented Feb 13, 2024

Uh oh!

mdebbar commented Feb 13, 2024

Uh oh!

kjlubick commented Feb 13, 2024

Uh oh!

Piinks commented Feb 13, 2024

Uh oh!

matanlurey commented Feb 13, 2024

Uh oh!

matanlurey commented Feb 13, 2024

Uh oh!

ricardoamador Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

ricardoamador Feb 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matanlurey Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

ricardoamador Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

matanlurey Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

ricardoamador commented Feb 13, 2024

Uh oh!

matanlurey commented Feb 13, 2024

Uh oh!

Piinks commented Feb 13, 2024

Uh oh!

ricardoamador left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yjbanov commented Feb 13, 2024

Uh oh!

matanlurey commented Feb 13, 2024

Uh oh!

jonahwilliams commented Feb 13, 2024

Uh oh!

matanlurey commented Feb 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Treat `flutter-gold` failures in `flutter/engine` as failures. #3487

Treat `flutter-gold` failures in `flutter/engine` as failures. #3487

ricardoamador Feb 13, 2024 •

edited

Loading

ricardoamador left a comment •

edited

Loading