Build Analysis completed before test were done?

[missymessa]

• Investigate issue here: https://teams.microsoft.com/l/message/19:afba3d1545dd45d7b79f34c1821f6055@thread.skype/1720481000264?tenantId=72f988bf-86f1-41af-91ab-2d7cd011db47&groupId=4d73664c-9f2f-450d-82a5-c2f02756606d&parentMessageId=1720481000264&teamName=.NET%20Core%20Eng%20Services%20Partners&channelName=First%20Responders&createdTime=1720481000264
• Write additional documentation regarding Build Analysis and auto-merge functionality

Release Note Category

• Feature changes/additions
• Bug fixes
• Internal Infrastructure Improvements

Release Note Description

GitHub has (recently?) clarified in their documentation that conclusion should only be set if status is set to completed. In this case, we were setting conclusion when status is set to in_progress, which is invalid.

It looks like there is a flag that can be set to work around this behavior, but it is invalid to disable the flag because it violates the GitHub documentation on valid parameters to the API.

[missymessa]

/cc @AlitzelMendez @agocke

[agocke]

I think this sequence summarizes the situation pretty well:

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]

There seems to be a delay (~10s) between pushing a new commit and start of the runtime pipelines. See

where the merge button is green even though the pipelines haven't started yet.

[missymessa]

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

[agocke]

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]

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]

@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]

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]

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]

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.