Skip to content

Commit 96e1103

Browse files
committed
fix: put bun's preload after its test subcommand, and correct two doc claims
The deny flag went before the subcommand, and `bun --preload X test <file>` stops treating `test` as bun's subcommand: it resolves it as the package.json script of that name, which here is the whole Node suite. Every matrix file then spawned that, hit the 120s per-file timeout, and failed. Measured before the fix: 0 pass, 23 genuine fail. After: 23 pass, 0 fail. The wiring guard did not catch it, because it only asserted the runner source mentions the fixture, which is true of the broken argv too. It asserts the order now, with the failure mode written down, since a flag-order mistake here surfaces as a timeout that looks nothing like its cause. Two doc claims are corrected. The framework-dev paragraph describing which required tests reach jspm predates the deny and said they still do; under the deny those calls get a 503 without leaving the process, and the point worth making is that they pass anyway because the resolve fails open. And both the fixture header and framework-dev claimed the deny has no blind spots while the next bullet described one: a spawned child starts with its own globalThis, so the claim is scoped to the test process now.
1 parent 34be87a commit 96e1103

4 files changed

Lines changed: 26 additions & 7 deletions

File tree

framework-dev.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ Two things to keep in mind when touching this. The stub serves only the packages
115115

116116
No required check may FAIL because a third party is down. The required `Unit + integration` job used to resolve vendors against the live jspm CDN, so a jspm outage redded pull requests that had nothing to do with vendoring; PR #1149, a five-file documentation change, is the one that finally made the case (it failed on the `#448` gitignore-healing test and passed on a re-run of the identical commit).
117117

118-
Be precise about what that does and does not say, because the weaker-sounding version is the true one. Required checks still REACH jspm: no in-repo app carries a pin file, so every test that cold-boots one (`test/preload-subset.test.mjs`, the `test/docs/*` boot tests, `test/integration/blog-http.test.mjs`, `packages/server/test/elision/differential-elision.test.js`) resolves its vendors live on the first request, transitively, through `resolveVendorImports`. What makes that acceptable is that the resolve fails OPEN: an unreachable CDN yields a partial importmap and a warning, never a throw, and none of those tests assert on a vendor entry. Measured with jspm forced to fail, each of them still passes in a few seconds. The rule is about what can turn a check red, not about counting packets.
118+
Plenty of required tests still TRY. No in-repo app carries a pin file, so every test that cold-boots one (`test/preload-subset.test.mjs`, the `test/docs/*` boot tests, `test/integration/blog-http.test.mjs`, `packages/server/test/elision/differential-elision.test.js`) asks `resolveVendorImports` to resolve its vendors on the first request. Under the deny those calls get a 503 without leaving the process, and each test still passes in a few seconds, because the resolve fails OPEN: an unreachable CDN yields a partial importmap and a warning, never a throw, and none of them assert on a vendor entry. That is what makes denying safe rather than disruptive, and it is why the deny prints one line per refused url: the list is there if that ever stops being true.
119119

120120
The rule is carried by the FILENAME, so the test runners can enforce it rather than leaving it to discipline. `scripts/run-node-tests.js` and `scripts/run-bun-tests.js` both drop any `*.live.test.*` file unless `WEBJS_REQUIRE_NETWORK=1` is set. Everything else resolves against `test/fixtures/jspm-double.mjs`, an offline double that models jspm rather than merely answering it (a 5xx or 429 is transient and retries per package, a 4xx probes per install, and an unresolvable install fails the WHOLE batch, which is the premise `jspmGenerate`'s fallback ladder is built on).
121121

@@ -125,7 +125,7 @@ Four things to keep in mind when touching this.
125125

126126
**A new vendor test uses the double, not the network.** `withJspmDouble(opts, body)` installs it, clears the vendor caches on both sides, and fails the test on any request the double was not asked to serve. Refusals are RECORDED rather than thrown on purpose: every fetch caller in `packages/server/src/vendor.js` catches, so a throw would be indistinguishable from the CDN being down and would quietly weaken whatever test hit it. The runtime deny answers 503 for the same reason, since that is the shape those call sites classify as transient.
127127

128-
**The deny is at RUNTIME, and that was learned the hard way.** Both runners preload `test/fixtures/deny-live-hosts.mjs`, which answers 503 for jspm.io and registry.npmjs.org unless `WEBJS_REQUIRE_NETWORK` is set. It needs no parsing and has no blind spots, and it covers the transitive callers a source scan structurally cannot see: the app-boot tests reach jspm through `resolveVendorImports` with no `fetch(` anywhere in their own source. A test that depends on a third party now fails on EVERY run rather than only during an outage, which arrives the day it is written instead of months later.
128+
**The deny is at RUNTIME, and that was learned the hard way.** Both runners preload `test/fixtures/deny-live-hosts.mjs`, which answers 503 for jspm.io and registry.npmjs.org unless `WEBJS_REQUIRE_NETWORK` is set. It needs no parsing, and within the test process it has no blind spots (a spawned child is the exception, below). It covers the transitive callers a source scan structurally cannot see: the app-boot tests reach jspm through `resolveVendorImports` with no `fetch(` anywhere in their own source. A test that depends on a third party now fails on EVERY run rather than only during an outage, which arrives the day it is written instead of months later.
129129

130130
The first three attempts were a STATIC scan over test sources, and each went blind a different way: a file-level exemption, so one `withMockedFetch` anywhere excused every live call in the file; then no regex-literal awareness, so `/rel=["']modulepreload["']/` desynced the mask to end of file; then regex awareness that read the `/` in `</li>` inside a nested `` html`...` `` template as a regex opener, swallowing the closing backtick. Each fix opened a new hole, because deciding whether a `/` starts a regex means lexing JavaScript, and a hand-rolled lexer facing nested template literals full of markup will keep being wrong. **Do not reintroduce it.** If the deny needs to be tighter, tighten the deny.
131131

scripts/run-bun-tests.js

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,9 @@ const LIVE_MARKER = '.live.test.';
101101
const wantsNetwork = Boolean(process.env.WEBJS_REQUIRE_NETWORK);
102102
// Same third-party deny the node runner installs, so a jspm outage cannot red
103103
// this job either (#1150). Bun ignores NODE_OPTIONS, hence the explicit flag.
104+
// It goes AFTER the `test` subcommand: `bun --preload X test <file>` treats
105+
// `test` as the package.json SCRIPT and runs the whole Node suite instead,
106+
// which fails in a way that looks nothing like a flag-order mistake.
104107
const denyArgs = wantsNetwork
105108
? []
106109
: ['--preload', resolve(ROOT, 'test', 'fixtures', 'deny-live-hosts.mjs')];
@@ -137,7 +140,7 @@ for (const f of files) {
137140
console.log(`SKIP(node-only) ${rel(f)}`);
138141
continue;
139142
}
140-
const r = spawnSync(BUN, [...denyArgs, 'test', f], {
143+
const r = spawnSync(BUN, ['test', ...denyArgs, f], {
141144
cwd: ROOT, encoding: 'utf8', timeout: PER_FILE_TIMEOUT_MS,
142145
env: { ...process.env, FORCE_COLOR: '0' },
143146
});

test/fixtures/deny-live-hosts.mjs

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,16 @@
2929
* `resolveVendorImports`, with no `fetch(` and no vendor entry point anywhere
3030
* in their source.
3131
*
32-
* Denying at runtime needs no parsing and has no blind spots. A test that
33-
* depends on a third party now fails on EVERY run rather than only during an
34-
* outage, which is a better signal than any scan could give, and it arrives
35-
* the day the test is written instead of months later.
32+
* Denying at runtime needs no parsing, and inside the test process it has no
33+
* blind spots. A test that depends on a third party now fails on EVERY run
34+
* rather than only during an outage, which is a better signal than any scan
35+
* could give, and it arrives the day the test is written instead of months
36+
* later.
37+
*
38+
* The one thing it does NOT cover is a SPAWNED child, which starts with its
39+
* own `globalThis`. `test/vendor-cli/vendor-cli.test.mjs` runs the CLI in
40+
* another process, so it passes its own preload and asserts a marker on every
41+
* spawn. A new test that spawns a process and vendors needs the same.
3642
*
3743
* WHY A 503 RATHER THAN A THROW. Every fetch caller in
3844
* `packages/server/src/vendor.js` catches, so a throw is indistinguishable

test/repo-health/live-cdn-callers.test.mjs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,16 @@ test('both runners install the deny and skip live files, unless the network is r
129129
assert.match(src, /deny-live-hosts/, `${runner} must install the third-party deny`);
130130
assert.match(src, /const denyArgs = wantsNetwork/, `${runner} must lift the deny when the network is required`);
131131
}
132+
133+
// Flag ORDER, not just presence. `bun --preload X test <file>` stops
134+
// treating `test` as the subcommand and runs the package.json script of that
135+
// name instead, which here is the whole Node suite: every matrix file then
136+
// spawns it, times out at 120s, and the job goes red having run zero Bun
137+
// tests. A guard that only greps for the fixture path passes on exactly that
138+
// argv, which is how it shipped once.
139+
const bun = readFileSync(join(ROOT, 'scripts/run-bun-tests.js'), 'utf8');
140+
assert.match(bun, /spawnSync\(BUN, \['test', \.\.\.denyArgs/,
141+
"the preload must come AFTER bun's `test` subcommand");
132142
});
133143

134144
test('every allowlisted live caller is a *.live.test.* file that exists', () => {

0 commit comments

Comments
 (0)