Run the test files in one process, and test the dedup ring and the changelog script in process by nissa-seru · Pull Request #18 · anima-research/github-mcpl

nissa-seru · 2026-10-28T15:56:25Z

npm test now runs every test file in one Node process, and the dedup ring's test and the changelog script's 24 tests now run in process, instead of through 2004 HTTP posts and a child Node process per test. Three tests that slept 50 ms before asserting that nothing from an event reached the host now wait for the host to answer a request first. In our runs, every test this change and 3cfdfe3 share gave the same result at both. In three rounds of runs alternating between the two commits, on a Linux machine with 12 CPUs at a one-minute load average of 97 to 129, the command that npm test runs used 68 to 76% less CPU time than at 3cfdfe3, and finished 19 to 57% sooner. Of the Node 22 releases engines allows, npm test now needs 22.8.0 or later, and CI's job has a 10-minute timeout.

Problem

At 3cfdfe3, the main commit this change starts from, npm test is node --import tsx --test test/*.test.ts (package.json:13), so Node runs each of the 18 test files in a process of its own. In our runs of that command, with --test-reporter=tap added:

the suite used 11.4 to 15.0 s of CPU time. With --experimental-test-isolation=none added, the same files used 6.1 to 6.5 s;
"the dedup gate remembers 2000 ids and then forgets the oldest" took 3.9 to 5.2 s. It posts 2004 signed deliveries to the webhook listener over loopback HTTP, one after another (test/webhook.test.ts:217 to :227);
test/release-changelog.test.ts's 24 tests took 5.8 to 8.5 s together. Each calls run(), which runs the script with execFileSync(process.execPath, [SCRIPT], …) (:61, :63).

Three tests in test/integration.test.ts deliver a webhook event the server must not pass on, wait 50 ms (:254, :319, :580), and then assert that the host received nothing from it.

Changes

Line numbers are at 738b552.

14b5668, the three absence checks. Each now asserts that the delivery was answered with a 2xx status (accepted, test/integration.test.ts:131), then makes one host request and awaits its answer (hostCaughtUp, :127), at :263 to :264, :328 to :329 and :589 to :590. Each test's assertions that nothing arrived from the event follow (:265 to :266, :330, :591 to :592). The comment above hostCaughtUp says "The server writes to the host in order on one stream, so once the answer to a later request is back, everything it sent before has arrived."
a64cc43, the dedup ring. src/webhook.ts exports DeliveryRing with its capacity readable (:81, :85), and the listener has a read-only dedupCapacity (:176). The test of 2004 posts is now "the dedup ring remembers 2000 ids and then forgets the oldest" (test/webhook.test.ts:215), which adds ids to a DeliveryRing of 2000 directly. It checks that 2000 ids are remembered, that a replay of gate-0 at that boundary is still remembered, and that two more ids then forget gate-0 and gate-1. The HTTP test of a replayed delivery also asserts that the listener remembers one id and holds a ring of 2000 (:211, :212). The old test's comment said "the oldest after gate-0 was re-seen at the tail is gate-1" (:223 to :224 at 3cfdfe3). The ring's code is the same at both commits apart from its export and its capacity no longer being private, and the new test, which asserts that two more ids forget a replayed gate-0 as well as gate-1, passes with it. So the comment described something the ring doesn't do.
59c7e3a, the changelog script. scripts/release-changelog.mjs exports release(dir) and ReleaseError (:145, :37), and scripts/release-changelog.d.mts declares the two. The script runs release(process.cwd()) only when isMain() is true (:205 to :214), which compares the real path of process.argv[1] with the script's own (:208). The 24 tests call release() in process through run() (test/release-changelog.test.ts:59, :61), with their assertions unchanged. Three new tests start Node (:211, :222, :233): the script releasing when run through a symlink, the script refusing with exit status 1 and its reason on stderr, and an import of the script with process.argv[1] set to test/*.test.ts, which writes nothing to stdout and changes neither the changelog nor the fragments.
738b552, one process and a bound on CI. package.json's test adds --experimental-test-isolation=none (:13), and CI's job has timeout-minutes: 10 (.github/workflows/ci.yml:16).
No changelog fragment. Outside test/, package.json and CI, the change edits src/webhook.ts (the export, the capacity no longer private, and the getter) and the changelog script (release(dir), the exports, isMain() and the .d.mts file).

Other open pull requests, by git merge-tree against each one's head at 14:40 UTC on October 28:

Merge, update-branch, workflow runs and review reads, behind opt-in allow-lists #11 and update_branch: write only to a head that is nobody's base #14 merge cleanly with this change, as with 3cfdfe3.
Describe a general connector in deployment #4 and Contributor mode: user token + notifications poller (#5) #6 already conflict with 3cfdfe3, and conflict with this change in the same files.
Comments: keep only those one rule allows, outside the files #11 changes #15 merges cleanly with 3cfdfe3, and conflicts with this change in four hunks: two in scripts/release-changelog.mjs and two in src/webhook.ts. In each, Comments: keep only those one rule allows, outside the files #11 changes #15 rewrites or removes a comment beside a line this change edits or adds.
Use @animalabs/mcpl-core 0.4.0, which needs Node 22.12.0 or later #16 merges cleanly with 3cfdfe3, and conflicts with this change in one hunk of .github/workflows/ci.yml, where Use @animalabs/mcpl-core 0.4.0, which needs Node 22.12.0 or later #16 adds a Node 22 and 24 matrix and this change adds timeout-minutes: 10.

Whichever of #15 and this change merges second has to resolve those hunks, and the same goes for #16.

What a contributor would notice:

Of the Node 22 releases engines allows (>=22.0.0, package.json:28), npm test needs 22.8.0 or later. Each release from 22.0.0 to 22.7.0 refuses --experimental-test-isolation=none with "bad option", and 22.8.0 takes it. Use @animalabs/mcpl-core 0.4.0, which needs Node 22.12.0 or later #16 changes engines to >=22.12.0.
The test files share one process, so a test that changes process-wide state has to put it back. A grep of test/ for assignments to process.env, globalThis, process.exitCode and process.stderr.write or process.stdout.write, and for calls to process.chdir, process.umask and process.on, finds only test/githubAdapter.test.ts:1815's replacement of process.stderr.write, which :1822 puts back in a finally.
A test that leaves a server listening keeps the run from ending, as it did with a process per file. CI's job now has a 10-minute timeout.
On 3cfdfe3's files, the flag alone used less CPU time in each of our rounds, but finished later in two of the three (table below).

What this leaves

test/githubAdapter.test.ts keeps the node:http server on 127.0.0.1 that it points the adapter at. Its header says why: "the paths, query strings, bodies and headers asserted below are the ones that would go to github.com."

Tests

On Linux 6.12.113 (x86_64, 12 CPUs) with Node 24.22.1 and npm 11.19.1, in clean checkouts of 738b552 and 3cfdfe3, each installed with npm ci (@animalabs/mcpl-core 0.3.0, octokit 4.1.4, tsx 4.23.15, TypeScript 5.9.3):

Typecheck and build: npm run typecheck and npm run build succeed at 738b552.
The suite: at 738b552, 617 tests run: 597 pass, 2 fail and 18 are skipped. At 3cfdfe3, 614 run: 594 pass, 2 fail and 18 are skipped. The same two fail at both commits on this machine, both in test/wp8-docs.test.ts: "README: the credential split creates both files at mode 600" stops on jq: not found, and "README: an empty host reaches the registration script by following step 1" on fatal: repository '…' does not exist for the directory it ran in.
The same results: the test names differ by exactly five: "the dedup gate remembers 2000 ids…" at 3cfdfe3, against the ring test and the three new script tests at 738b552. Every name the two commits share has the same result. That held in all twelve runs below, and in three runs of 738b552's files in one process in orders we imposed: reversed, and two seeded shuffles. We imposed each order by giving Node symlinks with numbered names.

Time, in three rounds, each running 3cfdfe3 and 738b552 with a process per file and in one process, one after another. CPU time is user plus system for the whole run, as bash's time reports it:

round	`3cfdfe3`, a process per file	`738b552`, one process	`3cfdfe3`, one process	`738b552`, a process per file
1	17.2 s, CPU 15.0 s	7.4 s, CPU 3.7 s	12.3 s, CPU 6.5 s	8.4 s, CPU 9.0 s
2	10.1 s, CPU 11.4 s	4.5 s, CPU 3.0 s	11.5 s, CPU 6.1 s	10.1 s, CPU 8.8 s
3	13.9 s, CPU 12.1 s	11.3 s, CPU 3.8 s	19.0 s, CPU 6.5 s	16.0 s, CPU 8.8 s

The first two columns are the command that npm test runs at each commit, run with node directly and with --test-reporter=tap added. The one-minute load average was 97 to 129 over the twelve runs. Against 3cfdfe3's command, 738b552's used 68 to 76% less CPU time and finished 19 to 57% sooner. The flag alone used 47 to 56% less CPU time on 3cfdfe3's files and 57 to 66% less on 738b552's.

One process: two test files that each print process.pid printed two different pids with Node's default isolation, and the same pid with --experimental-test-isolation=none.
Node 22, through npx node@<version>: --test --experimental-test-isolation=none on a one-test file exits 9 with "bad option" on each of the ten Node releases from 22.0.0 to 22.7.0, and passes on 22.8.0 and 22.12.0. On Node 22.12.0, the command that npm test runs at each commit, with --test-reporter=tap added, gives the counts above.
A leaked handle: a test file whose one test leaves an HTTP server listening was still running after 30 s with --experimental-test-isolation=none and with --experimental-test-isolation=process. With --test-force-exit added, it ended in 0.3 s.
Mutants, each run on its test file alone at 738b552:
- With eventEgressAllowed returning true (src/server.ts:1423), "a host that grants nothing…" and "a disabled github.events…", two of the three absence checks, fail, with four other tests in test/integration.test.ts. With :1291's !this.mcplEnabled check removed too, the third, "a plain-MCP host sees tools only…", fails as well.
- With the test's deliveries carrying a wrong signature, all three absence checks fail, with 14 other tests.
- In test/webhook.test.ts, a ring that holds one id fewer than its capacity, or one that moves a replayed id to the tail, fails the ring test. A listener whose ring holds 1999 fails the replayed-delivery test.
- In test/release-changelog.test.ts, isMain() comparing process.argv[1] without realpathSync fails the symlink test. isMain() without its try fails the import test. A refusal that exits 0 fails the refusal test. A script that, run as a program, deletes the fragments as soon as it has read them fails the symlink and refusal tests. A release that keeps the fragments fails "folds fragments into a versioned section…" and the symlink test.

Not verified

CI's time. At 3cfdfe3, GitHub's runner ran the Test step in about 4 s by its step times (CI run 40593411689). We didn't measure this change there.
macOS and Windows. Every run was on Linux.
Other Node versions. The suite ran on Node 24.22.1 and 22.12.0 only, and the flag on its own on the releases from 22.0.0 to 22.8.0 and on 22.12.0.
npm version. No run used it. Its hook runs node scripts/release-changelog.mjs (package.json:16), and two of the new tests start the script with node in a release directory (test/release-changelog.test.ts:211, :222).

Ogden-2688 made the change, and Esther-2584 read it and ran it before its last commit's message was reworded. Agnes-984 made the house's calls on it. Daniel-2612 wrote this description.

…rived Three integration tests delivered a webhook event the server must not pass on, slept 50 ms and then asserted the host had received nothing. On a busy machine 50 ms can pass before anything would have arrived, so the checks could pass without proving anything. Each now asserts the delivery was answered 2xx, so a refused delivery can't pass for an absence. The harness's deliver already waits for the listener to finish handling the event, and the tests then make one host request and await its answer: the server writes to the host in order on one stream, so anything it sent while handling the event has arrived by then. The three checks guard the server's egress check as a whole, not each path inside it.

The test for the webhook listener's dedup ring posted 2004 signed deliveries over loopback, one after another, taking about 5 s of a suite run on the VPS. - webhook: DeliveryRing is exported with its capacity, and the listener has a read-only dedupCapacity beside rememberedDeliveries. - A unit test of the ring at DELIVERY_DEDUP_RING checks 2000 ids remembered, a replay at the boundary still remembered, and two more ids forgetting both gate-0 and gate-1: the ring keeps the last 2000 ids by first arrival, and a replay doesn't move an id. - The HTTP test of a replayed delivery also asserts the listener remembers one id and holds a ring of 2000. With it, the listener's replay branch, its new-id branch and its ring size stay tested through HTTP; eviction is tested on the ring alone. - The test file's header now leaves the ring's case out of "every case goes through a real node:http server". The old test's comment said re-seeing an id moves it to the tail. It doesn't.

Each of the 24 changelog tests ran the script as a child Node process, about 5 s of a suite run on the VPS. - scripts/release-changelog.mjs exports release(dir), which returns the line the CLI prints and throws ReleaseError for a refusal, and ReleaseError. The CLI runs it on the current directory only when the script is the program Node was started with, comparing real paths, since argv[1] keeps a symlinked path that import.meta.url resolves. An absent argv[1], or one that names no file, means it isn't. - scripts/release-changelog.d.mts declares the two exports for the typecheck, which covers the tests. - All 24 existing cases call release() in process with the same assertions. Three new tests spawn Node: the CLI releasing through a symlink to the script; the CLI refusing a duplicate version with a fragment pending, which exits 1 with its reason on stderr and touches nothing; and an import of the script with argv[1] set to a glob pattern, which runs nothing and touches nothing.

node --test starts a process per test file by default, and each one loads tsx and transforms src/ again: about 0.25 s of CPU per file before any test runs. With --experimental-test-isolation=none the files run one after another in one process. With this branch's test files, the flag alone took the suite's CPU from 9.7-9.8 s to 3.6-3.7 s on a shared 12-core host, about 63% less, in two alternating pairs of runs. The verdicts were the same on Node 24.22.1 and 22.12.0, and in three imposed file orders. Node 22.7 refuses the flag and 22.8 takes it, so npm test needs Node 22.8 or later. What one process shares: the only test that changes process-wide state, githubAdapter.test.ts's process.stderr.write swap, restores it in a finally. No test writes process.env, globalThis, process listeners, process.exitCode, the cwd or the umask; src/'s module-level sets are constants, and the poller's timer is unref'd. A handle a test leaves open keeps the run from ending under either mode, so CI's job gets timeout-minutes: 10 instead of GitHub's 360-minute default, and a leak shows as a failed job.

main now carries the faster test suite (anima-research#18), which edits code lines beside four comments this branch rewrites or removes. Each conflict keeps this branch's comment text with main's code: ReleaseError and DeliveryRing are exported, collectFragments takes the directory, and the listener's dedupCapacity getter stays without the section banner this branch removes.

main now carries the faster test suite (anima-research#18), which gives CI's job a 10-minute timeout where this branch adds the Node 22 and 24 matrix. The job keeps both.

npm test spent most of its time waiting: about 25 s of every run waited out the server's own clock (the call budget, the rate-limit and registration waits) and the fake Slack API's delays, and the child processes of cli, serverVersion and release-changelog took 11-18 s more. Each test file also started a process of its own. With the files in one process the CPU fell by about 70%, but the waits then added up one after another, so they go too. - src/clock.ts: clock.now() and clock.sleep(ms, { unref }), Date.now() and setTimeout by default. slack-adapter.ts' start rule, its check that a wait fits and its rate-limit sleep, and server.ts' arrival stamp and registration wait read it. Nothing else changes for the server. test/clock.test.ts pins the default: a sleep waits out its time, and holds the process unless it is unref. - test/virtualClock.ts: virtualClock(t) runs clock.now() and clock.sleep() on a time fixed when the test calls it, which only its waits move, and records each wait, so a test asserts its waits went through the clock. The fake Slack API's elapseMs moves the same time; silentSlackApi().end() ends the requests it holds. - The tests that waited run on it: six in slackClient, two in uploadSteps, one in registrationRecovery, whose superseded registration now holds its wait open until the test has revoked. slackClient's and attachmentDownload's queue tests end the held post with end() rather than a 1 s client timeout. New slackClient tests pin that a wait counts the time an earlier request's wait used, and that a call's time starts when it reaches the server. - One wait stays real on purpose: the first case of "a rate limit the call can afford" waits Slack's 1 s on the real clock and asserts that the fake Slack got the request again at least 990 ms after its Retry-After. A virtual sleep resolves at once, so it is the one check that sees a retry sent without waiting for the sleep. - slackClient's acknowledgement test stops its adapter, so its armed removal can't fire during a later file now that files share a process. - slackClient's client-timeout test counts the connections the silent API took, once it has taken one and the event loop has polled again, rather than the requests it had read when the call failed. By probe, a stall of the process just after the client arms its 300 ms timeout lets the timeout end the request before the same process's server reads it, which failed the test once on main on a loaded host. A request sent again would need a new connection. - test/cli.test.ts: one start of the CLI end to end; the switch cases, the other refusals and the settings of a complete environment in-process against startupSettings. - test/serverVersion.test.ts: two copies of src/ in a fixture with its own version, at src/ and at the built layout tsconfig's outDir and rootDir give, imported in-process, from the repository's cwd. It no longer runs tsc, so it no longer checks that tsc places server.js at dist/src/; by reading, tsc rewrites neither path string server.ts reads, and the layout is tsconfig's. - scripts/release-changelog.mjs, scripts/release-changelog.d.mts and test/release-changelog.test.ts: github-mcpl's, byte for byte, as merged in anima-research/github-mcpl#18 (7eaea03), which tests release(dir) in-process and the CLI three times. - package.json: npm test runs the files in one process (--experimental-test-isolation=none, on Node 22.12.0 and 24).

Ogden-2688 added 4 commits October 28, 2026 10:18

nissa-seru merged commit 7eaea03 into anima-research:main Oct 28, 2026
4 checks passed

Navigation Menu

Repository navigation

Run the test files in one process, and test the dedup ring and the changelog script in process - #18

Run the test files in one process, and test the dedup ring and the changelog script in process#18
nissa-seru merged 4 commits into
anima-research:mainfrom
nissa-seru:test/suite-speed

nissa-seru commented Oct 28, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

nissa-seru commented Oct 28, 2026

Problem

Changes

What this leaves

Tests

Not verified

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant