[PR #125] [MERGED] test(credentials): unstub globals before envs so the hook cannot throw #126

New issue

Closed

opened 2026-10-31 22:04:49 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 22:04:49 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-typescript-sdk/pull/125
Author: @mattmillerai
Created: 9/4/2026
Status: ✅ Merged
Merged: 9/4/2026
Merged by: @mattmillerai

Base: main ← Head: matt/fix-credentials-test-unstub-order

📝 Commits (1)

851c1f4 test(credentials): unstub globals before envs so the hook cannot throw

📊 Changes

2 files changed (+6 additions, -2 deletions)

View changed files

📝 src/sdk/credentials.test.ts (+5 -1)
📝 src/sdk/models.test.ts (+1 -1)

📄 Description

Two-line fix. pnpm test is red on Node 26 — 17 failures — and green on the
Node versions CI runs.

ELI-5

Two tests in credentials.test.ts delete process to prove the SDK degrades
gracefully in a runtime that has no such thing (a browser). The cleanup hook
that runs after every test then tries to put the environment variables back,
and it reaches them through globalThis.process.env — which is exactly the
thing the test just deleted. So the cleanup crashes partway through, before
it gets to the line that would have restored process.

process stays gone for the rest of the file, and every test after those two
fails on the same missing global. Two real assertions become 17 red tests.

The fix is to restore the globals first, then the environment variables, so
there is a process to restore them into.

Why CI didn't catch it

It depends on how the runtime hands process to vitest's env restore.

Node	`pnpm test`
22	green (CI)
24	green (CI)
26	red, 17 failures

engines.node is >= 22 and CI matrixes 22 and 24, so nothing on the
required checks sees this. It shows up as soon as you run the suite on a
current Node locally.

Scope

src/sdk/credentials.test.ts — the live failure. Hook order swapped, with a
comment saying why the order matters, so the next person does not swap it
back.
src/sdk/models.test.ts — same hook shape, but it only stubs fetch, so
the bug is latent rather than live there. Swapped too, since a future test
that stubs a global the env restore depends on would hit the identical
trap.

No source file is touched.

Verification

pnpm test on Node 26: 432 passed, 0 failed (was 415 passed / 17 failed).
The two process tests still assert what they always did — they were passing
on their own bodies all along; it was their cleanup that failed. pnpm lint,
pnpm format:check, pnpm typecheck and the hygiene check are clean.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/comfy-typescript-sdk/pull/125 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/4/2026 **Status:** ✅ Merged **Merged:** 9/4/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/fix-credentials-test-unstub-order` --- ### 📝 Commits (1) - [`851c1f4`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/851c1f4784571108b7932e1533356263e8da3729) test(credentials): unstub globals before envs so the hook cannot throw ### 📊 Changes **2 files changed** (+6 additions, -2 deletions) <details> <summary>View changed files</summary> 📝 `src/sdk/credentials.test.ts` (+5 -1) 📝 `src/sdk/models.test.ts` (+1 -1) </details> ### 📄 Description Two-line fix. `pnpm test` is red on Node 26 — 17 failures — and green on the Node versions CI runs. ## ELI-5 Two tests in `credentials.test.ts` delete `process` to prove the SDK degrades gracefully in a runtime that has no such thing (a browser). The cleanup hook that runs after every test then tries to put the environment variables back, and it reaches them through `globalThis.process.env` — which is exactly the thing the test just deleted. So the cleanup crashes partway through, before it gets to the line that would have restored `process`. `process` stays gone for the rest of the file, and every test after those two fails on the same missing global. Two real assertions become 17 red tests. The fix is to restore the globals first, then the environment variables, so there is a `process` to restore them into. ## Why CI didn't catch it It depends on how the runtime hands `process` to vitest's env restore. | Node | `pnpm test` | | ---- | ----------- | | 22 | green (CI) | | 24 | green (CI) | | 26 | red, 17 failures | `engines.node` is `>= 22` and CI matrixes 22 and 24, so nothing on the required checks sees this. It shows up as soon as you run the suite on a current Node locally. ## Scope - `src/sdk/credentials.test.ts` — the live failure. Hook order swapped, with a comment saying why the order matters, so the next person does not swap it back. - `src/sdk/models.test.ts` — same hook shape, but it only stubs `fetch`, so the bug is latent rather than live there. Swapped too, since a future test that stubs a global the env restore depends on would hit the identical trap. No source file is touched. ## Verification `pnpm test` on Node 26: **432 passed, 0 failed** (was 415 passed / 17 failed). The two `process` tests still assert what they always did — they were passing on their own bodies all along; it was their cleanup that failed. `pnpm lint`, `pnpm format:check`, `pnpm typecheck` and the hygiene check are clean. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 22:04:49 +00:00