CI: migrate from Bors to GitHub Merge Queue; also, refactor some code, and add some more unit tests

[DilumAluthge]

It looks like the public Bors instance has been shut down, so we are forced to migrate to GitHub Merge Queue instead.

[ericphanson]

How do we configure the merge queue? Did you already set it up to trigger integration tests?

[mattBrzezinski]

How do we configure the merge queue? Did you already set it up to trigger integration tests?

My phone is upstairs, so I can't see if you're an admin in this repo, but here's the above settings. From skimming the README it seems like you just need to configure it here and in the workflow set on -> merge_group?

[ericphanson]

Cool, let's try it!

[DilumAluthge]

Hmmm. Why isn't the merge queue working?

[DilumAluthge]

I think that Merge Queue is unhappy because the integration tests aren't running on the PR.

But we intentionally don't want to run the integration tests on the PR, because we want to avoid hitting rate limits.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (184c654) 100.00% compared to head (218e9ce) 99.49%.

Additional details and impacted files

@@             Coverage Diff             @@
##            master     #477      +/-   ##
===========================================
- Coverage   100.00%   99.49%   -0.51%     
===========================================
  Files           12       12              
  Lines          394      398       +4     
===========================================
+ Hits           394      396       +2     
- Misses           0        2       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[DilumAluthge]

I have unchecked the "Only merge non-failing pull requests" option in the merge queue settings.

[DilumAluthge]

I think we basically just need to make sure that none of the required status checks are stuck at pending (yellow) on a PR. The easiest way to do that is to just fail those checks on PRs, which is what I've done here.

Because I've unchecked the "Only merge non-failing pull requests" option, those PR failures should not prevent us from adding a PR to the merge queue, at which point those same status checks will pass on the merge_group job.

[ericphanson]

Looking at https://github.com/orgs/community/discussions/46757#discussion-4836009 and https://theunixzoo.co.uk/blog/2023-11-16-migrating-to-gh-merge-queues.html I think we do need a workaround here. But I think we should pass the checks on PRs rather than failing them, and require all checks pass

[DilumAluthge]

My concern is that if we mark the integration tests as passed (green) on PRs, then I might look at a PR and mistakenly think to myself that the integration tests have run and passed, which would not be correct. At least with the red X, it's clear that the integration tests did not run and pass.

Maybe I can mark the integration tests as "skipped" on PRs. I'll try that out.

[ericphanson]

following the links there leads to https://github.com/imjohnbo/ok-to-test as well

[DilumAluthge]

Codecov is the only problem here, right?

I think we should just get unit test coverage to 100%, and then use GitHub Merge Queue.

Or, alternatively, self-host our own instance of Bors.

[DilumAluthge]

It looks like it's only like 3 lines uncovered by the unit tests, right?

[ericphanson]

Chasing down 100% coverage via mocking seems like a slog though :(

[DilumAluthge]

Alright, I have it down to only 2 uncovered lines (that are uncovered by the unit tests but covered by the integration tests). But I'm not sure how to write a unit test to cover those two lines, even with mocking.

I think, for the short-term, let's just revert to the original codecov.yml, and allow PRs to enter the merge queue even if the Codecov statuses are ❌.

We can maybe create a "developer docs" markdown page in the repo, and in that page, document that it's okay if Codecov shows up as 99% (and thus ❌) on a PR, because it will become 100% once the PR is added to the merge queue and integration tests run.

[DilumAluthge]

let's just revert to the original codecov.yml

#482

[DilumAluthge]

We can maybe create a "developer docs" markdown page in the repo, and in that page, document that it's okay if Codecov shows up as 99% (and thus ❌) on a PR, because it will become 100% once the PR is added to the merge queue and integration tests run.

#483

[DilumAluthge]

Why is this not merging? I unchecked the "only merge passing PRs" option in the branch protection settings.

[ericphanson]

are codecov checks required? maybe for required checks they must all be passing, or something like that

[DilumAluthge]

That's really annoying if true. What, then, is the point of being able to uncheck the "only merge passing PRs" checkbox?

[DilumAluthge]

I'm going to throw my hands up in defeat for now, and just lower the threshold to 99% (which is what the unit tests hit).

[DilumAluthge]

Alright, the threshold for codecov/project/minimum is now 99%.