search

dotnet / dnceng

.NET Engineering Services
MIT License
24 stars 18 forks source link

Build Analysis completed before test were done? #3327

Open missymessa opened 2 months ago

missymessa commented 2 months ago

Release Note Category

missymessa commented 2 months ago

/cc @AlitzelMendez @agocke

agocke commented 2 months ago

I think this sequence summarizes the situation pretty well: image

  1. Tests have completed, Build Analysis is Red.
  2. Auto-merge is enabled, nothing happens because Build Analysis is red.
  3. One hour later, a new commit is pushed.
  4. Before tests are done, the PR is automerged.
  5. After tests finish running, the tests fail and Build Analysis is red.

So it seems like there's a problem in 3-4.

matouskozak commented 2 months ago

There seems to be a delay (~10s) between pushing a new commit and start of the runtime pipelines. See image where the merge button is green even though the pipelines haven't started yet.

missymessa commented 2 months ago

Does the "auto-merge" feature treat neutral check statuses as "passing"?

agocke commented 2 months ago

https://docs.github.com/en/rest/checks/runs?apiVersion=2022-11-28#update-a-check-run seems to indicate merely providing a conclusion (including "neutral") is considered as a "completed" check.

missymessa commented 2 months ago

A "completed" check does not mean "passing". There's a difference between check status and state. If a check is completed, it could also have "failed" or something other than "passing".

missymessa commented 2 months ago

@agocke what are the branch protections for runtime? Does it include "Require status checks to pass before merging"?

If that's the case, then this is likely a bug on GitHub's side: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/automatically-merging-a-pull-request#enabling-auto-merge

agocke commented 2 months ago

Yup we have that enabled. Sorry, it’s not clear where the bug on the GitHub side is. Is there some documentation that says that “neutral” checks should block merging?

missymessa commented 2 months ago

https://github.com/octokit/octokit.net/blob/main/Octokit/Models/Common/CheckStatus.cs#L18

So, the Checks have a Status and Conclusion. A "neutral" check is not the same as a "success" (passing). If GitHub's documentation says that it observes the branch protection of only merging when the checks "pass" then either 1) GitHub treats "neutral" the same as "success" or 2) this is a bug and it shouldn't be doing it this way.

akoeplinger commented 1 month ago

Same issue reported a while ago about neutral counting as a passing check: https://github.com/orgs/community/discussions/45229.

I assume this is by design so you can have status checks that no-op but it'd be good if GitHub clarified this.