Resolve the GPT bootstrap test fixture without assuming a platform - #1162
jwrosewell wants to merge 2 commits into
Conversation
Two tests in ad_init.test.ts fail on any Windows checkout, because the helper takes the pathname of a file: URL and reads it as a filesystem path. A URL pathname is not a filesystem path: on Windows it carries a leading slash before the drive letter, so Node resolves it against the current drive and the drive segment doubles. The read then fails with ENOENT ... open 'D:\D:\...\gpt_bootstrap.js'. The fixture is now resolved from the vitest root, which is the lib directory per vitest.config.ts, the same way gpt_bootstrap.test.ts already resolves the same file. That removes the URL-to-path conversion and the Vite /@fs special case along with it, so no branch of the helper depends on the platform. CI runs this suite on ubuntu-latest, where a pathname carries no drive letter, so the fault has always been invisible there and appears only to someone running the suite locally on Windows.
aram356
left a comment
There was a problem hiding this comment.
Summary
The diagnosis here is right, and it's a real bug: a file: URL pathname isn't a filesystem path, and the D:\D:\... doubling follows directly from treating it as one. CI can't see it because there are no Windows runners, so thanks for catching it from a local run.
I'd like to ask for a slightly different fix, and for it to cover the rest of the file while you're in here. Details inline and below. Everything is verified locally against your branch: 901/901 tests, ESLint and Prettier clean.
Blocking
🔧 wrench
- Resolve the fixture from
import.meta.dirname— see inline atcrates/trusted-server-js/lib/test/integrations/gpt/ad_init.test.ts:123 - Three sibling files share the same fault — body finding below (those files have no diff hunks, so this can't be an inline comment)
Cross-cutting / body-level findings
-
🔧 wrench — the same pattern is in three more files —
process.cwd()fixture resolution also appears atgpt_bootstrap.test.ts:21,schedule_initial_ad_init.test.ts:16, andspa_hook.test.ts:152, plus three more sites insidead_init.test.tsitself (lines 146, 202, 1151). They resolve the same fixture with the same literal. Since this PR is the one establishing how that path gets resolved, it seems worth converging them all here rather than leaving six sites on the old pattern — otherwise the next person copies whichever one they land on. Suggested shape in the inline comment. -
📝 note — the comment about jsdom — I probed
import.meta.urlunder this jsdom/vitest setup and it comes back as a plainfile://URL (file:///Users/.../ad_init.test.ts), so jsdom doesn't seem to rewrite the scheme. The failure looks like it was purely.pathnamebeing used as a path:URL.pathnamereturning/D:/...is correct per the WHATWG URL spec, which is why Node shipsfileURLToPath. Worth mentioning because that explanation also sits ingpt_bootstrap.test.ts:16-18and is what the new comment repeats; if the resolution moves toimport.meta.dirname, both can just go away. -
🌱 seedling — no Windows runner in CI — This bug class is invisible to CI today. Adding
windows-latestto the vitest job matrix would catch the next one. Happy to open that separately so it stays out of this PR.
CI Status
- cargo test: PASS (required)
- cargo fmt: PASS (required)
- format-typescript: PASS (required)
- format-docs: PASS (required)
- vitest: PASS
- cargo test (axum native): PASS
- cargo test (cloudflare native): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- browser integration tests: PASS
- prepare integration artifacts: PASS
- CodeQL: PASS
- Analyze (rust): PASS
- Analyze (javascript-typescript): PASS
- Analyze (actions): PASS
- CLAUDE.md symlink guard: PASS
| // Resolved from the vitest root (the lib directory, per vitest.config.ts) | ||
| // rather than from `import.meta.url`, which the jsdom environment rewrites | ||
| // to a non-file scheme. This is the same approach gpt_bootstrap.test.ts | ||
| // takes for the same file, and it makes no assumption about the platform: | ||
| // a URL pathname is not a filesystem path, and on Windows it carries a | ||
| // leading slash before the drive letter. | ||
| const bootstrapPath = path.resolve( | ||
| process.cwd(), | ||
| '../../trusted-server-core/src/integrations/gpt_bootstrap.js' |
There was a problem hiding this comment.
🔧 wrench — Could we resolve this from import.meta.dirname instead of process.cwd()? Node defines it as a platform-native directory path, so there's no URL-to-path conversion left to get wrong, and it keeps the resolution file-relative the way the original code intended.
Since the same fixture is read from six other places, a single shared constant would handle all of them:
// crates/trusted-server-js/lib/test/fixtures/paths.ts (new file)
import { resolve } from 'node:path';
/**
* Absolute path to the edge-injected `gpt_bootstrap.js` that several suites
* evaluate verbatim.
*
* Resolved from `import.meta.dirname`, which Node defines as a
* platform-native directory path. A `file:` URL pathname is not a filesystem
* path: on Windows it carries a leading slash before the drive letter, so
* reading it directly makes Node resolve it against the current drive and the
* drive segment doubles. Resolving from the module's own directory also keeps
* the path independent of the working directory the suite is launched from.
*/
export const GPT_BOOTSTRAP_PATH = resolve(
import.meta.dirname,
'../../../../trusted-server-core/src/integrations/gpt_bootstrap.js'
);Each call site then collapses to one line, and this helper loses its comment entirely since the code no longer needs explaining:
const bootstrap = await readFile(GPT_BOOTSTRAP_PATH, 'utf8');Call sites to convert: this one, plus ad_init.test.ts:146, :202, :1151, gpt_bootstrap.test.ts:21, schedule_initial_ad_init.test.ts:16, spa_hook.test.ts:152. Each file's now-unused node:path import can go too, or ESLint fails with 'path' is defined but never used.
Two reasons for import.meta.dirname over process.cwd():
- It stays file-relative.
process.cwd()follows the shell's working directory, not the vitest root, so the suite passes fromlib/but fails from the repo root. Runningvitest --root crates/trusted-server-js/libfrom the top givesENOENT .../.claude/trusted-server-core/.... Currentmainpasses there, so the shared constant avoids a small behaviour change while fixing Windows. - Confirmed on Windows, not just reasoned about — a colleague ran it on a real Windows checkout.
Apply manually — this adds a new file and touches lines outside the diff, so it can't be a one-click suggestion.
I've verified the end state on your branch: 901/901 tests, no type errors, ESLint and Prettier clean, and the four affected files also pass when run from the repo root (where main currently fails 19 tests). Net about −36 lines. Happy to push it as a patch you can cherry-pick if that's easier than redoing it.
Every suite that evaluates gpt_bootstrap.js now reads it through GPT_BOOTSTRAP_PATH in test/fixtures/paths.ts, which resolves from import.meta.dirname, as the review on IABTechLab#1162 asked. Node defines that as a platform-native directory path, so no URL is turned into a path, and the result does not depend on the directory the suite is launched from. The seven reads that resolved from process.cwd() use the constant, being the four in ad_init.test.ts and one each in gpt_bootstrap.test.ts, schedule_initial_ad_init.test.ts and spa_hook.test.ts. Their node:path imports go, and so do the two comments explaining why the fixture was not resolved from import.meta.url, which the shared path makes unnecessary.
A test must resolve a file it reads without assuming an operating system.
runGptBootstrapWithGoogleTaginad_init.test.tstook the pathname of afile:URL and read it as a filesystem path, so on Windows the drive letter doubled and two tests failed withENOENT ... open 'D:\D:\...\gpt_bootstrap.js'.References #1161, which carries the full account.
The change
One file, nine lines added and twelve removed. The fixture is now resolved from the vitest root, which is the
libdirectory pervitest.config.ts:This is the approach the sibling test already takes for the same file.
gpt_bootstrap.test.tsresolves it the same way and explains in its own comment thatimport.meta.urlis avoided because the jsdom environment rewrites it to a non-file scheme.Removing the URL-to-path conversion also removes the Vite
/@fsspecial case, which had the same fault for the same reason, since slicing that prefix still leaves a leading slash in front of the drive letter. No branch of the helper now depends on the platform.Verification
Run on Windows, where the fault appears.
npx vitest runeslint . --max-warnings=0on the fileThe two tests that failed and now pass are
preserves legacy state in the edge bootstrap when getConfig does not report itandtracks setConfig state and re-enabling in the edge bootstrap.CI runs this suite on
ubuntu-latest, where a pathname carries no drive letter, so the job was green before this change and stays green after it. What changes is that the suite now also passes for anyone running it locally on Windows.A note for whoever reviews
prettier --checkreports this file as unformatted on a Windows checkout, and it reports the unchanged file onmainexactly the same way. That is the CRLF working tree rather than anything in this change, so no reformatting is included here.This branch is off
mainand is deliberately independent of the provider and permission pull requests.Produced with AI assistance and needs human review before merging.