Add a feature that allows treating presubmit: false as true, conditionally.

[matanlurey]

Closes flutter/flutter#138430 and I believe flutter/flutter#111418.

As @CaseyHillers mentions in flutter/flutter#138430, this would not be sufficient to understand the addition of a label (only if the PR is first created with a label, or a commit is pushed after the label is added), but it might be worth merging as-is (assuming I got all of the assumptions correct).

[gaaclarke]

Code looks good to me. What happens if you add the label after all the presubmit tests have run? Will it trigger the missing tests?

I was thinking a comment in the description would scale better since it would allow you to specify what tests you want to promote. Relying on labels won't scale.

[matanlurey]

@gaaclarke:

What happens if you add the label after all the presubmit tests have run? Will it trigger the missing tests?

No, though @CaseyHillers has promised me he'd pair on a follow-up feature to do so. I figure this is better than nothing :)

I was thinking a comment in the description would scale better since it would allow you to specify what tests you want to promote. Relying on labels won't scale.

This probably makes sense long term, but Casey's thought is we have a very tiny amount of post-submit targets today (in the engine in particular), so we could start with this feature and then later "graduate" to something else - my idea was something like:

presubmit:
  if-label: "label-name"

One nice aspect of this is we could share labels across different targets. Comments could work too!

[gaaclarke]

LGTM, modulo Casey's feedback. Thanks!

[CaseyHillers]

Please get @ricardoamador's review on this as well. If this causes capacity issues, we can always delete the label + revert this change. This should be a good base for us to iterate on, and unblock the engine testing team.

[ricardoamador]

I understand this is the easy path and gets you what you want you want now but you can handle the 'labeled' action on the 'pull request' event properly queue off the tests when a label is added.

[ricardoamador]

What does this do to the try pool in terms of usage? What is number of prod tests that could now be potentially added to the run?

[ricardoamador]

Questions.

[godofredoc]

@matanlurey thank you for implementing this functionality. Giving that all the changes to cocoon can potentially impact the entire team we would like to first understand the implications of landing the PR. Some open questions:

• In which scenarios are we expecting this functionality to run? e.g. just engine, autorollers, manually selected PRs.
• What would be the impact if a single person adds test: all in a framework PR? e.g. it could block post-submit benchmarks for ~1h

[matanlurey]

Thanks for the questions.

Replies inline, though I'll admit I'm motivated to see this land as soon as reasonable, because the alternatives to this PR is I do this manually by editing the .ci.yaml config in a PR, and then hope I don't forget to revert it before merging (we have a lot of cleanup work around clang_tidy that requires this workflow).

---

@ricardoamador:

I understand this is the easy path and gets you what you want you want now

Partially, and partially I'm a fan of "PRs are small ... and just do exactly one thing". I'm not sure this is the right workflow, and I'm not sure if we'll cause other resource constraint issues (that you, Casey, and Godofredo all brought up), so I figured I'd go with a simpler approach until value was proven (might just take a few days I suppose).

What does this do to the try pool in terms of usage?

No idea, but in practice I don't think it matters.

The alternative to this feature is worse and likely uses as much, or even more, resources (and time):

1. We break the build, requiring reverts and relands.
2. We manually turn on the equivalent by editing .ci.yaml.

What is number of prod tests that could now be potentially added to the run?

According to .ci.yaml, this would run:

• linux_android_emulator_tests
• linux_benchmarks
• linux_clang_tidy
• mac_clang_tidy

... but only PRs specifically tagged (for example, not the auto-rolls).

---

@godofredoc:

In which scenarios are we expecting this functionality to run?

I thought this was obvious in the request, but this is for manually tagging a subset of PRs. See my comments above.

What would be the impact if a single person adds test: all in a framework PR?

This is outside of my knowledge. My original PR had a check that only allowed this for flutter/engine, but it was removed after a review from @CaseyHillers. I'd be happy to reinstate it if it makes folks less nervous about trying this out (we could also just not create the label in the framework repo).

[ricardoamador]

If you add back the clause that checks that the repo is engine only we can give this a try. There is no way we can do this for flutter/flutter as there are currently 267 post-submit tasks and that would definitely affect the load on the try pool. Enabling for engine only should be a good place to start. I think there are less than 7 postsubmit tasks if I remember correctly in engine so we can see what the load is there before enabling on other repos.

if (ciYaml.slug != Config.engineSlug) {
    return false;
}

[ricardoamador]

if (ciYaml.slug != Config.engineSlug) {
    return false;
}

For now until the pool usage can be evaluated and we can see the effect, if any on our SLO.

[matanlurey]

If you add back the clause that checks that the repo is engine only we can give this a try. There is no way we can do this for flutter/flutter as there are currently 267 post-submit tasks and that would definitely affect the load on the try pool. Enabling for engine only should be a good place to start. I think there are less than 7 postsubmit tasks if I remember correctly in engine so we can see what the load is there before enabling on other repos.

if (ciYaml.slug != Config.engineSlug) {
    return false;
}

Done and test (re-)added as well.

[ricardoamador]

Send it!

[matanlurey]

@ricardoamador:

Send it!

Thanks! Out of curiosity, how long does it take to roll so I can try it?

[ricardoamador]

@matanlurey sorry did not see this earlier. It will be deployed in the morning at 8am.

[gaaclarke]

What does this do to the try pool in terms of usage?

No idea, but in practice I don't think it matters.

The alternative to this feature is worse and likely uses as much, or even more, resources (and time):

1. We break the build, requiring reverts and relands.
2. We manually turn on the equivalent by editing .ci.yaml.

Good point about the reverts and relands. One minor nit is that editing the .ci.yaml allows more surgical promotion. Assuming no breakage this flow does increase usage. I'm just harping on the same point I mentioned above. I think there is a difference between "I know I'll need this particular post-submit check to ensure my PR is correct" between "I want to make sure my PR isn't reverted because of post-submit check breakage". I'm more interested in the first case and I'm not quite sure how I feel about the second. If that's our desire, it seems like having the post-commit checks happen atomically when attempting to merge would be ideal since it would remove the need for reverts from post-submit check failure, but I don't think that's possible with github, right?

[matanlurey]

@gaaclarke:

One minor nit is that editing the .ci.yaml allows more surgical promotion

That's true, though we determined for the engine repo specifically it's not significant - it could be over time though and I'd be open to looking into something like outlined in #3256 (comment) if we get there.

If that's our desire, it seems like having the post-commit checks happen atomically when attempting to merge would be ideal since it would remove the need for reverts from post-submit check failure, but I don't think that's possible with github, right?

It really depends (TM).

The way I'd implement it today is by having the post-submit checks stay in the yellow (pending) state similar to Skia-gold, and once all other tests are passing and the change is LGTMd, run the tests before merging. Another option would be using a merge queue, which might be a better fit but is a bigger workflow change for the infra team.

/cc @CaseyHillers

[CaseyHillers]

I'm not sure merge queue is relevant in this case. The class of issues it solves revolve around out of sync issues, which we rarely face. Tree breakages happen as there is a gap in presubmit vs postsubmit testing (the framework being the best example of this).

Around the capacity concerns, I would be interested in flutter/flutter#138511 where Cocoon throttles users consuming too much capacity. Instead of outright blocking this, the work will only get run when there is nothing else to do. For many people, I suspect it would be preferable to run tests in off-peak to prevent having to revert+reland their change.