chore: run the basics server tests against Server.respond - #16872
chore: run the basics server tests against Server.respond#16872Nic-Polumeyv wants to merge 3 commits into
Server.respond#16872Conversation
|
Install the latest version of pnpm add https://pkg.svelte.dev/@sveltejs/kit/c/f03dc93598c42b55ecb312e95a62838115842985Open in |
|
|
Noodling on some tweaks in #16879. One thing neither PR covers AFAICT: the existing setup tests the dev server as well as the prod build, but this only tests the latter IIUC? |
|
Dev is back on 535985a, a second vitest project runs the same spec through Vite in middleware mode. 79 build / 67 dev, 12 skipped where Playwright skipped them too. |
2c0208d to
cc5e1b8
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: QUIET Plan: Advanced Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe basics application now runs a dedicated server-side Vitest suite in development and build modes. The suite covers server requests, errors, routing, cookies, rerouting, tracing, asset preloading, workers, and streaming. Shared helpers read error and trace records. The existing server test file retains selected coverage, and the client Vitest pattern excludes server tests. The Node response mock now tests invalid header handling. Merge Risk: 🟡 Moderate · up to The server-test migration may fail before executing because its configured environment is not declared, and cleanup can mask that failure. Resolve these test-run reliability issues before merging. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/kit/test/apps/basics/package.json (1)
11-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the build-mode server test self-contained.
When
vitest.server.config.jsruns withoutKIT_TEST_DEV,server.spec.jsimports.svelte-kit/output/server/manifest.jsandindex.js. A standalone run can therefore fail because the Vitest configuration has no setup that creates the build. Add a dedicated server-test script that performs the test setup andpnpm buildbefore Vitest. The Playwright&&chain is normal fail-fast orchestration and does not require a separate correction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: QUIET
Plan: Advanced
Run ID: c4e44b90-55b7-4e74-9920-948fba1f971d
📒 Files selected for processing (11)
.changeset/quiet-heads-rest.mdpackages/kit/src/runtime/server/index.jspackages/kit/test/apps/basics/package.jsonpackages/kit/test/apps/basics/src/routes/endpoint-output/fetch-asset/absolute/+server.jspackages/kit/test/apps/basics/src/routes/endpoint-output/head-write-error/+server.jspackages/kit/test/apps/basics/test/server.test.jspackages/kit/test/apps/basics/unit-test/server.spec.jspackages/kit/test/apps/basics/vite.config.jspackages/kit/test/apps/basics/vitest.server.config.jspackages/kit/test/records.jspackages/kit/test/utils.js
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
sveltejs/vite-plugin-svelte(manual)vitejs/vite(manual)sveltejs/svelte(manual)
💤 Files with no reviewable changes (3)
- packages/kit/test/apps/basics/src/routes/endpoint-output/fetch-asset/absolute/+server.js
- packages/kit/test/apps/basics/src/routes/endpoint-output/head-write-error/+server.js
- packages/kit/test/apps/basics/test/server.test.js
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
| test: { | ||
| name: 'kit-basics-server', | ||
| // for DOMParser | ||
| environment: 'jsdom', |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Description: Check jsdom declarations, catalog entries and hoisting settings.
set -uo pipefail
echo '--- jsdom in any package.json ---'
fd -H -t f 'package.json' -E node_modules --exec rg -n '"jsdom"' {} \; -X true 2>/dev/null
fd -H -t f 'package.json' -E node_modules | while IFS= read -r f; do
rg -n '"jsdom"' "$f" | sed "s|^|$f: |"
done
echo '--- catalog entries ---'
fd -H -t f 'pnpm-workspace.yaml' -E node_modules | while IFS= read -r f; do
rg -n -A3 'jsdom|catalog' "$f" | head -50
done
echo '--- hoisting configuration ---'
fd -H -t f '.npmrc' -E node_modules | while IFS= read -r f; do
echo "== $f"; cat "$f"
done
echo '--- other vitest configs that use jsdom ---'
rg -n --glob '!node_modules' "environment: '(jsdom|happy-dom)'"Repository: sveltejs/kit
Length of output: 1265
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- test app package and Vitest config ---'
cat -n packages/kit/test/apps/basics/package.json
cat -n packages/kit/test/apps/basics/vitest.server.config.js
printf '%s\n' '--- DOMParser usage ---'
sed -n '85,115p' packages/kit/test/apps/basics/src/server.spec.js 2>/dev/null || \
sed -n '85,115p' packages/kit/test/apps/basics/server.spec.js 2>/dev/null || true
printf '%s\n' '--- workspace and package-manager settings ---'
cat -n pnpm-workspace.yaml | sed -n '1,115p'
find . -maxdepth 3 -name '.npmrc' -o -name 'pnpm-workspace.yaml' | sortRepository: sveltejs/kit
Length of output: 6570
🤖 get_repo_knowledge executed:
get_repo_knowledge sveltejs/kit /tmp/coderabbit-repo-knowledge/sveltejs-kit-ab1bcef7
Length of output: 546
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- server test and lockfile importers ---'
cat -n packages/kit/test/apps/basics/unit-test/server.spec.js | sed -n '95,112p'
rg -n -A35 -B5 'test/apps/basics|test-basics|jsdom' pnpm-lock.yaml packages/kit/package.json packages/kit/test/apps/basics/package.json
printf '%s\n' '--- all workspace packages that declare jsdom ---'
rg -n -g 'package.json' '"jsdom"\s*:' .Repository: sveltejs/kit
Length of output: 50368
🏁 Script executed (no clone):
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Vitest package contract ---'
curl -fsSL https://registry.npmjs.org/vitest/4.1.10 | python3 -c '
import json, sys
p = json.load(sys.stdin)["versions"]["4.1.10"]
print(json.dumps({
"version": p.get("version"),
"dependencies": p.get("dependencies", {}),
"peerDependencies": p.get("peerDependencies", {}),
"peerDependenciesMeta": p.get("peerDependenciesMeta", {})
}, indent=2))
'
Length of output: 252
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- exact basics importer ---'
sed -n '859,901p' pnpm-lock.yaml
printf '%s\n' '--- exact Vitest package entries ---'
rg -n -A18 -B3 '^ vitest@4\.1\.10|^ vitest:' pnpm-lock.yaml | head -120Repository: sveltejs/kit
Length of output: 3856
🌐 Web query:
Vitest 4.1 jsdom environment install jsdom package peer dependency documentation
💡 Result:
In Vitest 4.1, the jsdom environment is not included by default and must be installed as a separate development dependency [1][2][3]. To use the jsdom environment, you must install the jsdom package: npm install -D jsdom Once installed, you can configure Vitest to use it by setting the environment option in your configuration file [1][2]: // vitest.config.ts import { defineConfig } from 'vitest/config' export default defineConfig({ test: { environment: 'jsdom', }, }) If you are using TypeScript and want proper type support for the jsdom global, you should also add the following to your tsconfig.json [4]: { "compilerOptions": { "types": ["vitest/jsdom"] } } Vitest does not manage the jsdom package as a direct peer dependency that is automatically installed, though it does include logic to check for its presence and may prompt you to install it if it is missing when you first attempt to use the jsdom environment [5][3]. The jsdom package is used dynamically by Vitest to emulate the browser environment [1][6].
Citations:
- 1: https://vitest.dev/guide/environment
- 2: https://vitest.dev/guide/features
- 3: GitHub discussion 1077 in vitest-dev/vitest (link omitted to avoid creating a cross-reference)
- 4: https://vitest.dev/config/environment
- 5: https://vitest.dev/config/
- 6: https://github.com/vitest-dev/vitest/blob/main/packages/vitest/src/integrations/env/jsdom.ts
Declare jsdom in the test-basics devDependencies.
Vitest does not include jsdom by default. This configuration selects the jsdom environment, and unit-test/server.spec.js calls new DOMParser(). Add "jsdom": "catalog:" to packages/kit/test/apps/basics/package.json; otherwise the suite can fail during environment setup.
cc5e1b8 to
7b36829
Compare
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
packages/kit/test/apps/basics/unit-test/server.spec.js-66-69 (1)
66-69: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winGuard
listenerinafterAllso setup failures are not masked.
beforeAllcan throw before line 38 or line 56, for example when the dynamic import of the built output at line 43 or line 44 fails.listeneris thenundefined, and line 68 throwsTypeError: Cannot read properties of undefined. That second error hides the real setup failure.🐛 Proposed fix
afterAll(async () => { await vite?.close(); - await new Promise((resolve) => listener.close(resolve)); + if (listener) await new Promise((resolve) => listener.close(resolve)); });
🧹 Nitpick comments (2)
packages/kit/test/apps/basics/unit-test/server.spec.js (2)
97-102: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCap the redirect loop in
load.The loop follows redirects without a limit. If a fixture ever returns a redirect cycle, the test hangs until the Vitest timeout and reports no useful cause. Add a small cap and fail with an explicit message.
♻️ Proposed refactor
+ let redirects = 0; while (response.status >= 300 && response.status < 400) { + if (++redirects > 10) throw new Error(`too many redirects from ${pathname}`); const location = /** `@type` {string} */ (response.headers.get('location')); const redirected = new URL(location, origin + pathname); response = await get(redirected.pathname + redirected.search, init); }
838-848: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReject the promise when the raw
http.getrequest fails.The promise settles only in the
endhandler. If the request emitserror, or the response never ends, the promise never settles. The test then hangs until the Vitest timeout without the underlying error. Attach anerrorhandler that rejects.♻️ Proposed refactor
- const html = await new Promise((fulfil) => { - http.get( + const html = await new Promise((fulfil, reject) => { + const req = http.get( `${origin}/load/fetch-request-headers`, { headers: { accept: '*/*', 'user-agent': 'node' } }, (res) => { let body = ''; res.on('data', (chunk) => (body += chunk)); res.on('end', () => fulfil(body)); + res.on('error', reject); } ); + req.on('error', reject); });
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: QUIET
Plan: Advanced
Run ID: 3aa70f18-e3b9-4fc9-8840-e5dae98a2a3d
📒 Files selected for processing (1)
packages/kit/test/apps/basics/unit-test/server.spec.js
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
sveltejs/vite-plugin-svelte(manual)vitejs/vite(manual)sveltejs/svelte(manual)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
Co-authored-by: Rich Harris <rich.harris@vercel.com>
7b36829 to
cb12ee2
Compare
2 participants
The server tests in
basicsnever needed a browser: they assert on status, headers and body. Running them through Playwright with JavaScript off meant a preview server and a browser per project, repeated in every no-js leg of the matrix, and a 1 to 2 minute loop to run one of them locally. The file said so itself (server.test.js:33, "this would ideally be a unit test ofServer").They now run under vitest from
unit-test/server.spec.js, calling the built app'sServer.responddirectly with no HTTP in between. WithKIT_TEST_DEVset the same file goes through Vite's dev server in middleware mode instead, so both modes keep their coverage. The suite runs from the app's owntest:devandtest:buildafter Playwright, against the build Playwright already made, so nothing builds twice and the unit job is untouched. 13 tests stay in Playwright because they assert whatvite previewand the dev server do aroundServer(static assets, prerendered output).Stacked on #17036: three of the moved
HEADtests need it in build mode.