test: refactor suite to use common matrix utils #33483
Conversation
|
|
WalkthroughThis pull request introduces a centralised test matrix module ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes Justification: The refactoring spans many files (~15+) but follows a consistent, homogeneous pattern—removing local flag definitions, adding matrix imports, and updating config wrappers. However, the introduction of the new Pre-merge checks and finishing touches✅ Passed checks (3 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📓 Path-based instructions (2)**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{test,spec}.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧬 Code graph analysis (1)test/basic.test.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). (1)
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
@nuxt/kit
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
test/e2e/spa-preloader-body.test.ts (1)
27-34: Verify builder inheritance from matrix configuration.Similar to the previous test file, the
builderfield has been removed. Ensure that e2e tests correctly inherit the matrix-provided builder configuration.test/e2e/runtime-compiler.test.ts (1)
25-28: Verify builder configuration inheritance.The
builderfield has been removed fromnuxtConfig. Confirm that the test correctly inherits the matrix-provided builder configuration, especially given the dev/prod mode variations tested here.test/e2e/suspense.test.ts (1)
24-27: Verify builder inheritance for suspense tests.Similar to other e2e tests, the
builderfield has been removed. Ensure that the matrix-provided builder configuration is correctly applied.
🧹 Nitpick comments (2)
test/matrix.ts (2)
9-10: Consider documenting the vite-env-api special case.The builder selection logic maps
'vite-env-api'to'vite', which is non-obvious. A brief comment explaining why this mapping exists would improve maintainability.Apply this diff to add clarifying documentation:
+// vite-env-api uses the vite builder with experimental environment API enabled const _builder = process.env.TEST_BUILDER as 'webpack' | 'rspack' | 'vite' | 'vite-env-api' export const builder = _builder === 'vite-env-api' ? 'vite' : (_builder ?? 'vite')
4-4: Document TEST_BUILDER valid values
Valid values arewebpack,rspack,viteandvite-env-api.isWebpackcorrectly targets the first two andbuilderhandles the others. Add a JSDoc comment or update your docs to list all four values for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
test/basic.test.ts(27 hunks)test/e2e/hmr.test.ts(2 hunks)test/e2e/runtime-compiler.test.ts(1 hunks)test/e2e/spa-preloader-body.test.ts(1 hunks)test/e2e/spa-preloader-within.test.ts(1 hunks)test/e2e/suspense.test.ts(1 hunks)test/fixtures/basic-types/nuxt.config.ts(2 hunks)test/fixtures/basic/nuxt.config.ts(1 hunks)test/fixtures/hmr/nuxt.config.ts(1 hunks)test/fixtures/runtime-compiler/nuxt.config.ts(1 hunks)test/fixtures/spa-loader/nuxt.config.ts(1 hunks)test/fixtures/suspense/nuxt.config.ts(1 hunks)test/matrix.ts(1 hunks)test/nuxt/composables.test.ts(2 hunks)test/utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
test/e2e/hmr.test.tstest/e2e/runtime-compiler.test.tstest/e2e/spa-preloader-body.test.tstest/fixtures/suspense/nuxt.config.tstest/fixtures/runtime-compiler/nuxt.config.tstest/fixtures/basic-types/nuxt.config.tstest/matrix.tstest/nuxt/composables.test.tstest/fixtures/basic/nuxt.config.tstest/fixtures/hmr/nuxt.config.tstest/e2e/spa-preloader-within.test.tstest/fixtures/spa-loader/nuxt.config.tstest/basic.test.tstest/utils.tstest/e2e/suspense.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Write unit tests for core functionality using
vitest
Files:
test/e2e/hmr.test.tstest/e2e/runtime-compiler.test.tstest/e2e/spa-preloader-body.test.tstest/nuxt/composables.test.tstest/e2e/spa-preloader-within.test.tstest/basic.test.tstest/e2e/suspense.test.ts
**/e2e/**/*.{ts,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Write end-to-end tests using Playwright and
@nuxt/test-utils
Files:
test/e2e/hmr.test.tstest/e2e/runtime-compiler.test.tstest/e2e/spa-preloader-body.test.tstest/e2e/spa-preloader-within.test.tstest/e2e/suspense.test.ts
🧠 Learnings (1)
📚 Learning: 2025-07-18T16:46:07.446Z
Learnt from: CR
PR: nuxt/nuxt#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-18T16:46:07.446Z
Learning: Applies to **/e2e/**/*.{ts,js} : Write end-to-end tests using Playwright and `nuxt/test-utils`
Applied to files:
test/e2e/spa-preloader-body.test.tstest/e2e/spa-preloader-within.test.ts
🧬 Code graph analysis (10)
test/e2e/hmr.test.ts (1)
test/matrix.ts (1)
isBuilt(7-7)
test/fixtures/suspense/nuxt.config.ts (1)
test/matrix.ts (1)
withMatrix(19-33)
test/fixtures/runtime-compiler/nuxt.config.ts (1)
test/matrix.ts (1)
withMatrix(19-33)
test/fixtures/basic-types/nuxt.config.ts (1)
test/matrix.ts (1)
withMatrix(19-33)
test/matrix.ts (1)
test/basic.test.ts (1)
isDev(23-30)
test/nuxt/composables.test.ts (1)
test/matrix.ts (1)
isTestingAppManifest(12-12)
test/fixtures/basic/nuxt.config.ts (1)
test/matrix.ts (1)
withMatrix(19-33)
test/fixtures/hmr/nuxt.config.ts (1)
test/matrix.ts (1)
withMatrix(19-33)
test/fixtures/spa-loader/nuxt.config.ts (1)
test/matrix.ts (1)
withMatrix(19-33)
test/basic.test.ts (2)
packages/nuxt/src/app/components/nuxt-island.ts (1)
setup(81-353)test/matrix.ts (4)
isDev(6-6)isWebpack(4-4)isRenderingJson(17-17)asyncContext(14-14)
⏰ 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). (16)
- GitHub Check: test-fixtures (windows-latest, built, rspack, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, dev, vite, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, dev, vite, default, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, webpack, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, rspack, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, async, manifest-on, js, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, default, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, async, manifest-on, js, lts/-1)
- GitHub Check: release-pkg-pr-new
- GitHub Check: test-benchmark
- GitHub Check: test-size
- GitHub Check: typecheck (ubuntu-latest, bundler)
- GitHub Check: typecheck (windows-latest, bundler)
- GitHub Check: code
🔇 Additional comments (26)
test/matrix.ts (2)
1-2: LGTM!The imports are appropriate:
defufor deep merging configuration objects andNuxtConfigfor type safety.
19-33: LGTM! The withMatrix function correctly centralises test configuration.The function appropriately merges user-provided config with matrix-derived defaults using
defu. The defaults include:
- Builder selection
- TypeScript bundler resolution
- Experimental flags (asyncContext, appManifest, renderJsonPayloads, viteEnvironmentApi)
- Compatibility date
This centralisation eliminates duplication across test fixtures and ensures consistent test matrix behaviour.
test/e2e/spa-preloader-within.test.ts (2)
7-7: LGTM! The isDev import correctly replaces local environment detection.The import from
../matrixcentralises the environment flag, improving consistency across the test suite.
28-34: Verify builder inheritance from matrix configuration
Thebuilderfield was removed fromnuxtConfig. Confirm the e2e setup (e.g.withMatrixintest/e2e/test-utils.ts) correctly applies the matrix-provided builder. NobuilderorwithMatrixreferences were found—manual verification required.test/e2e/spa-preloader-body.test.ts (1)
7-7: LGTM! Consistent migration to centralised matrix flags.The import aligns with the test suite-wide refactoring to use matrix-provided environment flags.
test/e2e/hmr.test.ts (3)
7-7: LGTM! The imports correctly replace local environment checks.Using
isBuiltandisWebpackfrom the matrix module eliminates duplicate environment detection logic.
24-24: LGTM! The skip condition correctly uses the matrix flag.The change from
process.env.TEST_ENV === 'built'toisBuiltimproves readability whilst maintaining the same logic. Note thatisBuiltis defined as!isDevintest/matrix.ts.
18-21: Verify builder inheritance for this dev-mode test.The
builderfield has been removed fromnuxtConfigwhilstdev: trueis explicitly set. Confirm that the test correctly uses the matrix-provided builder in dev mode.test/utils.ts (1)
9-9: LGTM! The import centralises the isRenderingJson flag.Moving
isRenderingJsonfrom a local export to the matrix module reduces duplication and ensures consistency across the test suite.test/e2e/runtime-compiler.test.ts (1)
5-5: LGTM! Consistent adoption of matrix-based environment flags.The import aligns with the centralised matrix approach used throughout the test suite.
test/e2e/suspense.test.ts (1)
5-5: LGTM! Consistent migration to centralised environment flags.The import follows the established pattern across the test suite refactoring.
test/fixtures/spa-loader/nuxt.config.ts (1)
1-13: LGTM! Correct adoption of the withMatrix wrapper.The fixture correctly:
- Imports and uses
withMatrixinstead ofdefineNuxtConfig- Removes
compatibilityDate(now provided by matrix defaults)- Preserves fixture-specific config like
spaLoadingTemplateand route rules- The experimental
spaLoadingTemplateLocationsetting correctly overrides the matrix defaults viadefu's merge behaviourThis aligns the fixture with the centralised matrix-based configuration approach.
test/fixtures/suspense/nuxt.config.ts (1)
1-3: LGTM!The refactoring correctly replaces
defineNuxtConfigwithwithMatrixand delegates all configuration to the centralised matrix module. The empty configuration object indicates this fixture will use the matrix defaults for builder, experimental flags, and compatibility date.test/nuxt/composables.test.ts (2)
26-26: LGTM!The import correctly brings in the
isTestingAppManifestflag from the centralised matrix module, aligning with the PR's goal of standardising environment-driven test flags.
340-340: LGTM!The skip condition correctly uses the boolean
isTestingAppManifestflag instead of direct environment variable checking. The logic is semantically equivalent to the previousprocess.env.TEST_MANIFEST === 'manifest-off'check.test/fixtures/runtime-compiler/nuxt.config.ts (1)
1-11: LGTM!The refactoring correctly adopts
withMatrixwhilst preserving fixture-specific configuration (vue.runtimeCompilerandexperimental.externalVue). Thedefumerge inwithMatrixensures these overrides take precedence over matrix defaults.test/fixtures/hmr/nuxt.config.ts (1)
1-7: LGTM!The refactoring correctly transitions to the
withMatrixwrapper whilst retaining the fixture-specificexperimental.inlineRouteRulesconfiguration. Matrix defaults will provide the previously environment-driven settings.test/fixtures/basic/nuxt.config.ts (1)
6-8: LGTM!The refactoring correctly adopts
withMatrixfor the basic fixture. The extensive fixture-specific configuration is preserved, and matrix defaults (builder, experimental flags, compatibilityDate) will be merged appropriately viadefu.test/fixtures/basic-types/nuxt.config.ts (2)
4-4: LGTM!The import correctly brings in both
withMatrixandtypescriptBundlerResolutionfrom the centralised matrix module, ensuring consistent configuration across the test suite.
6-6: LGTM!The refactoring correctly adopts
withMatrixand uses the centralisedtypescriptBundlerResolutionvalue at line 88. This ensures consistency with the matrix module's TypeScript bundler resolution setting.Also applies to: 88-88
test/basic.test.ts (6)
7-7: LGTM!The additions to imports are appropriate:
createPageis used in test setup, andNuxtIslandResponseprovides proper typing for island response objects throughout the test suite.Also applies to: 10-10
12-13: LGTM!The imports correctly bring in matrix flags (
asyncContext,isDev,isRenderingJson,isTestingAppManifest,isWebpack) and test utilities, aligning with the centralised matrix approach for environment-driven test conditions.
17-17: LGTM!The transition from function calls to boolean flag usage (
isDev) is correct. The test setup at line 17 and the conditional at line 27 properly use the centralised boolean flag from the matrix module.Also applies to: 27-27
137-137: LGTM!All conditional expressions and test guards correctly use
isDevas a boolean flag rather than a function call, maintaining consistency with the matrix module approach throughout the test suite.Also applies to: 252-252, 624-624, 1153-1153, 1202-1202, 1452-1452
1820-1820: LGTM!The
describe.runIf,describe.skipIf, andit.skipIftest guards correctly use the booleanisDevandisWebpackflags from the matrix module for conditional test execution.Also applies to: 1829-1829, 2092-2092, 2122-2122, 2220-2220
2048-2053: LGTM!All remaining usages of
isDev,isWebpack,isWindows,isRenderingJson, andasyncContextcorrectly use the boolean flags from the matrix module. The refactoring maintains semantic equivalence whilst centralising environment flag management.Also applies to: 2056-2056, 2519-2519, 2550-2550, 2560-2560, 2625-2625, 2631-2631, 2698-2698, 3089-3089
CodSpeed Performance ReportMerging #33483 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/fixtures/basic/nuxt.config.ts (1)
36-37: Consider using exact equality for builder comparison.The current
includes()check works correctly for the known builder values ('vite', 'webpack', 'rspack', 'vite-env-api'), but using exact equality would be more precise and maintainable.Apply this diff for more precise matching:
- // Virtual CSS modules only work with Vite, not with webpack/rspack - if (typeof nuxt.options.builder === 'string' && (nuxt.options.builder.includes('webpack') || nuxt.options.builder.includes('rspack'))) { return } + // Virtual CSS modules only work with Vite, not with webpack/rspack + if (nuxt.options.builder === 'webpack' || nuxt.options.builder === 'rspack') { return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/fixtures/basic/nuxt.config.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
test/fixtures/basic/nuxt.config.ts
🧬 Code graph analysis (1)
test/fixtures/basic/nuxt.config.ts (1)
test/matrix.ts (1)
withMatrix(19-33)
⏰ 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). (3)
- GitHub Check: build
- GitHub Check: codeql (javascript-typescript)
- GitHub Check: code
🔇 Additional comments (1)
test/fixtures/basic/nuxt.config.ts (1)
6-8: LGTM! Correct use of matrix wrapper.The changes correctly refactor this fixture to use the centralised matrix configuration. The
withMatrixwrapper now provides builder selection, compatibility date, and experimental flags, eliminating the need for local environment-based configuration.
🔗 Linked issue
📚 Description
seems we weren't properly running all tests in matrix with the appropriate conditions.