test: mock client side requests in lighthouse#1171
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThis PR centralises external-API test mocks into a new CommonJS fixture Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
161-188:⚠️ Potential issue | 🟡 MinorAvoid running both colour modes twice in CI.
pnpm test:a11y:prebuiltalready runs dark and light, so the matrix duplicates work. Consider removing the matrix or switching this step to a single‑mode script.
lighthouse-setup.cjs
Outdated
| page.on('request', request => { | ||
| const requestUrl = request.url() | ||
| const result = mockRoutes.matchRoute(requestUrl) | ||
|
|
||
| if (result) { | ||
| request.respond({ | ||
| status: result.response.status, | ||
| contentType: result.response.contentType, | ||
| body: result.response.body, | ||
| }) | ||
| } else { | ||
| request.continue() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f lighthouse-setup.cjsRepository: npmx-dev/npmx.dev
Length of output: 81
🏁 Script executed:
git ls-files | grep -E "(lighthouse|mock)" | head -20Repository: npmx-dev/npmx.dev
Length of output: 156
🏁 Script executed:
rg -t js "mockRoutes\|getExternalApiName" --max-count=20Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
cat -n test/fixtures/mock-routes.cjsRepository: npmx-dev/npmx.dev
Length of output: 15791
🏁 Script executed:
wc -l lighthouse-setup.cjsRepository: npmx-dev/npmx.dev
Length of output: 84
🏁 Script executed:
cat -n lighthouse-setup.cjsRepository: npmx-dev/npmx.dev
Length of output: 3017
Block unmocked external API requests during audits to preserve determinism.
Currently, when a request URL matches a known external API domain (e.g., npm registry, GitHub API) but no fixture exists, request.continue() allows the real API call through. This can cause non-deterministic audit results. Using getExternalApiName() to detect these cases and respond with an error keeps audits isolated and reproducible.
Suggested implementation
page.on('request', request => {
const requestUrl = request.url()
const result = mockRoutes.matchRoute(requestUrl)
+ const apiName = mockRoutes.getExternalApiName(requestUrl)
if (result) {
request.respond({
status: result.response.status,
contentType: result.response.contentType,
body: result.response.body,
})
+ } else if (apiName) {
+ request.respond({
+ status: 500,
+ contentType: 'text/plain',
+ body: `Unmocked external request: ${apiName}`,
+ })
} else {
request.continue()
}
})Verify the response behaviour works as expected with your Puppeteer and Lighthouse versions.
| import { test as base } from '@nuxt/test-utils/playwright' | ||
| import { createRequire } from 'node:module' | ||
|
|
||
| return false | ||
| } | ||
| const require = createRequire(import.meta.url) | ||
| const mockRoutes = require('../fixtures/mock-routes.cjs') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "mock-routes" --fullRepository: npmx-dev/npmx.dev
Length of output: 272
🏁 Script executed:
find . -name "tsconfig*" -type fRepository: npmx-dev/npmx.dev
Length of output: 96
🏁 Script executed:
rg "mockRoutes" -t ts -t tsx --context 3Repository: npmx-dev/npmx.dev
Length of output: 88
🏁 Script executed:
fd "mock-routes" --full-pathRepository: npmx-dev/npmx.dev
Length of output: 90
🏁 Script executed:
rg "mockRoutes" -t ts --context 3Repository: npmx-dev/npmx.dev
Length of output: 1011
🏁 Script executed:
cat test/fixtures/mock-routes.cjsRepository: npmx-dev/npmx.dev
Length of output: 12452
🏁 Script executed:
head -20 tsconfig.jsonRepository: npmx-dev/npmx.dev
Length of output: 380
🏁 Script executed:
rg '"strict"' tsconfig.json cli/tsconfig.jsonRepository: npmx-dev/npmx.dev
Length of output: 98
🏁 Script executed:
grep -r "strict" tsconfig.json cli/tsconfig.json 2>/dev/null || echo "No explicit strict setting found"Repository: npmx-dev/npmx.dev
Length of output: 98
🏁 Script executed:
cat ./cli/tsconfig.jsonRepository: npmx-dev/npmx.dev
Length of output: 397
🏁 Script executed:
head -50 ./tsconfig.json | cat -nRepository: npmx-dev/npmx.dev
Length of output: 506
🏁 Script executed:
rg "test" tsconfig.jsonRepository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
fd "tsconfig" --full-path | xargs catRepository: npmx-dev/npmx.dev
Length of output: 717
Add explicit typings for mockRoutes to keep TypeScript strict.
The CJS require() returns any, so route and response shapes are not type-checked. Adding a local type assertion ensures strict compliance with TypeScript's type safety requirements.
🔧 Proposed fix
import type { Page, Route } from '@playwright/test'
import { test as base } from '@nuxt/test-utils/playwright'
import { createRequire } from 'node:module'
+type MockResponse = { status: number; contentType: string; body: string }
+type MockRoute = { name: string; pattern: string; match: (url: string) => MockResponse | null }
+type MockRoutesModule = {
+ routes: MockRoute[]
+ matchRoute: (url: string) => { name: string; response: MockResponse } | null
+}
+
const require = createRequire(import.meta.url)
-const mockRoutes = require('../fixtures/mock-routes.cjs')
+const mockRoutes = require('../fixtures/mock-routes.cjs') as MockRoutesModule| function matchBundlephobiaApi(urlString) { | ||
| const url = new URL(urlString) | ||
|
|
||
| if (url.pathname === '/api/size') { | ||
| const packageSpec = url.searchParams.get('package') | ||
| if (packageSpec) { | ||
| return json({ | ||
| name: packageSpec.split('@')[0], | ||
| size: 12345, | ||
| gzip: 4567, | ||
| dependencyCount: 3, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle scoped packages in the Bundlephobia mock.
packageSpec.split('@')[0] returns an empty string for scoped names (e.g. @scope/pkg@1.0.0), so the name field becomes blank. Consider reusing parseScopedPackage for consistent parsing.
🔧 Proposed fix
if (packageSpec) {
- return json({
- name: packageSpec.split('@')[0],
+ const parsed = parseScopedPackage(packageSpec)
+ return json({
+ name: parsed.name,
size: 12345,
gzip: 4567,
dependencyCount: 3,
})
}
| const cdpPatterns = mockRoutes.routes.map(route => ({ | ||
| urlPattern: route.pattern.replace('/**', '/*'), | ||
| requestStage: 'Request', | ||
| })) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "mock-routes*" -o -name "lighthouse-setup*" | head -20Repository: npmx-dev/npmx.dev
Length of output: 115
🏁 Script executed:
git ls-files | grep -E "(mock-routes|lighthouse-setup)" | head -20Repository: npmx-dev/npmx.dev
Length of output: 111
🏁 Script executed:
git ls-files | grep -E "\.cjs$" | head -20Repository: npmx-dev/npmx.dev
Length of output: 129
🏁 Script executed:
cat -n lighthouse-setup.cjs | sed -n '40,65p'Repository: npmx-dev/npmx.dev
Length of output: 1238
🏁 Script executed:
cat -n test/fixtures/mock-routes.cjsRepository: npmx-dev/npmx.dev
Length of output: 15791
🏁 Script executed:
grep -r "downloads/point" test/ --include="*.ts" --include="*.js" --include="*.cjs"Repository: npmx-dev/npmx.dev
Length of output: 471
🏁 Script executed:
grep -r "v1/packages" test/ --include="*.ts" --include="*.js" --include="*.cjs"Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
grep -r "lighthouse-setup" test/ .github/ --include="*.ts" --include="*.js" --include="*.cjs" --include="*.json" --include="*.yaml" --include="*.yml"Repository: npmx-dev/npmx.dev
Length of output: 148
🏁 Script executed:
cat -n test/nuxt/composables/use-npm-registry.spec.ts | sed -n '1,50p'Repository: npmx-dev/npmx.dev
Length of output: 1954
🏁 Script executed:
rg "https://api.npmjs.org/downloads" test/ -A 2 -B 2Repository: npmx-dev/npmx.dev
Length of output: 1423
🏁 Script executed:
cat -n lighthouse-setup.cjs | sed -n '1,100p'Repository: npmx-dev/npmx.dev
Length of output: 3971
🏁 Script executed:
rg "Fetch\.enable" lighthouse-setup.cjs -A 5 -B 5Repository: npmx-dev/npmx.dev
Length of output: 734
The pattern conversion from /** to /* will break CDP interception for multi-segment paths.
CDP Fetch patterns use glob syntax where * matches a single path segment and /** matches recursively. Converting patterns like https://api.npmjs.org/** to https://api.npmjs.org/* prevents CDP from pausing requests to endpoints with multiple path segments (e.g., https://api.npmjs.org/downloads/point/last-week/vue). Requests that don't match CDP's patterns won't pause, so mockRoutes.matchRoute() is never called and the mock response is never served.
Keep the patterns unchanged or use /** syntax if that's supported by CDP's urlPattern implementation.
| cdp.on('Fetch.requestPaused', async event => { | ||
| const requestUrl = event.request.url | ||
| const result = mockRoutes.matchRoute(requestUrl) | ||
|
|
||
| if (result) { | ||
| const body = Buffer.from(result.response.body).toString('base64') | ||
| await cdp.send('Fetch.fulfillRequest', { | ||
| requestId: event.requestId, | ||
| responseCode: result.response.status, | ||
| responseHeaders: [ | ||
| { name: 'Content-Type', value: result.response.contentType }, | ||
| { name: 'Access-Control-Allow-Origin', value: '*' }, | ||
| ], | ||
| body, | ||
| }) | ||
| } else { | ||
| await cdp.send('Fetch.continueRequest', { | ||
| requestId: event.requestId, | ||
| }) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Add error handling within the Fetch.requestPaused handler.
The cdp.send() calls inside the event handler are not wrapped in a try/catch. If the target closes whilst a request is paused (before fulfillRequest or continueRequest completes), this will throw an unhandled promise rejection.
🛡️ Proposed fix to add error handling
cdp.on('Fetch.requestPaused', async event => {
+ try {
const requestUrl = event.request.url
const result = mockRoutes.matchRoute(requestUrl)
if (result) {
const body = Buffer.from(result.response.body).toString('base64')
await cdp.send('Fetch.fulfillRequest', {
requestId: event.requestId,
responseCode: result.response.status,
responseHeaders: [
{ name: 'Content-Type', value: result.response.contentType },
{ name: 'Access-Control-Allow-Origin', value: '*' },
],
body,
})
} else {
await cdp.send('Fetch.continueRequest', {
requestId: event.requestId,
})
}
+ } catch {
+ // Target may have closed mid-request; safe to ignore.
+ }
})
1 participant
No description provided.