Skip to content

ci: run the fast suite on pushes to master - #470

Merged
PathGao merged 1 commit into
masterfrom
ci/test-on-push-to-master
Aug 6, 2026
Merged

ci: run the fast suite on pushes to master#470
PathGao merged 1 commit into
masterfrom
ci/test-on-push-to-master

Conversation

@PathGao

@PathGao PathGao commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Nothing in this repository runs on a push.

build.yml        workflow_dispatch                 (manual release)
test.yml         pull_request
test_build.yml   pull_request + workflow_dispatch

That was harmless for as long as every change arrived as a pull request from a
fork — there was no other way in. It stops being harmless now that contributors
branch inside this repository: git push origin master is a single command that
lands code on the default branch and runs npm audit, npm run check,
npm test and cargo test exactly zero times. Nothing warns you, because
nothing failed; there simply is no run.

This adds push on master to test.yml only.

Why test.yml and not test_build.yml

test.yml is one ubuntu-22.04 job: apt deps, npm ci, npm audit,
npm run check, npm test, cargo test. It is the check that answers "does
this commit still work", and it is cheap enough to run on every commit that
reaches master.

test_build.yml stays on pull_request only. I read it before agreeing with
you, and the file argues its own case: three platforms (macos-latest universal,
ubuntu-22.04, windows-latest), each running the suites and a full
tauri build, and its comment records that macOS jobs "finish in 6-8 minutes but
were waiting 20-30 for a slot" from a pool "an order of magnitude smaller than
the Linux and Windows ones" (#361). Putting it on push means paying that matrix
a second time for every merge, for a build of code that a pull request already
built minutes earlier.

The honest residual gap: a direct push to master that breaks packaging
an NSIS hook, a bundle path, a Cargo.toml feature that only fails on Windows —
is not caught by this PR. test.yml runs on Linux only. That gap already exists,
it is not made worse here, and test_build.yml already carries workflow_dispatch
so you can run the platform matrix on master on demand before cutting a release.
If you would rather close it, push: branches: [master] on test_build.yml is a
two-line follow-up — I did not assume the cost was yours to accept.

Concurrency: checked, and the report was inverted

build.yml has no concurrency block. grep -rn concurrency .github/workflows/
returns exactly one hit, and it is in test_build.yml, added in #361 for the
expensive matrix:

concurrency:
  group: ${{ github.workflow }}-${{ github.ref }}
  cancel-in-progress: true

So the thing that "does something with concurrency" is the workflow this PR is
choosing not to touch.

Does merging then run the same commit twice? No, and a concurrency group
could not fix it if it did:

  • Merges land squashed — every commit on master carries its (#N) and a single
    parent lineage — so the SHA on master is a new object. The pull_request run
    tested refs/pull/N/merge, which is a third object again. There is no commit
    that gets tested twice.
  • A group keyed ${{ github.workflow }}-${{ github.ref }} puts the pull request
    run in Test-refs/pull/468/merge and the push run in Test-refs/heads/master.
    Different groups. Concurrency de-duplicates runs within a ref; it has no
    mechanism to cancel a run on a different one. It is the wrong instrument for
    the stated worry.

So the extra run per merge is real — one ubuntu runner, one fast suite, per
merge — and I want to be straight that it is a cost you are taking on, not a
free win. What it buys: the pull_request run tested head merged into base as
of when it ran
. If master moves after that, the result is stale, and the push
run is the only thing that tests what master actually contains. That is not
hypothetical here — on 2026-08-05, #462, #463 and #464 merged at 14:33, 14:38
and 14:40, inside seven minutes of each other.

A concurrency group on test.yml would be worth having for a different reason
rapid pushes to a pull request branch queue redundant runs, which is exactly why
#361 added one to test_build.yml. I left it out because this PR should be the
trigger and nothing else, and because it needs care: a blanket
cancel-in-progress: true would let a fast follow-up push to master cancel the
only run that was ever going to test the commit underneath it. The correct form
is cancel-in-progress: ${{ github.ref != 'refs/heads/master' }}. Say the word
and it is a separate three-line PR.

Verification

The failure mode for a workflow edit is not a red X, it is silence — a malformed
file simply never runs, which is the same class of defect as #460 (an installer
hook that was never called and produced no error). So:

  • Parsed, not eyeballed. YAML.parse on all three workflow files, using the
    yaml package already in this repo's lockfile, printing the resulting trigger
    object. test.yml now yields {"pull_request":null,"push":{"branches":["master"]}},
    and build.yml and test_build.yml are byte-identical to master and parse
    unchanged. This also produced the concurrency findings above rather than my
    reading for them.
  • GitHub itself parses it, observed not predicted. The Test check on this
    pull request is produced from this branch's copy of test.yml, so a file
    Actions could not read would show up here as a failed run with a workflow
    annotation, or as no run at all. Run
    31062189024
    event=pull_request, conclusion success, every step green through
    npm audit, check frontend, run frontend behavior tests and
    run cargo test. gh api repos/sftwrdotdev/Markpad/actions/workflows still
    lists all three workflows as state=active, with test.yml at its unchanged
    path.
  • I cannot demonstrate the push trigger firing. It is scoped to master, so
    pushing this branch does not exercise it; the first real proof is the first
    commit that lands on master after this merges. I would rather say that than
    imply I watched it work.
  • npm run check — 645 files, 0 errors. npm test — 692 pass, 0 fail. Run on a
    clean checkout of this branch. I touched no source, so this is a
    "nothing broke" baseline, not a claim about the change.
  • scripts/workflowSecurityAudit.test.ts reads .github/workflows/test.yml and
    asserts the npm audit step; it is green, so the trigger addition did not
    disturb the shape that test pins.

Your call

This changes what runs when you push, which is something you live with on every
commit and I do not. The extra ubuntu run per merge is the price; a commit on
master that nothing has ever tested is what it buys off. If you would rather
have the coverage without the duplicate, the alternative is to keep test.yml on
pull_request and instead protect master so nothing can be pushed to it
directly — that is a repository setting, not a file, and it is yours to make.

I deliberately did not add a guard test asserting the trigger, even though this
repository would normally pin a fact like this in scripts/, because other work
is in flight in that directory right now. Happy to add one after.

build.yml triggers on workflow_dispatch, test.yml on pull_request, and
test_build.yml on pull_request plus workflow_dispatch. Nothing listens to push,
so `git push origin master` runs no checks at all. That was invisible while
every change arrived as a pull request from a fork; it stops being invisible now
that contributors branch inside this repository and can push to master directly.

test.yml is the suite that belongs on this trigger: npm audit, npm run check,
npm test and cargo test on one ubuntu-22.04 runner. test_build.yml is
deliberately left on pull_request only -- three platform builds, and its own
comment records macOS jobs waiting 20-30 minutes for a runner slot.

Scoped to master. Pull request branches already run through the pull_request
trigger above, so an unrestricted push trigger would run every branch twice
without covering anything new.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
@PathGao
PathGao merged commit 14508ed into master Aug 6, 2026
4 checks passed
@PathGao
PathGao deleted the ci/test-on-push-to-master branch August 6, 2026 01:32
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