Conversation
|
✅ Deploy Preview for hpe-theme-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for unrivaled-bublanina-3a9bae ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for hpe-design-icons-grommet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR adds a Vitest-based unit testing setup for the apps/docs package (plus a pre-push hook) as groundwork for a future refactor of the docs “structure”/navigation system.
Changes:
- Add Vitest + config for
apps/docs, plustest:unitscripts and new dev dependencies. - Add initial unit tests for structure, navigation, and search utilities (currently using mock data).
- Add a Husky
pre-pushhook to run the new docs unit tests before pushing.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
pnpm-lock.yaml |
Locks new dev dependencies (Vitest, jsdom, plugin-react, UI) and updates some GitHub tarball URLs. |
apps/docs/vitest.config.mts |
Introduces Vitest config (React plugin, jsdom env, module alias). |
apps/docs/src/utils/__tests__/search.test.ts |
Adds tests for slug/search logic + documentation of hardcoded routes (implemented via local mock functions/data). |
apps/docs/src/layouts/navigation/__tests__/navItems.test.ts |
Adds navigation “structure” tests using locally defined mock nav items. |
apps/docs/src/data/__tests__/structure.validation.test.ts |
Adds structure validation tests using a local mockStructure. |
apps/docs/package.json |
Adds test:unit / test:unit:watch scripts and dev dependencies required for Vitest. |
apps/docs/RFC-STRUCTURE-REFACTORING.md |
Adds an RFC describing upcoming phases for refactoring structure/routing/category mapping. |
.husky/pre-push |
Runs pnpm --filter docs run test:unit on pre-push. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Mock structure data to avoid JSX parsing issues | ||
| const mockStructure = [ | ||
| { | ||
| name: 'Home', | ||
| seoDescription: 'Home page description', | ||
| pages: ['Foundation', 'Components', 'Templates', 'Learn', 'Design tokens'], | ||
| }, | ||
| { | ||
| name: 'Foundation', | ||
| category: 'Our brand', | ||
| seoDescription: 'Foundation section', | ||
| pages: ['Color', 'Typography', 'Spacing'], | ||
| }, | ||
| { | ||
| name: 'Components', | ||
| category: 'Components', | ||
| seoDescription: 'Components section', | ||
| pages: ['Button', 'Card', 'Text'], | ||
| }, | ||
| { | ||
| name: 'Templates', | ||
| category: 'Templates', | ||
| seoDescription: 'Templates section', | ||
| pages: ['Login', 'Dashboard'], | ||
| }, | ||
| { | ||
| name: 'Learn', | ||
| category: 'Learn', | ||
| seoDescription: 'Learning resources', | ||
| pages: ['Getting Started', 'Tutorials'], | ||
| }, | ||
| { | ||
| name: 'Design tokens', | ||
| category: 'Design tokens', | ||
| seoDescription: 'Design tokens section', | ||
| pages: ['Colors', 'Typography', 'Spacing'], | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
structure.validation.test.ts validates a hardcoded mockStructure rather than the real exported structure data, so it won't detect breakages/typos in the actual docs structure. If importing src/data/structure.js is problematic due to JSX/icon functions, consider mocking the icon imports (or mocking only the icon fields) so you can still validate the real structure entries and relationships.
| // Mock structure data to avoid JSX parsing issues | |
| const mockStructure = [ | |
| { | |
| name: 'Home', | |
| seoDescription: 'Home page description', | |
| pages: ['Foundation', 'Components', 'Templates', 'Learn', 'Design tokens'], | |
| }, | |
| { | |
| name: 'Foundation', | |
| category: 'Our brand', | |
| seoDescription: 'Foundation section', | |
| pages: ['Color', 'Typography', 'Spacing'], | |
| }, | |
| { | |
| name: 'Components', | |
| category: 'Components', | |
| seoDescription: 'Components section', | |
| pages: ['Button', 'Card', 'Text'], | |
| }, | |
| { | |
| name: 'Templates', | |
| category: 'Templates', | |
| seoDescription: 'Templates section', | |
| pages: ['Login', 'Dashboard'], | |
| }, | |
| { | |
| name: 'Learn', | |
| category: 'Learn', | |
| seoDescription: 'Learning resources', | |
| pages: ['Getting Started', 'Tutorials'], | |
| }, | |
| { | |
| name: 'Design tokens', | |
| category: 'Design tokens', | |
| seoDescription: 'Design tokens section', | |
| pages: ['Colors', 'Typography', 'Spacing'], | |
| }, | |
| ]; | |
| import { structure } from '../structure'; | |
| // Use the real structure data exported by the docs to ensure this test | |
| // catches any breakages or typos in the actual navigation structure. | |
| const mockStructure = structure; |
| it('should have valid parent/child relationships', () => { | ||
| const allNames = new Set(mockStructure.map(p => p.name)); | ||
|
|
||
| mockStructure.forEach(page => { | ||
| if (page.pages && page.pages.length > 0) { | ||
| // For mock data, we just verify the structure is properly formed | ||
| // not that every child exists (that's validated against real structure) | ||
| page.pages.forEach(childName => { | ||
| expect(typeof childName).toBe('string'); | ||
| expect(childName.length).toBeGreaterThan(0); | ||
| }); | ||
| } |
There was a problem hiding this comment.
In the "valid parent/child relationships" test, allNames is computed but never used, and the assertions only check that each childName is a non-empty string. If the goal is relationship integrity, this should assert that referenced children exist in the real structure (or remove the unused variable/comment to avoid implying stronger validation than is happening).
| alias: { | ||
| '@shared/aries-core': path.resolve( | ||
| __dirname, | ||
| '../../shared/aries-core/src', | ||
| ), |
There was a problem hiding this comment.
vitest.config.mts is an ESM config file, so __dirname will be undefined at runtime. This will throw when resolving the alias and prevent Vitest from starting. Use fileURLToPath(new URL(..., import.meta.url)) (or switch the config to a CJS/.ts config) to compute the path safely in ESM.
| // Create minimal test implementations without importing structure | ||
| // to avoid JSX parsing issues in vitest | ||
|
|
||
| // Test data | ||
| const mockPages = [ | ||
| { name: 'Home', pages: [] }, | ||
| { name: 'Components', pages: ['Button', 'Card'] }, | ||
| { name: 'Button', parentPage: 'Components' }, | ||
| { name: 'Card', parentPage: 'Components' }, | ||
| ]; | ||
|
|
||
| const nameToSlugImpl = (name: string) => { | ||
| return name | ||
| .toLowerCase() | ||
| .replace(/\s+/g, '-') | ||
| .replace(/[^\w-]+/g, '') | ||
| .replace(/_/g, '') // Remove underscores | ||
| .replace(/--+/g, '-') | ||
| .replace(/^-+/, '') | ||
| .replace(/-+$/, ''); | ||
| }; |
There was a problem hiding this comment.
These tests define local "minimal" implementations (e.g. nameToSlugImpl, findPageByName) instead of importing and exercising the real utilities in src/utils/search.js. As written, the tests can pass even if production behavior regresses. Consider importing the real functions and using vi.mock('../data', ...) to stub structure (or refactor the utilities to accept structure data as an injected dependency) so the tests validate actual app logic.
|
|
||
| console.warn( | ||
| `⚠️ Found ${hardcodedRoutes.length} hardcoded routes - identified as Phase 2 refactoring items`, | ||
| ); |
There was a problem hiding this comment.
This test logs via console.warn(...) unconditionally, which will add noise to local/CI test output even when everything passes. If you want to track hardcoded routes, prefer an assertion (or snapshot) and/or gate the warning behind a failing expectation or a vi.spyOn(console, 'warn') assertion.
| console.warn( | |
| `⚠️ Found ${hardcodedRoutes.length} hardcoded routes - identified as Phase 2 refactoring items`, | |
| ); |
| // Navigation structure tests without JSX parsing | ||
| // These tests validate the navigation is built correctly from structure data | ||
|
|
||
| const buildNavItem = (label: string, url?: string, children?: any[]) => ({ | ||
| label, | ||
| url, | ||
| children, | ||
| }); | ||
|
|
||
| describe('Navigation Items Structure', () => { | ||
| const mockNavItems = [ | ||
| buildNavItem('Home', '/'), | ||
| buildNavItem('Components', '/components', [ | ||
| buildNavItem('Button', '/components/button'), | ||
| buildNavItem('Card', '/components/card'), | ||
| ]), | ||
| buildNavItem('Templates', '/templates', [ | ||
| buildNavItem('Login', '/templates/login'), | ||
| ]), | ||
| ]; |
There was a problem hiding this comment.
navItems.test.ts currently only asserts against mockNavItems built by the test itself (and a local buildNavItem helper). This doesn't exercise any production navigation-building code, so it won't catch regressions in the real side-nav implementation. To make this test meaningful, import the actual nav builder / nav items module and, if necessary, mock the structure data it consumes.
Deploy Preview
Putting in place prior to: #5881
The current page structure data is quite fragile, difficult to follow, and requires a lot of manual maintenance. I'd like to refactor the structure data so that it is easier to work with. This will also make the side navigation definitions cleaner to implement.
What does this PR do?
This pull request introduces unit testing with Vitest to the
apps/docspackage, adds comprehensive structure and navigation tests, and integrates these tests into the pre-push workflow to improve code quality and maintainability. The changes include new test scripts, configuration, and mock-based tests to validate the docs system's data structures and utilities.Testing Infrastructure & Automation
.husky/pre-pushhook to automatically run unit tests for the docs system before pushing code, ensuring that structure-related tests are always checked.test:unitandtest:unit:watch) toapps/docs/package.jsonfor running Vitest-based unit tests, and added Vitest along with related dependencies (@vitejs/plugin-react,@vitest/ui,jsdom) todevDependencies. [1] [2]vitest.config.mtsconfiguration file to enable Vitest with React support and proper module aliasing, using the jsdom environment for browser-like testing.Unit Tests for Data Structures and Utilities
structure.validation.test.tsto validate the structure data's schema, data integrity, navigation consistency, and to check for circular references, using mock data to avoid JSX parsing issues.navItems.test.tsto test the navigation items' structure, hierarchy, hub page children, data consistency, and navigation building logic, again using mock data for reliability and speed.search.test.tsto test utility functions such asnameToSlug, page lookup, navigation paths, data consistency, and to document hardcoded routes that are candidates for future refactoring.What are the relevant issues?
Where should the reviewer start?
How should this be manually tested?
Any background context you want to provide?
Screenshots (if appropriate)
Should this PR be mentioned in Design System updates?
Is this change backwards compatible or is it a breaking change?