Infra support to run a "mock" release of the engine with golden file verification

[matanlurey]

Type of Request

infra task

Infrastructure Environment

LUCI + Cocoon? Not 100% sure.

What is happening?

In #144835, @gaaclarke and I plan to run a "mock" release of flutter/engine, for the first time with golden-file tests enabled, and test out our proposed workflow.

Steps to reproduce

N/A

Expected results

I think I need infra support:

1. What process should I follow to create a "mock" release branch?
2. How can I enable golden-file tests/GOLDCTL for this branch (it's disabled today IIRC)

/cc @zanderso

[keyonghan]

What process should I follow to create a "mock" release branch?

One thing we can try is pick an old enough branch (which we are sure that we won't use), and run the workflow there. /cc @XilaiZhang @Jasguerrero who I believe just recently tried something similar for a recipe branch.

How can I enable golden-file tests/GOLDCTL for this branch (it's disabled today IIRC)

IIUC, the gold tests will auto run in the branch (It is only skipped for the official dart-internal targets). Based on #144347 (comment), images will be sent to Gold, though the gold status will not surface in the release PR due to https://github.com/flutter/cocoon/blob/main/app_dart/lib/src/request_handlers/push_gold_status_to_github.dart#L82

[christopherfujino]

What process should I follow to create a "mock" release branch?

One thing we can try is pick an old enough branch (which we are sure that we won't use), and run the workflow there. /cc @XilaiZhang @Jasguerrero who I believe just recently tried something similar for a recipe branch.

https://github.com/flutter/engine/tree/flutter-3.18-candidate.18 is the branch Xilai and I were using, you can use it also. There will not be a future release from that branch.

[christopherfujino]

If the code you need is on that branch...

[matanlurey]

It is not (very recent PRs).

[christopherfujino]

It is not (very recent PRs).

@matanlurey if you share the earliest commit that will qualify, I can find you an unused branch.

[matanlurey]

It is not (very recent PRs).

@matanlurey if you share the earliest commit that will qualify, I can find you an unused branch.

flutter/engine#51739 (8630c4b).

[christopherfujino]

It is not (very recent PRs).

@matanlurey if you share the earliest commit that will qualify, I can find you an unused branch.

flutter/engine#51739 (8630c4b).

Ahh ok, that's too fresh for Keyong's approach (that's newer than any of our existing beta releases, so we can't confirm any of the roll branches won't be published). You could open a PR and run presubmits, but I'm guessing you want to test postsubmit too?

[matanlurey]

Right, I need to land and then open up cherry-picks against it, etc, similar to a real release branch.

[christopherfujino]

I believe your options then are to wait for the next beta release, and then pick a release candidate branch older than that; or

1. create an arbitrarily named branch from your desired point
2. create a branch of the same name on the https://flutter.googlesource.com/recipes/ repo.
3. create and merge an empty (or trivial whitespace diff) commit to that recipes branch, to trigger a build of https://ci.chromium.org/ui/p/flutter/builders/luci.flutter.prod/recipes-bundler (this uploads the version of the recipes your release builds will use)
4. add the name of the branch to the enabled_branches key in .ci.yaml file of that branch and merge this commit: https://github.com/flutter/engine/blob/main/.ci.yaml#L11
5. Wait a build of this builder to be scheduled and complete: https://ci.chromium.org/ui/p/flutter/builders/luci.flutter.staging/Linux%20ci_yaml%20engine%20roller

Then you should be ready to test your workflow. You should get verification from one of @godofredoc, @XilaiZhang, or @Jasguerrero that this is still how this works :)

[matanlurey]

I don't mind doing those steps. It's also 4:45 and I won't do them today anyway.

/cc @gaaclarke maybe we can chat for 5m tomorrow and get your take.

[matanlurey]

Temporarily assigning to myself

[matanlurey]

@XilaiZhang and I are going to pair on this together next week.

Edit: @gaaclarke will take the lead on this, I have a lot on my plate :)

[gaaclarke]

Notes:

• So, the steps chris gave seemed to explain how to branch the recipes repository and link it to an engine branch. We created a branch of the engine named flutter-test-golden-1, but there needs to be a corresponding branch in the recipes repo that has the exact same name. After those branches are created, there was a build triggered apropos the ci.yaml. If you post a PR to the branched engine before that builder is finished, the ci.yaml validation presubmit check will fail to execute.
• We need to verify with Matan what is considered a valid version number for .engine-release.version. In this case I would have liked to have added latin characters to the version number, but it seems from the comment that that isn't supported? To be safe I chose 0.0.1.

[gaaclarke]

The process seemed to work correctly up until the skia gold bot. Goldens were generated with the proper name addition. The bot didn't seem to pick up the new images though: flutter/engine#52005 (comment)

I'm not sure what the proper course of action at this point and need to mind meld with @matanlurey and @zanderso when they are both around.

[gaaclarke]

Looks like we need a change in cocoon. This logic appears to be skipping the skia gold bot reporting a diff.

https://github.com/flutter/cocoon/blob/9dd49cd93309b4d737d244e34d699f448e6ef17b/app_dart/lib/src/request_handlers/push_gold_status_to_github.dart#L86

I'm not sure if manually going in and accepting those goldens would be fine. We probably don't want to do that as part of the process though since it would be easy to forget and mess up.

cc @matanlurey

[gaaclarke]

Everything has been verified to work in flutter/engine#52005

The skia gold bot is running on engine release branches and the produced golden tests have the release name appended to them.

[gaaclarke]

I'm going to mark this as closed. I'm convinced we are good.

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

[gaaclarke]

The mock test executed correctly, but on the next release the PR that sets the release name did not seem to reap the golden results like what happened in the mock test: flutter/engine#53148

As a result the golden tests were removed from that release in flutter/engine#53163.

We should run the mock release again to see if the recipes have regressed or to try to identify an x-factor between the test setup and release setup.

@christopherfujino has more context for this decision here: flutter/engine#53163 (comment)

[matanlurey]

@gaaclarke I am going to assume you're taking the lead on this, but let me know if you'd like to pair/chat it out.

[gaaclarke]

I double checked the cocoon code to make sure the logic for running skia gold was still intact. It is.

• https://github.com/flutter/cocoon/blob/c792fa3737d5f119a783048bb97c3c0d51959dc8/app_dart/lib/src/service/config.dart#L70
• https://github.com/flutter/cocoon/blob/c792fa3737d5f119a783048bb97c3c0d51959dc8/app_dart/lib/src/request_handlers/push_gold_status_to_github.dart#L81

It seems like the conditions that made the mock test pass in the past are not the same conditions we see in real releases. I don't think running another mock release will be helpful until that is addressed. And maybe not helpful at all considering what we are seeing. We should track down the log statement that reads log.fine('This change\'s destination, ${pr.base!.ref}, does not run Skia Gold checks, skipping.') since that will help us identify the differences potentially.

[gaaclarke]

I looked through the code a bit and I believe we are running into an x-factor between the environment where the mock release can be run and the actual releases. I don't think doing another mock release will help. I've filed a new bug that outlines what I know of the issue: #149670