test: use fixtures for server side api fetches #828
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
📝 WalkthroughWalkthroughThis pull request adds an end-to-end test fixtures system: a Nuxt module ( 🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
| // Main | ||
| // ============================================================================ | ||
|
|
||
| async function main(): Promise<void> { |
There was a problem hiding this comment.
If I understand correctly this either (re)-generates everything, or strictly the ones you ask for (w. args)? Re-generating could lead to annoyance since you'd have to update test expectations, and re-generate screenshots too (I guess there's none of that currently, but still). Thinking out aloud, a small CLI might be neat, letting you select from the pre-defined options in REQUIRED_PACKAGES and what you want to generate and what not.
There was a problem hiding this comment.
I imagine we will only be adding new snapshots rather than updating existing ones.... but happy for a PR to improve this 🙏
| import { expect, test } from '@nuxt/test-utils/playwright' | ||
|
|
||
| test.describe('Create Command', () => { | ||
| // TODO: these tests depend on external npm registry API - we should add data fixtures |
![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
🧹 Nitpick comments (4)
CONTRIBUTING.md (2)
506-515: Consider adding language identifier to fenced code blocks.Static analysis flagged this directory tree structure as missing a language specification. Adding
textor an empty identifier would resolve the markdownlint warning.📝 Suggested fix
-``` +```text test/fixtures/ ├── npm-registry/
538-543: Consider adding language identifier to this code block.Similar to the above, this error message example is flagged by markdownlint for missing a language specification.
📝 Suggested fix
-``` +```text UNMOCKED EXTERNAL API REQUEST DETECTEDtest/fixtures/npm-registry/packuments/@vitejs/plugin-vue.json (1)
1359-1397: Consider trimming large/PII‑heavy fixture fieldsThese fixtures embed full README text and maintainer emails, which inflates the repo and retains personal data. If tests don’t assert on these fields, consider stripping them (e.g.,
readme/readmeFilename,_npmUser.email,maintainers[].email,dist.signatures/attestations) via the fixture generator to keep data lean.modules/runtime/server/cache.ts (1)
142-149: Avoid real‑time timestamps in fixture responses.Using current time for
publishedAt/lastSyncedcan introduce non‑determinism in snapshot assertions. Consider a fixed fallback or derive solely from fixture data.Possible adjustment
- return { - data: { - name: packageName, - specifier, - version, - publishedAt: packument.time?.[version] || new Date().toISOString(), - lastSynced: Date.now(), - }, - } + const publishedAt = packument.time?.[version] ?? '1970-01-01T00:00:00.000Z' + return { + data: { + name: packageName, + specifier, + version, + publishedAt, + lastSynced: 0, + }, + }
| function packageToFixturePath(packageName: string): string { | ||
| if (packageName.startsWith('@')) { | ||
| const [scope, name] = packageName.slice(1).split('/') | ||
| return `npm-registry/packuments/@${scope}/${name}.json` | ||
| } | ||
| return `npm-registry/packuments/${packageName}.json` | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat test/e2e/test-utils.tsRepository: npmx-dev/npmx.dev
Length of output: 8218
Guard scoped package parsing to prevent undefined paths.
The function can receive scoped package names like @scope without a second segment when the preprocessing logic leaves it unchanged (in handleNpmRegistry when parts.length <= 2, or in handleFastNpmMeta when there is no version specifier). This causes name to become undefined, producing a path like npm-registry/packuments/@scope/undefined.json. Add validation to fail fast with a clear error message.
Suggested fix
if (packageName.startsWith('@')) {
const [scope, name] = packageName.slice(1).split('/')
+ if (!scope || !name) {
+ throw new Error(`Invalid scoped package name: ${packageName}`)
+ }
return `npm-registry/packuments/@${scope}/${name}.json`
}
scripts/generate-fixtures.ts
Outdated
| 'nuxt', | ||
| 'vue', | ||
| 'lodash', | ||
| 'vite', | ||
| 'next', | ||
| 'color', | ||
| 'ufo', | ||
| 'is-odd', | ||
| 'date-fns', | ||
| 'lodash.merge', |
There was a problem hiding this comment.
That list is a bit long, is there something unique about each and every item on this list?
There was a problem hiding this comment.
no, we could probably cut it down. but does it hurt?
There was a problem hiding this comment.
Not really, but why have more code if we could have less. In particular, date-fns fixture is enormous. Just parsing JSON that is nearing 100k lines may become a performance bottleneck.
There was a problem hiding this comment.
will see how far I can cut it down
There was a problem hiding this comment.
updated - see what you think! ✅
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/fixtures/npm-registry/packuments/vite.json (1)
1-12: Consider trimming the packument fixture to fields used in tests.
This snapshot is very large; pruning unused metadata keeps fixtures lighter and reduces update churn.modules/runtime/server/cache.ts (1)
90-153: Prefer deterministic timestamps in fast‑npm‑meta mocks.
publishedAtandlastSyncedfall back to current time, which can introduce non‑determinism if tests assert on these fields. Consider deriving from fixture metadata.♻️ Suggested change
- return { - data: { - name: packageName, - specifier, - version, - publishedAt: packument.time?.[version] || new Date().toISOString(), - lastSynced: Date.now(), - }, - } + const publishedAt = + packument.time?.[version] ?? + packument.time?.modified ?? + '1970-01-01T00:00:00.000Z' + + return { + data: { + name: packageName, + specifier, + version, + publishedAt, + lastSynced: Date.parse(publishedAt), + }, + }
wojtekmaj
left a comment
wojtekmaj
left a comment
There was a problem hiding this comment.
1/4 the size of the original fixtures, isn't that nice?! Well done!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
modules/runtime/server/cache.ts (2)
228-234: Consider using deterministic timestamps for fixture responses.Lines 232-234 use
new Date().toISOString()andDate.now()which introduce non-determinism. Whilst this may not affect most tests, it could cause issues in snapshot testing or when comparing cached values.Suggested fix
const result: Record<string, unknown> = { name: packageName, specifier, version, - publishedAt: packument.time?.[version] || new Date().toISOString(), - lastSynced: Date.now(), + publishedAt: packument.time?.[version] ?? '1970-01-01T00:00:00.000Z', + lastSynced: 0, }
411-582: Consider breaking downfetchFromFixturesinto smaller helper functions.At ~170 lines, this function handles many concerns: mock responses, fast-npm-meta, GitHub API, fixture matching, stub generation, and version resolution. Extracting helpers for each fixture type (e.g.,
handlePackumentFixture,handleOrgFixture) would improve readability and testability.test/e2e/test-utils.ts (1)
325-365: Consider loading GitHub contributors from the fixture file.The hardcoded contributor data here (3 contributors) differs from
test/fixtures/github/contributors.json(5 contributors). This could cause inconsistencies between client-side Playwright mocks and server-side fixture responses. Loading from the fixture would ensure consistency.Suggested fix
async function handleGitHubApi(route: Route): Promise<boolean> { const url = new URL(route.request().url()) const pathname = url.pathname // Contributors endpoint: /repos/{owner}/{repo}/contributors const contributorsMatch = pathname.match(/^\/repos\/([^/]+)\/([^/]+)\/contributors$/) if (contributorsMatch) { + const fixture = readFixture('github/contributors.json') await route.fulfill({ - json: [ - { - login: 'danielroe', - id: 28706372, - avatar_url: 'https://avatars.githubusercontent.com/u/28706372?v=4', - html_url: 'https://github.com/danielroe', - contributions: 150, - }, - { - login: 'antfu', - id: 11247099, - avatar_url: 'https://avatars.githubusercontent.com/u/11247099?v=4', - html_url: 'https://github.com/antfu', - contributions: 120, - }, - { - login: 'pi0', - id: 5158436, - avatar_url: 'https://avatars.githubusercontent.com/u/5158436?v=4', - html_url: 'https://github.com/pi0', - contributions: 100, - }, - ], + json: fixture || [], }) return true } return false }
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
modules/runtime/server/cache.ts (2)
141-162: Consider using deterministic timestamps in mock responses.Line 147 uses
new Date().toISOString()which produces non-deterministic output. For fully reproducible CI tests, consider using a fixed timestamp.Suggested change
return { data: { - analyzedAt: new Date().toISOString(), + analyzedAt: '2025-01-01T00:00:00.000Z', collected: {
519-554: Stub packument uses non-deterministic timestamps.Lines 533-535 use
new Date().toISOString()for the stub packument'stimefields. For fully deterministic CI tests, consider using fixed timestamps.Suggested change
+ const stubTimestamp = '2025-01-01T00:00:00.000Z' const stubPackument = { 'name': match.name, 'dist-tags': { latest: stubVersion }, 'versions': { [stubVersion]: { name: match.name, version: stubVersion, description: `Stub fixture for ${match.name}`, dependencies: {}, }, }, 'time': { - created: new Date().toISOString(), - modified: new Date().toISOString(), - [stubVersion]: new Date().toISOString(), + created: stubTimestamp, + modified: stubTimestamp, + [stubVersion]: stubTimestamp, }, 'maintainers': [], }
| // Bundlephobia API - return mock size data | ||
| if (host === 'bundlephobia.com' && pathname === '/api/size') { | ||
| const packageSpec = searchParams.get('package') | ||
| if (packageSpec) { | ||
| return { | ||
| data: { | ||
| name: packageSpec.split('@')[0], | ||
| size: 12345, | ||
| gzip: 4567, | ||
| dependencyCount: 3, | ||
| }, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Scoped package name parsing bug in bundlephobia mock.
For scoped packages like @scope/[email protected], split('@')[0] returns an empty string because the result is ['', 'scope/name', '1.0.0']. Use the existing parseScopedPackageWithVersion helper for consistency.
Proposed fix
// Bundlephobia API - return mock size data
if (host === 'bundlephobia.com' && pathname === '/api/size') {
const packageSpec = searchParams.get('package')
if (packageSpec) {
+ const parsed = parseScopedPackageWithVersion(packageSpec)
return {
data: {
- name: packageSpec.split('@')[0],
+ name: parsed.name,
size: 12345,
gzip: 4567,
dependencyCount: 3,
},
}
}
}
this is a first step to using api mocking on the server to make CI tests (lighthouse + playwright) faster and less flaky
the majority of the time is still building the app, but this should help
before
after
