Repository: JuliaRegistries/CompatHelper.jl
Pull request #477 (2023-12-01 21:46:42)

CI: migrate from Bors to GitHub Merge Queue; also, refactor some code, and add some more unit tests
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?

![Screenshot 2023-12-01 at 17-56-15 Settings · Branches · Branch protection rule · JuliaRegistries_CompatHelper jl](https://github.com/JuliaRegistries/CompatHelper.jl/assets/4356074/f0101929-8785-4adf-bafc-381c3bbee181)


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](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-a-merge-queue) it seems like you just need to configure it here and in the workflow set `on -> merge_group`?
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.
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: Yeah, that makes sense. My concern is that contributors will see the red X and spend time trying to debug when there's actually nothing for them to do. It could also give reviewers the sense that a PR isn't ready, when maybe in fact it is. "skipped" seems the most accurate if we can get that to work.
ericphanson: > 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

This seems less problematic to me, as you can't merge directly (as far as I understand), you can only merge through the merge queue. So even if you think the pr is 100% ready to go, it will still go through the queue and the tests will still run. So we won't merge stuff that doesn't pass the tests.
DilumAluthge: > and require all checks pass

Unfortunately, even if I'm able to get the integration tests to show up as "skipped", we still will have the Codecov status checks as red, because apparently the code coverage of the unit tests is only 99%, and we need the integration tests to get to 100%.

So the Codecov statuses are going to show up as red on PRs, unless we can figure out how to write some unit tests (maybe with the use of mocking) to get that last 1% of coverage.
ericphanson: we can also do this: https://docs.codecov.com/docs/common-recipe-list#ease-target-coverage
DilumAluthge: I want to keep enforcing that master stays at 100% code coverage.
ericphanson: I think with this codecov setup we delete the `minimum` check
ericphanson: <img width="525" alt="Screenshot 2023-12-02 at 19 05 31" src="https://github.com/JuliaRegistries/CompatHelper.jl/assets/5846501/a5951fc5-a819-430f-b807-70f42d1dddeb">
can you remove this check @DilumAluthge ? then we can try this out
ericphanson: ok, let's try it! 
ericphanson: Just realized trunk branch here is called `master` not `main`. Fixed
ericphanson: ah, annoying - integration tests are not running here, bc they are running on the cancelled merge queue run from a few mins ago, and we have a global concurrency lock. I wonder if we can gate the lock after the `if` somehow so we only lock for actual runs
DilumAluthge: The issue (I think) is that when you withdrew this PR from the merge queue, the integration tests job (on the merge_group event) did not get auto-cancelled. It's still running: https://github.com/JuliaRegistries/CompatHelper.jl/actions/runs/7071936138

We need to figure out how to auto-cancel merge_group jobs when a PR is withdrawn from the merge queue.
DilumAluthge: Oh I see what you are saying: if the `if:` is going to evaluate to false anyway (on the PR build), why does it need to take the lock.
ericphanson: > We need to figure out how to auto-cancel merge_group jobs when a PR is withdrawn from the merge queue.

For this it might be enough to turn on

>   cancel-in-progress: false

which we have in the integration tests. But I figured maybe we need that to not have dangling PRs and such on the integration repo.
ericphanson: > Oh I see what you are saying: if the `if:` is going to evaluate to false anyway (on the PR build), why does it need to take the lock.

Actually, I think this might not matter too much, because even if the integration tests "ran" with this no-op `true` status, that would just get it into the queue where it still needs to wait for the old run to finish anyway, since in the queue we would do a "real" run. So probably there's little actual gains to be had here. The only actual speedup is if we can cancel in-progress runs then.
ericphanson: Argh! It says the codecov target is 99% even in the merge queue:
<img width="949" alt="Screenshot 2023-12-02 at 19 57 36" src="https://github.com/JuliaRegistries/CompatHelper.jl/assets/5846501/6a512dfa-3a45-4d78-8dd3-d070f6d75f04">

So my codecov.yml changes did not work as intended.
ericphanson: I will remove from queue so we can try alternatives.
DilumAluthge: Sometimes there's a delay before the Codecov status updates.
ericphanson: I thought maybe it was ORing the two possibilities (99% or 100%) so I used a regex to try to make them more exclusive. The regex could be made more robust, but this should be enough to test the theory
ericphanson: what if we just trigger integration tests some other way? e.g. adding a label to the PR. So they skip themselves (and fail) whenever the label is not present. Then when we're ready to run them, we add the label.

This is a security issue if we are restricting access to integration tests for security reasons (bc one could push bad ocmmits after the label is added). I don't think that is the case here where the integration tests do not have access to anything very sensitive. But maybe we could also have the label removed automatically when new commits are pushed if needed.

In this paradigm we wouldn't need merge queue at all, but we could still use it if we wanted.
DilumAluthge: I sure do miss Bors.

The label idea won't be sufficient to get integration tests to run on PRs from forks.
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: 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%.

Patch:
diff --git a/.github/workflows/ci_integration.yml b/.github/workflows/ci_integration.yml
index c7faf70..d40532d 100644
--- a/.github/workflows/ci_integration.yml
+++ b/.github/workflows/ci_integration.yml
@@ -6,35 +6,28 @@ name: CI (integration tests)
 # GitHub's abuse rate limits.
 
 on:
+  merge_group: # GitHub merge queue
+  pull_request: # but we will skip the integration tests on PR builds, to avoid hitting rate limits
   push:
     branches:
       - master
-      - staging
-      - trying
 concurrency:
   group: integration-tests-global-lock
   cancel-in-progress: false
 jobs:
   integration:
-    if: github.event_name == 'push'
-    name: Integration/Julia ${{ matrix.version }}/${{ matrix.os }}/${{ matrix.arch }}
-    runs-on: ${{ matrix.os }}
-    strategy:
-      max-parallel: 1
-      fail-fast: true
-      matrix:
-        arch:
-          - x64
-        os:
-          - ubuntu-latest
-        version:
-          - '1'
+    # We don't actually want to run integration tests on pull requests,
+    # because we want to avoid hitting rate limits.
+    # So, if this is a PR build, mark the integration tests as "skipped".
+    if: github.event_name != 'pull_request'
+    name: Integration
+    runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v4
       - uses: julia-actions/setup-julia@v1
         with:
-          version: ${{ matrix.version }}
-          arch: ${{ matrix.arch }}
+          version: '1'
+          arch: x64
       - uses: actions/cache@v3
         env:
           cache-name: cache-artifacts
diff --git a/.github/workflows/ci_unit.yml b/.github/workflows/ci_unit.yml
index 8168df9..4845442 100644
--- a/.github/workflows/ci_unit.yml
+++ b/.github/workflows/ci_unit.yml
@@ -1,11 +1,10 @@
 name: CI (unit tests)
 on:
+  merge_group: # GitHub merge queue
   pull_request:
   push:
     branches:
       - master
-      - staging
-      - trying
     tags: '*'
   workflow_dispatch:
 jobs:
@@ -44,6 +43,11 @@ jobs:
       - uses: julia-actions/julia-runtest@v1
         env:
           COMPATHELPER_RUN_INTEGRATION_TESTS: "false"
+      - uses: julia-actions/julia-processcoverage@v1
+      - uses: codecov/codecov-action@v3
+        with:
+          file: lcov.info
+          token: ${{ secrets.CODECOV_TOKEN }}
   documentation:
     name: Documentation
     runs-on: ubuntu-latest
diff --git a/Project.toml b/Project.toml
index 9cf2d6a..c1ee363 100644
--- a/Project.toml
+++ b/Project.toml
@@ -17,15 +17,18 @@ TimeZones = "f269a46b-ccf7-5d73-abea-4c690281aa53"
 UUIDs = "cf7118a7-6976-5b1a-9a39-7adc72f591a4"
 
 [compat]
+Aqua = "0.8"
 Base64 = "1"
 Dates = "1"
 GitForge = "0.4"
 Mocking = "0.7"
 MultilineStrings = "0.1"
 Pkg = "1"
+Random = "<0.0.1, 1"
 RegistryInstances = "0.1"
 SHA = "0.7, 1"
 TOML = "1"
+Test = "<0.0.1, 1"
 TimeZones = "1"
 UUIDs = "1"
 julia = "1.6"
diff --git a/bors.toml b/bors.toml
deleted file mode 100644
index ff3e0f1..0000000
--- a/bors.toml
+++ /dev/null
@@ -1,20 +0,0 @@
-block_labels = [ "DO NOT MERGE" ]
-
-delete_merged_branches = true
-
-status = [
-    "Doctests",
-    "Documentation",
-    "Integration/Julia 1/ubuntu-latest/x64",
-    "Unit/Julia 1.6.0/ubuntu-latest/x64",
-    "Unit/Julia 1.6/ubuntu-latest/x64",
-    "Unit/Julia 1/ubuntu-latest/x64",
-    "Unit/Julia nightly/ubuntu-latest/x64",
-    "codecov/patch",
-    "codecov/project",
-    "codecov/project/minimum",
-]
-
-# We have a long timeout, because sometimes it takes a while
-# before the `codecov/*` statuses show up.
-timeout_sec = 3600 # 60 minutes
diff --git a/codecov.yml b/codecov.yml
index b3c072f..01bcf93 100644
--- a/codecov.yml
+++ b/codecov.yml
@@ -4,4 +4,10 @@ coverage:
       default:
         target: auto
       minimum:
-        target: "100%"
+        # We want to enforce 100%.
+        # The Unit Tests + Integration Tests hit 100%.
+        # But the Unit Tests alone only hit 99.49%.
+        # And so, because of GitHub Merge Queue limitations,
+        # we need to set this threshold to 99%.
+        # target: "100%"
+        target: "99%"
diff --git a/src/dependencies.jl b/src/dependencies.jl
index 26375df..db7d8bc 100644
--- a/src/dependencies.jl
+++ b/src/dependencies.jl
@@ -109,3 +109,13 @@ function get_latest_version_from_registries!(
 
     return deps
 end
+
+function populate_dep_versions_from_reg!(deps; options)
+    if options.use_existing_registries
+        get_existing_registries!(deps, options.depot; options)
+    else
+        get_latest_version_from_registries!(deps, options.registries; options)
+    end
+
+    return nothing
+end
diff --git a/src/main.jl b/src/main.jl
index 98b103f..919afca 100644
--- a/src/main.jl
+++ b/src/main.jl
@@ -53,11 +53,7 @@ function main(
         project_file = @mock joinpath(local_clone_path, subdir, "Project.toml")
         deps = get_project_deps(project_file; include_jll=options.include_jll)
 
-        if options.use_existing_registries
-            get_existing_registries!(deps, options.depot; options)
-        else
-            get_latest_version_from_registries!(deps, options.registries; options)
-        end
+        populate_dep_versions_from_reg!(deps; options)
 
         for dep in deps
             pr = @mock make_pr_for_new_version(


Test patch:
diff --git a/test/dependencies.jl b/test/dependencies.jl
index cc140a4..391f38c 100644
--- a/test/dependencies.jl
+++ b/test/dependencies.jl
@@ -119,3 +119,35 @@ end
         end
     end
 end
+
+@testset "populate_dep_versions_from_reg!" begin
+    # For this test, we will just use CompatHelper's own `Project.toml` file.
+    project_file = joinpath(pkgdir(CompatHelper), "Project.toml")
+
+    # Just for this test, we hardcode this list
+    unregistered_stdlibs = [
+        "Base64",
+        "Dates",
+        "Pkg",
+        "UUIDs",
+    ]
+
+    @test ispath(project_file)
+    @test isfile(project_file)
+    for use_existing_registries in [true, false]
+        options = CompatHelper.Options(;
+            use_existing_registries,
+        )
+        deps = CompatHelper.get_project_deps(project_file)
+        for dep in deps
+            @test dep.latest_version === nothing
+        end
+        CompatHelper.populate_dep_versions_from_reg!(deps; options)
+        for dep in deps
+            if !(dep.package.name in unregistered_stdlibs)
+                @test dep.latest_version isa VersionNumber
+                @test dep.latest_version > v"0"
+            end
+        end
+    end
+end