Skip to content

Resolve the GPT bootstrap test fixture without assuming a platform - #1162

Merged
aram356 merged 2 commits into
IABTechLab:mainfrom
jwrosewell:fix/test-path-os-independent
Sep 16, 2026
Merged

aram356 merged 2 commits into
IABTechLab:mainfrom
jwrosewell:fix/test-path-os-independent

Conversation

@jwrosewell

Copy link
Copy Markdown
Collaborator

A test must resolve a file it reads without assuming an operating system. runGptBootstrapWithGoogleTag in ad_init.test.ts took the pathname of a file: URL and read it as a filesystem path, so on Windows the drive letter doubled and two tests failed with ENOENT ... 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 lib directory per vitest.config.ts:

const bootstrapPath = path.resolve(
  process.cwd(),
  '../../trusted-server-core/src/integrations/gpt_bootstrap.js'
);

This is the approach the sibling test already takes for the same file. gpt_bootstrap.test.ts resolves it the same way and explains in its own comment that import.meta.url is avoided because the jsdom environment rewrites it to a non-file scheme.

Removing the URL-to-path conversion also removes the Vite /@fs special 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.

before after
npx vitest run 2 failed, 901 passed 901 passed, 0 failed
Type errors none none
eslint . --max-warnings=0 on the file clean clean

The two tests that failed and now pass are preserves legacy state in the edge bootstrap when getConfig does not report it and tracks 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 --check reports this file as unformatted on a Windows checkout, and it reports the unchanged file on main exactly 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 main and is deliberately independent of the provider and permission pull requests.


Produced with AI assistance and needs human review before merging.

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 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 at crates/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 at gpt_bootstrap.test.ts:21, schedule_initial_ad_init.test.ts:16, and spa_hook.test.ts:152, plus three more sites inside ad_init.test.ts itself (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.url under this jsdom/vitest setup and it comes back as a plain file:// URL (file:///Users/.../ad_init.test.ts), so jsdom doesn't seem to rewrite the scheme. The failure looks like it was purely .pathname being used as a path: URL.pathname returning /D:/... is correct per the WHATWG URL spec, which is why Node ships fileURLToPath. Worth mentioning because that explanation also sits in gpt_bootstrap.test.ts:16-18 and is what the new comment repeats; if the resolution moves to import.meta.dirname, both can just go away.

  • 🌱 seedling — no Windows runner in CI — This bug class is invisible to CI today. Adding windows-latest to 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

Comment thread crates/trusted-server-js/lib/test/integrations/gpt/ad_init.test.ts Outdated
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.
@aram356
aram356 merged commit 35560c8 into IABTechLab:main Sep 16, 2026
19 checks passed
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.

2 participants