Repository: flutter
Issue: Re-evaluate workflow of marking tests as "passed" on untriaged or negative digests
Labels: c: proposal,P2,c: tech-debt,team-engine,triaged-engine

_Would solve https://github.com/flutter/flutter/issues/145043._

Major edits:
- Made it clear the workflow/gardening concern is specific to the `flutter/engine` roller (and rolls into the engine)

## How Flutter Gold Works

Currently, our Skia Gold Client implementations ([engine](https://github.com/flutter/engine/blob/2253387ca30ef4f22e16234264493ba3d5f781a9/testing/skia_gold_client/lib/skia_gold_client.dart#L418-L420), [framework](https://github.com/flutter/flutter/blob/2fde3ac38b6e162037e87d83e875ef08e3306917/packages/flutter_goldens/lib/skia_client.dart#L349-L351)) share the following logic:

```dart
final String resultStdout = result.stdout.toString();
if (result.exitCode != 0 &&
    !(resultStdout.contains('Untriaged') || resultStdout.contains('negative image'))) {
  /* ... */
```

In short, that means that if a call to `gold_ctl.py` returns a non-zero exit code (typically a universal signal for a failure), we instead (silently, though I'll send PRs changing that) swallow the failure, and allow the caller to treat the test as passing.

As a practical example, imagine the following workflow:

1. We decide that straight-edge buttons are so 2010s, and rounded-edge buttons are the new hotness.
1. User `Octocat` makes a change to the `Button` widget, it now renders rounded-edge buttons.
1. The `button_test.dart`, which uses `SkiaGoldClient` takes a screenshot and uploads it to Skia Gold.
1. The test is treated as _passing_ (no expectations failed).
1. Later, the [`Flutter Gold`](https://github.com/flutter/flutter/wiki/Writing-a-golden-file-test-for-package:flutter) check produces an alert, asking for 1+ images to be untriaged.

In other words, at any given moment, a test that would _fail_ (on post-submit), is (intentionally) marked as _passing_ on pre-submit.

## Why Is It Running This Way?

_See https://github.com/flutter/flutter/issues/48744 for the original context._

There are a number of auto-rollers (see https://bdero.me/rollerdash/) in use by the Flutter team:

- `skia-flutter-autoroll` (external -> engine)
- `clang-flutter-engine` (external -> engine)
- `dart-sdk-flutter-engine` (external -> engine)
- `flutter-engine-flutter-autoroll` (engine -> framework)
- `flutter-plugins-flutter-autoroll` (framework -> packages)
- ... and so on.

> _These bots use the Skia Roller service, which, which has received minor customizations for Flutter's workflow in the past._

In the past, before we had the setup discussed above, it is possible, either intentionally (i.e. optimizations or intentional changes to rendering) or unintentionally (flakes) that an automated "roll" (incrementing the vended dependency into another repository) would stall and new PRs would be created over and over attempting to roll, because of untriaged digests.

As a result, **engine** gardeners had to spend more time watching the rollers (which AFAICT, was largely a manual, pull-based process), and it was frequently possible where 100s of commits would get backed up. The Skia Roller service is fairly limited, for example it does not have configuration to specify a "roll a maximum of N commits at a time", so when 100s of commits stacked up, the  **engine** gardeners had to resort to (time-consuming) manual rolls in order to incrementally land a long-stalled roll.

The changes above simplified things for the rollers - now only test _assertion_ failures (or crashes) cause the roller PRs to be continuously closed and re-opened, and due to the "pending" behavior of the Flutter Gold check, the roller PR "stays open" to signal that it's waiting for manual (human) triage.

## What's Next

It is within scope to consider:

- Improving the Skia Roller service overall (either by convincing the Skia team it is a priority, or contributing ourselves)
- Improving the Skia Roller Flutter SDK conditional logic (likely contributing ourselves, assuming this is acceptable)
- Changing our tree **engine** gardening workflow to work around perceived limitations
- Migrating off the Skia Roller to another solution
- Do nothing, and accept the status quo (but we should do this intentionally, not by default)

---

My thoughts: We've optimized too far for the rollers, that is, this current workflow optimizes a bit too far for the automated changes to roll into the SDK easily, and limit the amount of manual intervention (human) **engine** gardeners need to spend reviewing failing roll PRs. I'm under the impression that _gardening is intended to be a full-time job (for that week)_, so if we're still certain that is the right policy, it seems reasonable to re-evaluate if optimizing for the rollers/**engine** gardeners is the right trade-off for our teams.

If we can somehow improve the bots to reduce toil _and_ revert this workflow change, that is seems like the best outcome. What is unclear to me is what if we can't (or it's too expensive to) reduce the toil - is the workflow change still worth it? This is coming up specifically because `flutter/engine` is in the process of shipping its own graphics renderer, and anything that makes gold-testing less reliable or harder to understand undermines our testing strategy.

/cc @johnmccutchan @zanderso @Piinks @jonahwilliams @christopherfujino @goderbauer @gaaclarke