Skip to content

ci: run the test suite instead of only compiling it - #12

Merged
noneck merged 2 commits into
mainfrom
ci/run-tests
Jul 22, 2026
Merged

ci: run the test suite instead of only compiling it#12
noneck merged 2 commits into
mainfrom
ci/run-tests

Conversation

@noneck

@noneck noneck commented Jul 21, 2026

Copy link
Copy Markdown
Member

DO NOT MERGE BEFORE 2026-07-27 — live demos run on this server until then.

The gap

.github/workflows/ci.yml has been build-only, and its own header comment said why:

the package has no test script yet, so there is nothing to run beyond the compile. When a test script is added, add a npm test step here so regressions are caught before merge.

That script has existed since the SoQL work, along with test/soql.test.mjs and test/encoding.test.mjs. CI has been compiling the tests without ever running them, and the comment explaining the omission stayed behind to make it look intentional.

Changes

Runs npm test. No separate build step is added — npm test is already npm run build && node --test …, so a standalone npm run build would just run tsc a second time. A compile error still fails the job, with tsc's output at the top of the step log. The job is renamed BuildBuild & test to match; main has no branch protection, so no required-check name is affected.

Adds test/** to the path filters. Load-bearing rather than tidying: the tests are .mjs, so **/*.ts does not match them, and a PR touching only a test file would have skipped CI entirely without saying so — the same silent-no-op class of failure the gate exists to catch.

Fixes the test script for Node 20. See below.

Refreshes the header comment, including that the tests are offline — every one mocks globalThis.fetch, so CI needs no Socrata API key and no network access.

The gate paid for itself on its first run

The initial CI run of this PR passed on Node 22 and failed on Node 20:

Could not find '/home/runner/work/nyc-record-mcp/nyc-record-mcp/test/*.test.mjs'

node --test did not accept glob patterns until Node 22. With the pattern quoted, node itself does the matching, so on Node 20 the quoted string is treated as a literal path and no tests run at all. npm test has been broken on a supported Node version since the SoQL work, and nothing surfaced it because CI only ever compiled.

Unquoting the glob hands expansion to the shell, so node receives literal file paths on every version. A bare test/ directory argument is not a portable alternative — it works on Node 20 but fails on current Node, where positional handling changed. The workflow comment records this so the quotes don't come back.

Verification

  • Locally: 8 tests, 8 passing.
  • CI after the fix: Build & test (Node 20.x) pass, Build & test (Node 22.x) pass.
  • The workflow was parse-checked and the resulting job, steps, matrix, and path filters read back from the parsed YAML rather than eyeballed.

Scope

Branched from main, independent of #11 (strict tool schemas). Either can merge first. #11 adds a third test file that this workflow picks up automatically via the existing glob.

Two sibling repos have the same quoted-glob test script and build-only CI — flagged separately, not fixed here.

🤖 Generated with Claude Code

noneck and others added 2 commits July 21, 2026 12:32
The header comment said "the package has no test script yet ... when a `test`
script is added, add a `npm test` step here." That script has existed since the
SoQL work, along with test/soql.test.mjs and test/encoding.test.mjs, and CI has
been compiling them without ever running them.

No separate build step is added, because `npm test` already is
`npm run build && node --test "test/*.test.mjs"` — a standalone `npm run build`
step would only run tsc twice. A compile error still fails the job, with tsc's
output at the top of the step log. Job renamed Build -> Build & test to match;
main has no branch protection, so no required-check name is affected.

Also adds `test/**` to the path filters. The tests are `.mjs`, so `**/*.ts` does
not match them and a test-only PR would have skipped this gate entirely — the
same silent-no-op failure mode the gate is meant to catch.

Verified locally on the branch: 8 tests, 8 passing. Every test mocks
`globalThis.fetch`, so CI needs no Socrata key and no network.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…er did

The first CI run of this PR failed on Node 20 and passed on Node 22:

    Could not find '/home/runner/work/nyc-record-mcp/test/*.test.mjs'

`node --test` did not accept glob patterns until Node 22. With the pattern
quoted, node itself does the matching, so on Node 20 the quoted string is
treated as a literal path and no tests run — `npm test` has been broken on a
supported Node version since the SoQL work. Nothing surfaced it because CI only
ever compiled.

Unquoting the glob hands expansion to the shell, so node receives literal file
paths on every version. A bare `test/` directory argument is not a portable
alternative: it works on Node 20 but fails on current Node, where positional
handling changed.

This is the gap being added to CI paying for itself on its own first run.

Verified locally: 8 tests, 8 passing.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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