test: add HMR tests with playwright #750
Conversation
WalkthroughAdds Playwright E2E HMR tests and fixtures plus a Vite-based playground; introduces Playwright CI workflow, Playwright config, and devDependency; broadens vitest discovery (excluding e2e/hmr); updates core to call server.updateRoutes after page changes; and adjusts chokidar awaitWriteFinish for CI. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-08-16T13:00:51.271ZApplied to files:
📚 Learning: 2025-08-23T12:50:36.614ZApplied to files:
🧬 Code graph analysis (1)src/core/RoutesFolderWatcher.spec.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
Comment |
commit: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #750 +/- ##
==========================================
+ Coverage 65.10% 70.61% +5.51%
==========================================
Files 33 33
Lines 2390 2396 +6
Branches 835 834 -1
==========================================
+ Hits 1556 1692 +136
+ Misses 692 580 -112
+ Partials 142 124 -18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Deploying unplugin-vue-router with
|
| Latest commit: |
f3508cd
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2780b909.unplugin-vue-router.pages.dev |
| Branch Preview URL: | https://test-e2e-hmr.unplugin-vue-router.pages.dev |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
playwright.config.ts (1)
23-38: Single browser configuration for speed.Only WebKit is enabled. This speeds up tests and is reasonable for HMR testing, which should behave consistently across browsers.
e2e/hmr/playground/vite.config.ts (1)
45-45: Potentially redundant glob patterns.Both
'src/loaders/**/*'and'@/loaders/**/*'resolve to the same path since@aliases to'./src'. If testing both patterns is not the intent, consider keeping only one.e2e/hmr/fixtures/vite-server.ts (1)
65-73: Consider validating file paths.No validation that
newContentFilePathexists before reading. Missing files will cause cryptic test failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
e2e/__snapshots__/routes.spec.ts.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
.github/workflows/playwright.yml(1 hunks).gitignore(1 hunks)e2e/hmr/.gitignore(1 hunks)e2e/hmr/fixtures/vite-server.ts(1 hunks)e2e/hmr/global.setup.ts(1 hunks)e2e/hmr/hmr.spec.ts(1 hunks)e2e/hmr/playground/edits/(home)-with-route-block.vue(1 hunks)e2e/hmr/playground/index.html(1 hunks)e2e/hmr/playground/package.json(1 hunks)e2e/hmr/playground/src/App.vue(1 hunks)e2e/hmr/playground/src/main.ts(1 hunks)e2e/hmr/playground/src/pages/(home).vue(1 hunks)e2e/hmr/playground/src/router.ts(1 hunks)e2e/hmr/playground/vite.config.ts(1 hunks)package.json(1 hunks)playground/src/pages/[name].vue(1 hunks)playground/src/pages/about.vue(1 hunks)playwright.config.ts(1 hunks)vitest.config.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-16T13:01:42.709Z
Learnt from: posva
Repo: posva/unplugin-vue-router PR: 700
File: src/codegen/generateParamParsers.ts:91-94
Timestamp: 2025-08-16T13:01:42.709Z
Learning: In the unplugin-vue-router codebase, path normalization for import specifiers is handled elsewhere in the system (not at individual call sites like generateParamParsers.ts). Individual functions should not normalize paths themselves as it violates separation of concerns.
Applied to files:
e2e/hmr/playground/vite.config.ts
🧬 Code graph analysis (3)
e2e/hmr/playground/src/router.ts (1)
client.d.ts (1)
routes(7-7)
e2e/hmr/playground/src/main.ts (1)
e2e/hmr/playground/src/router.ts (1)
router(4-7)
e2e/hmr/hmr.spec.ts (1)
e2e/hmr/fixtures/vite-server.ts (2)
test(21-80)expect(82-82)
🪛 GitHub Actions: Playwright Tests
e2e/hmr/hmr.spec.ts
[error] 20-23: Playwright test failed. Errors: 'use() was not called in fixture "applyEditFile"' and 'hmrToken was not set in the test' (see lines around 20 in hmr.spec.ts).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (17)
playground/src/pages/about.vue (2)
7-7: LGTM!Test data changes are clear and appropriate for HMR testing.
Also applies to: 12-12, 20-20
5-9: No issues found—dual meta definition is intentional.The
definePage()and<route>block define separate meta properties (nandnumber) that are intentionally merged per documented behavior inextendRoutes.ts. The template's{{ $route.meta }}display confirms this is demonstration code for the merging capability.e2e/hmr/playground/package.json (1)
1-7: LGTM!Minimal test fixture configuration is appropriate.
vitest.config.ts (1)
27-32: LGTM!Correctly separates Vitest and Playwright test execution paths.
playwright.config.ts (1)
6-15: LGTM!CI configuration follows best practices with appropriate retry and worker settings.
e2e/hmr/.gitignore (1)
1-1: LGTM!Appropriate ignore pattern for temporary test artifacts.
e2e/hmr/playground/src/App.vue (1)
1-3: LGTM!Minimal setup is appropriate for test fixture.
package.json (1)
185-185: LGTM!Playwright dependency addition is necessary for the new e2e test infrastructure.
e2e/hmr/playground/src/pages/(home).vue (1)
1-7: LGTM!Simple test fixture component correctly displays route metadata with proper test ID.
e2e/hmr/playground/index.html (1)
1-12: LGTM!Standard HTML5 entry point for the HMR test playground.
e2e/hmr/playground/src/main.ts (1)
1-14: LGTM!Standard Vue app bootstrap with useful navigation logging for HMR debugging.
.github/workflows/playwright.yml (1)
1-29: LGTM!Standard Playwright CI workflow with proper setup and artifact handling.
e2e/hmr/playground/edits/(home)-with-route-block.vue (1)
1-15: LGTM!Edit fixture correctly includes route block with metadata for HMR testing.
e2e/hmr/playground/src/router.ts (1)
1-13: LGTM!Router correctly implements HMR support with logging for debugging.
e2e/hmr/hmr.spec.ts (1)
4-25: HMR token mechanism validates no reload.The token pattern correctly ensures the page doesn't reload during HMR updates. However, the pipeline failure indicates
hmrTokenwasn't set, suggesting the test didn't reachensureHmrToken.e2e/hmr/playground/vite.config.ts (1)
1-37: LGTM!Configuration is clean and appropriate for test infrastructure.
e2e/hmr/fixtures/vite-server.ts (1)
9-23: LGTM!Type definitions and path setup are correct.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
e2e/hmr/fixtures/vite-server.ts (4)
5-7: Consolidate redundant imports.
cpSyncandrmSyncare imported separately when they're already available viafsfrom line 5.
33-33: Fix fixture type annotation.The
@ts-expect-errorsuppression indicates a type mismatch that should be resolved properly.
36-36: Remove debug statement.The
console.logshould be removed before merging.
74-78: Validateaddress()result before casting.
server.httpServer.address()can returnnullor astring(for pipes). The cast assumes a TCP socket without validation.
🧹 Nitpick comments (1)
e2e/hmr/fixtures/vite-server.ts (1)
15-24: Consider acceptingprojectRootas a parameter.The function is coupled to the module-level
projectRootconstant. Accepting it as a parameter would improve flexibility and testability.-export function applyEditFile( +export function applyEditFile( + projectRoot: string, sourceFilePath: string, newContentFilePath: string ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e/hmr/fixtures/vite-server.ts(1 hunks)e2e/hmr/hmr.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/hmr/hmr.spec.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-16T12:19:37.748Z
Learnt from: posva
Repo: posva/unplugin-vue-router PR: 700
File: src/utils/index.ts:1-1
Timestamp: 2025-08-16T12:19:37.748Z
Learning: The `constants` export from Node.js `fs/promises` module is version-dependent: it's available in Node.js 18.4.0 and above, but not in earlier versions. In older Node.js versions, `constants` must be imported from `node:fs` instead. When reviewing Node.js fs API usage, always consider the project's minimum Node.js version requirements.
Applied to files:
e2e/hmr/fixtures/vite-server.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Cloudflare Pages
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
e2e/hmr/hmr.spec.ts(1 hunks)src/core/RoutesFolderWatcher.ts(1 hunks)src/core/context.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/hmr/hmr.spec.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/core/context.ts (1)
src/core/RoutesFolderWatcher.ts (1)
HandlerContext(87-92)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
src/core/RoutesFolderWatcher.ts (1)
43-43: LGTM: CI-specific stabilization.The
awaitWriteFinishoption prevents race conditions in CI where file events may fire before writes complete. Note that this adds latency to file watching in CI environments.src/core/context.ts (2)
192-194: TODO comment suggests incomplete HMR work.The call to
updateRoutes()is appropriate, but the TODO indicates HMR triggering may need additional implementation. Verify if this is tracked separately.
215-216: TODO comment suggests incomplete HMR work.Similar to
addPage, the TODO indicates HMR triggering may need additional implementation. Ensure this is tracked.
| // no need to manually trigger the update of vue-router/auto-routes because | ||
| // the change of the vue file will trigger HMR | ||
| // server?.invalidate(filePath) | ||
| server?.updateRoutes() |
There was a problem hiding this comment.
Comment contradicts implementation.
Lines 206-207 state "no need to manually trigger" but line 209 calls updateRoutes(). The commented-out invalidate() on line 208 suggests uncertainty about the approach. Clarify if the comment is outdated or if updateRoutes() serves a different purpose than the comment implies (e.g., route metadata updates).
🤖 Prompt for AI Agents
In src/core/context.ts around lines 206 to 209 the comment says "no need to
manually trigger the update of vue-router/auto-routes because the change of the
vue file will trigger HMR" while the next line calls server?.updateRoutes() and
a server?.invalidate(filePath) call is commented out; reconcile this by updating
the comment to accurately describe why updateRoutes() is still required (for
example: it refreshes route metadata or triggers route registration beyond HMR)
or, if updateRoutes() is redundant, remove the call and use the proper
invalidate() call, and delete the stale commented-out code; ensure the retained
comment precisely documents the chosen approach and why it’s necessary.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
e2e/hmr/fixtures/vite-server.ts (3)
5-7: Remove redundant import.
cpSyncandrmSyncare already available viafs.Apply this diff:
-import fs from 'node:fs' +import fs, { cpSync, rmSync } from 'node:fs' import { fileURLToPath } from 'node:url' -import { cpSync, rmSync } from 'node:fs'
33-33: Remove type suppression.The
@ts-expect-errorindicates improper typing. Playwright fixtures should be correctly typed without suppression.
72-76: Validateaddress()result.
server.httpServer.address()can returnnullor a string (for pipe/socket). The cast toAddressInfoassumes a TCP socket without validation.Apply this diff:
baseURL: async ({ devServer }, use) => { const http = devServer.httpServer! const addr = http.address() as AddressInfo + if (!addr || typeof addr === 'string') { + throw new Error('Expected AddressInfo from server.address()') + } await use(`http://127.0.0.1:${addr.port}`) },
🧹 Nitpick comments (1)
e2e/hmr/fixtures/vite-server.ts (1)
15-24: Consider moving afterprojectRootdefinition.The function references
projectRootdefined at line 28. While this works because the function is only invoked after initialization, moving it after line 28 would improve readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
e2e/hmr/fixtures/vite-server.ts(1 hunks)e2e/hmr/hmr.spec.ts(1 hunks)src/core/RoutesFolderWatcher.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/hmr/hmr.spec.ts
- src/core/RoutesFolderWatcher.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-16T12:19:37.748Z
Learnt from: posva
Repo: posva/unplugin-vue-router PR: 700
File: src/utils/index.ts:1-1
Timestamp: 2025-08-16T12:19:37.748Z
Learning: The `constants` export from Node.js `fs/promises` module is version-dependent: it's available in Node.js 18.4.0 and above, but not in earlier versions. In older Node.js versions, `constants` must be imported from `node:fs` instead. When reviewing Node.js fs API usage, always consider the project's minimum Node.js version requirements.
Applied to files:
e2e/hmr/fixtures/vite-server.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (lts/*, windows-latest)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
e2e/hmr/fixtures/vite-server.ts (3)
9-13: LGTM!Type definition is clear and correctly structured.
26-28: LGTM!Path setup is correct and follows standard patterns.
35-68: LGTM!The fixture correctly sets up a clean playground, starts a Vite dev server, and properly cleans up resources.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
e2e/hmr/playground/vite.config.ts (1)
47-47: Redundant glob patterns.Both
'src/loaders/**/*'and'@/loaders/**/*'match the same files since@aliases to./src.Apply this diff to remove redundancy:
- autoExportsDataLoaders: ['src/loaders/**/*', '@/loaders/**/*'], + autoExportsDataLoaders: ['@/loaders/**/*'],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e/hmr/playground/vite.config.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-16T13:01:42.709Z
Learnt from: posva
Repo: posva/unplugin-vue-router PR: 700
File: src/codegen/generateParamParsers.ts:91-94
Timestamp: 2025-08-16T13:01:42.709Z
Learning: In the unplugin-vue-router codebase, path normalization for import specifiers is handled elsewhere in the system (not at individual call sites like generateParamParsers.ts). Individual functions should not normalize paths themselves as it violates separation of concerns.
Applied to files:
e2e/hmr/playground/vite.config.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (6)
e2e/hmr/playground/vite.config.ts (6)
1-4: LGTM!Imports are appropriate for the E2E HMR test configuration.
6-6: LGTM!Correct use of
fileURLToPathwithimport.meta.urlfor ESM path resolution.
8-10: LGTM!Configuration settings are appropriate for E2E testing.
11-34: LGTM!Alias configuration is consistent and appropriate for E2E testing with source files.
45-45: Verify if commented code is intentional.The
getRouteNameoption is commented out. Confirm if this is intentional for the HMR tests or should be removed.
39-52: LGTM!Plugin configuration is correct for HMR testing. The
watch: truesetting is necessary for HMR behavior despite the CI comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
e2e/hmr/fixtures/vite-server.ts (2)
32-32: Remove type suppression and fix fixture typing.The
@ts-expect-errorsuppresses a legitimate type error. Worker-scoped fixtures require separate type parameters inbase.extend<TestFixtures, WorkerFixtures>.Apply this diff to fix the typing:
+type WorkerFixtures = { + devServer: ViteDevServer +} + +type TestFixtures = { + baseURL: string + projectRoot: string +} + -export const test = base.extend<ViteFixtures>({ +export const test = base.extend<TestFixtures, WorkerFixtures>({ projectRoot, - // @ts-expect-error: type matched what is passed to use(server) devServer: [
71-75: Validateaddress()before casting.
server.httpServer.address()can returnnullor a string (for pipes/sockets). The cast toAddressInfoassumes a TCP socket without validation.Apply this diff:
baseURL: async ({ devServer }, use) => { const http = devServer.httpServer! const addr = http.address() as AddressInfo + if (!addr || typeof addr === 'string') { + throw new Error('Expected AddressInfo from server.address()') + } await use(`http://127.0.0.1:${addr.port}`) },
🧹 Nitpick comments (2)
src/core/RoutesFolderWatcher.ts (1)
45-45: LGTM - CI-gated awaitWriteFinish is appropriate for HMR test stability.This addresses file watching race conditions in CI environments. The default stabilityThreshold (2s) may add latency but is acceptable for CI test reliability.
Optionally, add a brief comment explaining the rationale:
+ // Wait for writes to finish in CI to avoid race conditions in HMR tests awaitWriteFinish: !!process.env.CI,Or configure with shorter thresholds if test speed matters:
- awaitWriteFinish: !!process.env.CI, + awaitWriteFinish: process.env.CI ? { stabilityThreshold: 500, pollInterval: 100 } : false,e2e/hmr/fixtures/vite-server.ts (1)
14-23: DecoupleapplyEditFilefrom module-levelprojectRoot.The function references the module-level
projectRootvariable, creating tight coupling. SinceprojectRootis exposed as a fixture, consider makingapplyEditFilea fixture method or acceptingprojectRootas a parameter.Apply this diff to accept
projectRootas a parameter:export function applyEditFile( + projectRoot: string, sourceFilePath: string, newContentFilePath: string ) { fs.writeFileSync( path.join(projectRoot, sourceFilePath), fs.readFileSync(path.join(projectRoot, newContentFilePath), 'utf8'), 'utf8' ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e/hmr/fixtures/vite-server.ts(1 hunks)src/core/RoutesFolderWatcher.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-16T12:19:37.748Z
Learnt from: posva
Repo: posva/unplugin-vue-router PR: 700
File: src/utils/index.ts:1-1
Timestamp: 2025-08-16T12:19:37.748Z
Learning: The `constants` export from Node.js `fs/promises` module is version-dependent: it's available in Node.js 18.4.0 and above, but not in earlier versions. In older Node.js versions, `constants` must be imported from `node:fs` instead. When reviewing Node.js fs API usage, always consider the project's minimum Node.js version requirements.
Applied to files:
e2e/hmr/fixtures/vite-server.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Cloudflare Pages
Summary by CodeRabbit