Skip to content

Add a test-matrix status check, name every job - #3356

Open
singlerider wants to merge 5 commits into
blacklanternsecurity:devfrom
singlerider:ci-matrix-split
Open

Add a test-matrix status check, name every job#3356
singlerider wants to merge 5 commits into
blacklanternsecurity:devfrom
singlerider:ci-matrix-split

Conversation

@singlerider

@singlerider singlerider commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

The ruleset on dev requires four status checks by literal name: test (3.10) through test (3.13). That means renaming a job orphans a required check, and the list drifts from the matrix. 3.14 is tested today but isn't in the ruleset, so it has never blocked a merge.

This adds one test matrix passed job that depends on the test matrix and fails unless every dependency succeeded. Require that single context instead, and the matrix and job names go back to being repo-side concerns.

Scope: what this gate does and does not cover

needs: cannot reference jobs in another workflow, so this job covers the test matrix in tests.yml and nothing else. CodeQL, the distro suite, the CLA check and the benchmark all live in their own workflows and are unaffected.

An earlier revision of this PR called the job all checks passed, which was a lie in a place that matters: the name of a required status check is what a reviewer reads when deciding whether the gate is sufficient. It is now named for the one thing it actually enforces. If the distro suite should also gate merges, that is a second required context, not something this job can absorb.

Pinning

The action is pinned to a commit sha rather than the mutable release/v1 branch: the one check that decides mergeability should not be able to change underneath us.

The first version of this PR pinned afee1c1e, which is the sha of the annotated tag object for v1.2.2, not the commit it points at. GitHub happens to resolve both, so it worked, but the intended and conventional target is the commit, 05ac9388. I confirmed the two archives are byte-identical trees before switching, so this is a correctness-of-convention fix and not a behavior change.

Skips

It sets allowed-skips: test, so a skipped test matrix still satisfies the gate while a failed or cancelled one does not. That matters for #3355, which skips the suite on docs-only pull requests; without it the gate would read the skip as not-success and block every docs-only PR.

Note this is genuinely allowed-skips and not allowed-failures: a cancelled or failed matrix still fails the gate. The one completed run of the earlier revision demonstrates this. The matrix was cancelled and the gate correctly went red.

Job naming

It also names every job after what it does, which is only safe once no required context points at a matrix job:

before after
test (3.10) run test suite on python 3.10
publish_code publish package and docker images
tag_commit tag the release commit
publish_docs publish the docs site
test-distros (fedora) install and smoke-test on fedora
Analyze (python) scan python for security issues
benchmark compare performance against the base branch
cla check the CLA is signed
update_docs regenerate the docs and open a PR
update-nuclei-version open a PR when a new nuclei is released
update-trufflehog-version open a PR when a new trufflehog is released

Same jobs, same steps, same triggers.

Merging this alone changes nothing about what gates dev. A check can't be marked required until it has reported once, so this has to land first.

Admin steps, after this merges

Settings > Rules > protecc > Require status checks to pass:

  1. Remove test (3.10)
  2. Remove test (3.11)
  3. Remove test (3.12)
  4. Remove test (3.13)
  5. Add test matrix passed

Do this in the same window as the merge. In between, PRs will show the four old contexts stuck on "Expected — Waiting for status to be reported" and won't be mergeable.

CLAAssistant and CodeQL can't be renamed from this repo; GitHub Apps post those, not our jobs.

Conflicts

This and #3355 both insert a new job at the top of tests.yml and will conflict textually. Whichever lands second needs a trivial rebase; the two changes are independent in substance.

CI note

benchmark is red here because a fork PR gets a read-only token and can't post its comment (403). It fails identically on the other two CI PRs and passes on same-repo branches.

@singlerider singlerider reopened this Jul 28, 2026
@singlerider
singlerider force-pushed the ci-matrix-split branch 3 times, most recently from 33980bb to cea65d7 Compare July 28, 2026 17:57
@singlerider singlerider changed the title Run the full test suite on one Python version for PRs Add an aggregate CI status check, name every job Jul 28, 2026
Required status checks are matched by literal context string, and the ruleset
on dev names four of them directly: test (3.10) through test (3.13). Two
consequences. Renaming a job orphans a required check, which then waits
forever for a name nothing reports. And the list drifts: 3.14 is in the matrix
but not in the ruleset, so it has been non-blocking since it was added.

Add a single `all checks passed` job that depends on the matrix. The ruleset
can require that one context instead, leaving the matrix and the job names as
repo-side concerns.

Also name every job after what it does, which is safe to do in the same change
now that no required context points at a matrix job:

  test                      -> run test suite on python 3.10
  publish_code              -> publish package and docker images
  tag_commit                -> tag the release commit
  publish_docs              -> publish the docs site
  test-distros              -> install and smoke-test on fedora
  analyze                   -> scan python for security issues
  benchmark                 -> compare performance against the base branch
  cla                       -> check the CLA is signed
  update_docs               -> regenerate the docs and open a PR
  update-nuclei-version     -> open a PR when a new nuclei is released
  update-trufflehog-version -> open a PR when a new trufflehog is released

This must merge before the ruleset is updated: a context is not selectable as
a required check until it has reported at least once. Merging this alone
changes nothing, the old required checks keep gating dev. The ruleset edit is
a separate admin step, described in the PR.
alls-green was referenced by the mutable release/v1 branch. A gate whose job
is to decide mergeability should not be able to change under us, so pin the
commit sha.

allowed-skips: test because the docs-only path filter skips the test matrix.
Without it the gate reads a skipped dependency as not-success and blocks every
docs-only PR. Anything other than success or skipped still fails the gate.
@singlerider
singlerider marked this pull request as ready for review July 28, 2026 18:40
The pin was the SHA of the v1.2.2 annotated tag object rather than the commit
it points at. Both happen to resolve, but the convention is the commit sha.

'all checks passed' also overpromised: needs: cannot cross workflows, so the
gate covers the test matrix only, not CodeQL, distros or the benchmark. Renamed
to 'test matrix passed' so the required check says what it actually enforces.
@singlerider singlerider changed the title Add an aggregate CI status check, name every job Add a test-matrix status check, name every job Jul 28, 2026
@singlerider
singlerider marked this pull request as draft July 28, 2026 19:11
@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90%. Comparing base (e9ad309) to head (801f849).

Additional details and impacted files
@@          Coverage Diff          @@
##             dev   #3356   +/-   ##
=====================================
- Coverage     90%     90%   -0%     
=====================================
  Files        454     454           
  Lines      46687   46687           
=====================================
- Hits       41956   41950    -6     
- Misses      4731    4737    +6     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@singlerider
singlerider marked this pull request as ready for review August 11, 2026 05:05
singlerider and others added 2 commits August 10, 2026 22:31
The benchmark 'Generate benchmark comparison report' step failed with
'pathspec ci-matrix-split did not match any file(s) known to git'.
On pull_request runs, actions/checkout leaves a detached HEAD at the
merge ref and creates no local branch named after github.head_ref, so
benchmark_report.py's 'git checkout <head_ref>' could not find it. Fall
back to the remote-tracking ref origin/<branch> (available thanks to
fetch-depth: 0) and detach onto it.

The cascading 'Comment benchmark results on PR' step then 403'd with
'Resource not accessible by integration' because posting to the
issues/comments API needs issues: write, which the workflow did not
grant. Add issues: write alongside pull-requests: write.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant