ci: run web and JavaScript tests on pull requests - #878
Conversation
0-jake-0
left a comment
There was a problem hiding this comment.
Reviewed. This is the right fix for #877, and the diagnosis in the PR body is correct: the workflow was already written for pull_request and only missing the event registration. Two pieces of evidence for that beyond what you listed —
concurrency.cancel-in-progress: ${{ github.event_name == 'pull_request' }}has been inert in this file since it was written, because nopull_requestevent could reach it. This PR is what makes that line do anything..github/workflows/README.md:50already claims this workflow is "the only placejs/tests/runs on push/PR". The trigger makes the sentence true rather than aspirational.
I also confirmed the fork-safety story: no secrets. reference anywhere in web-integration.yml, and all three legs are in-process (no service containers, no license), so a fork PR with a read-only token runs the whole thing. That is not true of most of its siblings — kdb-integration.yml needs a license secret — which is why doing web first is the right call rather than a blanket sweep.
Adding the workflow file to its own paths is correct and matches the reasoning already recorded at the top of rust-test.yml.
The one thing I would change: scope push to main
push: here has no branches: filter. With pull_request added, a branch in this repo now schedules the workflow twice per push — once as refs/heads/<branch>, once as refs/pull/N/merge — and because the concurrency group is web-integration-${{ github.ref }}, the two land in different groups, so neither cancels the other. That is three jobs, timeout-minutes: 30 each, duplicated on every push to an in-repo PR branch. #873, #874 and #876 are all in-repo branches, so this is the common case, not the edge case.
Your evidence run didn't surface it because #878 is itself from a fork, where push never fires in this repo.
rust-test.yml and python-test.yml already solve exactly this:
push:
branches: [ "main" ]
paths: [ ... ]
pull_request:
branches: [ "main" ]
paths: [ ... ]Adding branches: [ "main" ] to both triggers here would match that convention, keep every merged commit's own status on main (which the concurrency comment says it wants), and make the cancel-in-progress guard the thing that supersedes PR pushes — which is what it was written to do.
The one behaviour it gives up is a bare push to an in-repo branch with no PR open. Given the branching policy in CLAUDE.md (branch, push, open a PR against main), that window is short and covered the moment the PR exists.
Smaller, optional
.github/workflows/README.md lists web-integration.yml under "## Integration tests", while "## CI (run on push / PR)" is the section above it. The bullet's own "push/PR" wording is now accurate, so nothing is wrong — but a half-line noting that this one integration workflow also runs on PRs (and the others deliberately do not, being service- and secret-backed) would stop the next reader wondering whether the divergence is an oversight. That divergence is the substantive part worth recording: twelve sibling workflows keep the push-only shape and the same inert cancel guard, on purpose.
Happy to merge this as-is if you'd rather not touch the push trigger in a fix-scoped PR — the duplicate runs are wasteful, not wrong. But I'd merge this before #875, so that PR becomes the first JS change in this repo to land on a real JS signal.
Generated by Claude Code
|
Addressed in
Validation: |
|
Follow-up in Local validation for that lockfile-only change: |
What this changes
Adds a path-filtered
pull_requesttrigger toweb-integration.yml, mirroring the existing nine web/wasm/JavaScriptpushpaths. The workflow file itself is also included so future edits to this gate are visible to CI.Why
Closes #877.
PR #807 added
pnpm testto the web workflow, but the workflow still registered onlypush,workflow_call, andworkflow_dispatch. That protectsmainand release preflight, not pull requests: #875 changed the JavaScript client while its reported checks contained no JS typecheck or Vitest run.The existing concurrency policy, bump guard, and workflow documentation already define
pull_requestbehavior; this restores the missing event registration without addingpull_request_target, permissions, or secrets.How it was verified
This is a workflow-only change; no Rust or package code changed.
go run github.com/rhysd/actionlint/cmd/actionlint@v1.7.12 .github/workflows/web-integration.yml— cleanPyYAML
BaseLoaderstructural assertion —pull_request.pathsexactly matches all 9push.pathsentries and includes the workflow file,js/**, andcrates/wingfoil-wasm/**git diff --check— cleanThis PR scheduled the newly registered event immediately. All three target jobs passed:
Web Integration Tests,wingfoil-wasm build + unit tests, andwingfoil-js build + typecheck(including Vitest).The full reported matrix is green: 8 successful checks, 1 neutral/skipped reporting job, 0 failures.
cargo fmt --all(not applicable: no Rust changes)cargo lintandcargo lint-all(not applicable locally; upstreamLint (fmt & clippy)passed)cargo test -p wingfoil --all-features(not applicable locally; upstreamTest (wingfoil)passed)New behaviour is covered by a test asserting values and tick times (not applicable: event routing only)
Notes for the reviewer
The new event fired on this PR, so the change has direct end-to-end evidence rather than only static YAML validation. Web adapter, wire-types, wasm, JS, and future edits to this workflow now schedule the existing three jobs; unrelated PRs remain filtered out.