feat(test): e2e #162
feat(test): e2e #162
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces a comprehensive end-to-end (E2E) testing setup for a VitePress OpenAPI documentation project. It establishes a GitHub Actions workflow for automated Playwright tests, configures a development environment for documentation, and sets up test configurations. The changes include creating a new documentation structure, adding test specifications, and implementing a continuous integration pipeline to ensure the reliability and functionality of the project. Changes
Sequence DiagramsequenceDiagram
participant GHA as GitHub Actions
participant Playwright as Playwright Tests
participant VitePress as VitePress Docs
GHA->>Playwright: Trigger tests
Playwright->>VitePress: Start dev server
Playwright->>VitePress: Navigate to pages
Playwright->>Playwright: Run assertions
Playwright-->>GHA: Test results
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (5)
e2e/home.spec.ts-snapshots/home-1-chromium-linux.pngis excluded by!**/*.pnge2e/home.spec.ts-snapshots/home-1-firefox-linux.pngis excluded by!**/*.pnge2e/one-page.spec.ts-snapshots/one-page-1-chromium-linux.pngis excluded by!**/*.pnge2e/one-page.spec.ts-snapshots/one-page-1-firefox-linux.pngis excluded by!**/*.pngpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
.github/workflows/e2e.yml(1 hunks).gitignore(1 hunks)docs/package.json(1 hunks)e2e/dev/.gitignore(1 hunks)e2e/dev/docs/.vitepress/config.ts(1 hunks)e2e/dev/docs/.vitepress/theme/index.ts(1 hunks)e2e/dev/docs/index.md(1 hunks)e2e/dev/docs/introduction.md(1 hunks)e2e/dev/docs/one-page.md(1 hunks)e2e/dev/docs/operations/[operationId].md(1 hunks)e2e/dev/docs/operations/[operationId].paths.js(1 hunks)e2e/dev/docs/tags/[tag].md(1 hunks)e2e/dev/docs/tags/[tag].paths.js(1 hunks)e2e/dev/docs/without-sidebar.md(1 hunks)e2e/dev/package.json(1 hunks)e2e/dev/public/openapi.json(1 hunks)e2e/home.spec.ts(1 hunks)e2e/one-page.spec.ts(1 hunks)package.json(3 hunks)playwright.config.ts(1 hunks)pnpm-workspace.yaml(1 hunks)
🔇 Additional comments (10)
playwright.config.ts (1)
40-43: Consider enabling WebKit tests for better coverage.WebKit tests are currently disabled. Consider enabling them for better browser coverage, especially for Safari users.
pnpm-workspace.yaml (1)
3-3: LGTM!The addition of e2e/dev package to the workspace is correct and aligns with the PR objectives.
e2e/dev/docs/introduction.md (1)
5-7: Verify OpenAPI component imports and required props.The components
<OAInfo />and<OAServers />might require props that aren't being passed.✅ Verification successful
Components usage matches documentation examples pattern
The components are used without props in other example/documentation files, suggesting this is the intended pattern for documentation. Similar usage is found in
docs/example/introduction.mdand some instances indocs/pages/introduction.md.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if components are properly imported and if they require props ast-grep --pattern 'import { OAInfo, OAServers } from $_' # Check component usage patterns in other files rg -A 2 '<OAInfo' rg -A 2 '<OAServers'Length of output: 1634
.gitignore (1)
11-15: LGTM! Well-organized Playwright ignore patterns.The Playwright-specific ignore patterns are properly added and clearly sectioned.
e2e/dev/docs/without-sidebar.md (1)
8-12: Verify OpenAPI spec import and type safety.The OpenAPI spec is imported directly from a JSON file. Let's verify its existence and type safety.
✅ Verification successful
OpenAPI spec import and type safety verified successfully
The OpenAPI spec file exists at the correct location and the type system properly handles the imported JSON through the OpenAPI.Document type definition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the OpenAPI spec file exists fd 'openapi.json' e2e/dev/public # Check for type definitions ast-grep --pattern 'interface $_ { $$$ spec?: $_ $$$ }'Length of output: 1593
Script:
#!/bin/bash # Check for OpenAPI type imports and definitions rg "OpenAPI\." -A 2 ast-grep --pattern 'import { $_ } from "@openapi-contrib/openapi-schema-to-typescript"'Length of output: 6451
e2e/dev/docs/index.md (1)
1-14: LGTM! Homepage configuration follows VitePress conventions.The front matter configuration properly sets up the homepage with a clear hierarchy and navigation structure.
docs/package.json (1)
34-34: LGTM! Version bump for vitepress.The patch version update from ^1.6.2 to ^1.6.3 aligns with the changes in the root package.json.
package.json (2)
47-48: LGTM! New e2e development script.The script follows the naming convention and correctly points to the e2e development environment.
65-65: LGTM! Dependencies for e2e testing.The addition of Playwright for e2e testing and the consistent vitepress version update across package files are appropriate.
Also applies to: 89-89
e2e/dev/public/openapi.json (1)
1-26: LGTM! Well-structured API metadata.The OpenAPI specification includes comprehensive metadata, proper server configuration, and security schemes.
enzonotario
force-pushed
the
feature/e2e
branch
from
18b19bc to
57f2daf
Compare
commit: |
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (8)
e2e/one-page.spec.ts (1)
3-9: 🛠️ Refactor suggestionEnhance test robustness with additional assertions.
The current test only relies on screenshot comparison. Add more specific assertions for better reliability.
Apply these improvements:
test('one-page', async ({ page }) => { await page.goto('/one-page') + + // Wait for content to be fully loaded + await page.waitForSelector('.content-container', { state: 'visible' }) + + // Verify specific elements + await expect(page.locator('.content-container')).toBeVisible() + + // Check accessibility + await page.checkA11y() await expect(page).toHaveScreenshot({ fullPage: true, }) })🧰 Tools
🪛 GitHub Check: test
[failure] 3-3: e2e/one-page.spec.ts
Error: Playwright Test did not expect test() to be called here.
Most common reasons include:
- You are calling test() in a configuration file.
- You are calling test() in a file that is imported by the configuration file.
- You have two different versions of @playwright/test. This usually happens
when one of the dependencies in your package.json depends on @playwright/test.
❯ TestTypeImpl._currentSuite node_modules/.pnpm/playwright@1.50.0/node_modules/playwright/lib/common/testType.js:74:13
❯ TestTypeImpl._createTest node_modules/.pnpm/playwright@1.50.0/node_modules/playwright/lib/common/testType.js:80:24
❯ node_modules/.pnpm/playwright@1.50.0/node_modules/playwright/lib/transform/transform.js:288:12
❯ e2e/one-page.spec.ts:3:1🪛 GitHub Actions: Run Test
[error] 3-3: Playwright Test configuration error: Unexpected test() call. This might be due to calling test() in a configuration file, importing test() in a configuration file, or having multiple versions of @playwright/test.
e2e/dev/docs/operations/[operationId].md (1)
14-15:⚠️ Potential issueAdd error handling for invalid operationId.
The operationId is extracted from route params without validation.
Apply this diff:
-const operationId = route.data.params.operationId +const operationId = route.data.params.operationId +if (!operationId) { + throw new Error('Invalid operation ID') +}e2e/dev/docs/tags/[tag].md (1)
14-15:⚠️ Potential issueAdd error handling for invalid tag.
The tag is extracted from route params without validation.
Apply this diff:
-const tag = route.data.params.tag +const tag = route.data.params.tag +if (!tag) { + throw new Error('Invalid tag') +}e2e/dev/docs/tags/[tag].paths.js (1)
4-17:⚠️ Potential issueAdd error handling and type safety.
The paths configuration needs validation for spec loading and tag names.
Create a utility file and add validation as suggested in the previous review.
e2e/dev/docs/operations/[operationId].paths.js (1)
1-17:⚠️ Potential issueRefactor to reduce duplication and add validation.
Consider creating shared utilities for spec validation and adding proper error handling.
Follow the previous review suggestion to create a spec validator utility.
.github/workflows/e2e.yml (3)
12-14: 🧹 Nitpick (assertive)Update checkout action to v4.
The checkout action version is outdated.
- uses: actions/checkout@v3 + uses: actions/checkout@v4
14-17:⚠️ Potential issueAdd Node.js setup step.
Missing Node.js setup could lead to version inconsistencies.
Add after checkout as suggested in the previous review.
27-29: 🧹 Nitpick (assertive)Add test retry configuration.
Playwright tests should have retry configuration to handle flaky tests.
- name: Run Playwright tests - run: pnpm exec playwright test + run: pnpm exec playwright test --retries=2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (5)
e2e/home.spec.ts-snapshots/home-1-chromium-linux.pngis excluded by!**/*.pnge2e/home.spec.ts-snapshots/home-1-firefox-linux.pngis excluded by!**/*.pnge2e/one-page.spec.ts-snapshots/one-page-1-chromium-linux.pngis excluded by!**/*.pnge2e/one-page.spec.ts-snapshots/one-page-1-firefox-linux.pngis excluded by!**/*.pngpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
.github/workflows/e2e.yml(1 hunks).gitignore(1 hunks)e2e/dev/.gitignore(1 hunks)e2e/dev/docs/.vitepress/config.ts(1 hunks)e2e/dev/docs/.vitepress/theme/index.ts(1 hunks)e2e/dev/docs/index.md(1 hunks)e2e/dev/docs/introduction.md(1 hunks)e2e/dev/docs/one-page.md(1 hunks)e2e/dev/docs/operations/[operationId].md(1 hunks)e2e/dev/docs/operations/[operationId].paths.js(1 hunks)e2e/dev/docs/tags/[tag].md(1 hunks)e2e/dev/docs/tags/[tag].paths.js(1 hunks)e2e/dev/docs/without-sidebar.md(1 hunks)e2e/dev/package.json(1 hunks)e2e/dev/public/openapi.json(1 hunks)e2e/home.spec.ts(1 hunks)e2e/one-page.spec.ts(1 hunks)package.json(2 hunks)playwright.config.ts(1 hunks)pnpm-workspace.yaml(1 hunks)
🧰 Additional context used
🪛 GitHub Check: test
e2e/one-page.spec.ts
[failure] 3-3: e2e/one-page.spec.ts
Error: Playwright Test did not expect test() to be called here.
Most common reasons include:
- You are calling test() in a configuration file.
- You are calling test() in a file that is imported by the configuration file.
- You have two different versions of @playwright/test. This usually happens
when one of the dependencies in your package.json depends on @playwright/test.
❯ TestTypeImpl._currentSuite node_modules/.pnpm/playwright@1.50.0/node_modules/playwright/lib/common/testType.js:74:13
❯ TestTypeImpl._createTest node_modules/.pnpm/playwright@1.50.0/node_modules/playwright/lib/common/testType.js:80:24
❯ node_modules/.pnpm/playwright@1.50.0/node_modules/playwright/lib/transform/transform.js:288:12
❯ e2e/one-page.spec.ts:3:1
e2e/home.spec.ts
[failure] 3-3: e2e/home.spec.ts
Error: Playwright Test did not expect test() to be called here.
Most common reasons include:
- You are calling test() in a configuration file.
- You are calling test() in a file that is imported by the configuration file.
- You have two different versions of @playwright/test. This usually happens
when one of the dependencies in your package.json depends on @playwright/test.
❯ TestTypeImpl._currentSuite node_modules/.pnpm/playwright@1.50.0/node_modules/playwright/lib/common/testType.js:74:13
❯ TestTypeImpl._createTest node_modules/.pnpm/playwright@1.50.0/node_modules/playwright/lib/common/testType.js:80:24
❯ node_modules/.pnpm/playwright@1.50.0/node_modules/playwright/lib/transform/transform.js:288:12
❯ e2e/home.spec.ts:3:1
🪛 GitHub Actions: Run Test
e2e/one-page.spec.ts
[error] 3-3: Playwright Test configuration error: Unexpected test() call. This might be due to calling test() in a configuration file, importing test() in a configuration file, or having multiple versions of @playwright/test.
e2e/home.spec.ts
[error] 3-3: Playwright Test configuration error: Unexpected test() call. This might be due to calling test() in a configuration file, importing test() in a configuration file, or having multiple versions of @playwright/test.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: test
🔇 Additional comments (22)
e2e/dev/docs/.vitepress/theme/index.ts (1)
8-18: Consider adding error handling and more explicit configuration.Currently,
config: {}is empty, and there's no check to ensurespecis successfully loaded. Handling errors (e.g., a missing or invalid OpenAPI spec) will help prevent run-time failures. Consider expanding your configuration object to include relevant options and add error handling for missing or malformed specs, either by logging a descriptive error or providing default fallbacks. This was also suggested in a previous review.e2e/dev/docs/.vitepress/config.ts (2)
6-10: Add explicit type annotation for better type safety.Using an explicit type annotation for
sidebarhelps the TypeScript compiler catch potential errors early and provides better IntelliSense. This was also suggested in a previous review.
21-51: Verify generated links and paths against the actual routes.While dynamically building the sidebar from the OpenAPI spec is convenient, ensure that all generated links (e.g.,
'/introduction','/operations/') map to valid documentation pages. Mismatched links can lead to 404 errors or broken pages.Would you like a shell script to scan the documentation for matching routes and confirm that all links exist?
playwright.config.ts (2)
3-3: Consider making the port configurable via environment variables.Hardcoding the port simplifies local usage but can cause conflicts with other services. Adopting an environment-based approach grants greater flexibility in CI and multi-project environments. This was also suggested in a previous review.
20-26: Add navigation or action timeouts for stability.Under slow environments or complex pages, default timeouts may be insufficient, potentially causing flaky tests. Consider adding explicit timeouts (e.g.,
navigationTimeout,actionTimeout) to ensure reliable test behavior during load or network delays. This was also suggested in a previous review.pnpm-workspace.yaml (1)
3-3: LGTM! Workspace configuration properly includes e2e package.The addition of the
e2e/devpackage to the workspace is correct and aligns with the PR's objective of introducing end-to-end tests.e2e/dev/.gitignore (1)
1-6: Add Playwright-specific ignore patterns.While the basic ignore patterns are good, the file should include Playwright-specific patterns for test results and reports.
Add these Playwright-specific patterns:
node_modules dist .temp cache .idea .vscode + +# Playwright +/test-results/ +/playwright-report/ +/blob-report/ +/playwright/.cache/e2e/dev/docs/introduction.md (1)
5-7: Verify OpenAPI component availability.Please ensure that the
OAInfoandOAServerscomponents are properly registered and available in the VitePress setup.✅ Verification successful
Components are properly registered and available ✅
Both
OAInfoandOAServerscomponents are correctly imported from the Common directory and properly exported in the component registry.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for component registrations rg -l "OAInfo|OAServers" --type tsLength of output: 60
Script:
#!/bin/bash # Check component definitions and exports cat src/components/index.tsLength of output: 1953
.gitignore (1)
11-15: LGTM! Proper Playwright ignore patterns added.The Playwright-specific ignore patterns are correctly added and properly sectioned.
e2e/dev/docs/one-page.md (2)
6-10: Add error handling for useData hook.Consider adding error handling in case the hook fails.
<script setup lang="ts"> import { useData } from 'vitepress' -const { isDark } = useData() +const { isDark = false } = useData() ?? {} </script>
12-12: The isDark prop is passed but not implemented in OASpec component.The OASpec component does not define or use the isDark prop, yet it's being passed here.
e2e/home.spec.ts (1)
3-10: Add performance metrics and strengthen assertions.The home page test could benefit from performance measurements and more specific assertions.
🧰 Tools
🪛 GitHub Check: test
[failure] 3-3: e2e/home.spec.ts
Error: Playwright Test did not expect test() to be called here.
Most common reasons include:
- You are calling test() in a configuration file.
- You are calling test() in a file that is imported by the configuration file.
- You have two different versions of @playwright/test. This usually happens
when one of the dependencies in your package.json depends on @playwright/test.
❯ TestTypeImpl._currentSuite node_modules/.pnpm/playwright@1.50.0/node_modules/playwright/lib/common/testType.js:74:13
❯ TestTypeImpl._createTest node_modules/.pnpm/playwright@1.50.0/node_modules/playwright/lib/common/testType.js:80:24
❯ node_modules/.pnpm/playwright@1.50.0/node_modules/playwright/lib/transform/transform.js:288:12
❯ e2e/home.spec.ts:3:1🪛 GitHub Actions: Run Test
[error] 3-3: Playwright Test configuration error: Unexpected test() call. This might be due to calling test() in a configuration file, importing test() in a configuration file, or having multiple versions of @playwright/test.
e2e/dev/docs/without-sidebar.md (2)
14-14: Consider adding error boundaries for spec rendering.Add error handling in case the spec fails to load or is invalid.
10-10: Verify the OpenAPI spec import path.Ensure the relative path to
openapi.jsonis correct and the file exists in the public directory.✅ Verification successful
OpenAPI spec import path is correct ✅
The file exists at the expected location and the relative path '../public/openapi.json' correctly resolves to the OpenAPI specification file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the OpenAPI spec file exists cd e2e/dev && [ -f "public/openapi.json" ] && echo "File exists" || echo "File missing"Length of output: 78
e2e/dev/docs/index.md (1)
10-13: Verify documentation links.Please ensure:
- The internal link to
/introductionpoints to an existing page- The external documentation URL is accessible
✅ Verification successful
Links verified successfully
Both the internal link to
/introductionand the external documentation URL are valid and accessible.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if introduction page exists fd -t f "introduction.md" e2e/dev/docs/ # Verify external URL curl -I https://vitepress-openapi.vercel.app/Length of output: 945
e2e/dev/package.json (2)
10-13: Add Playwright test dependency.The @playwright/test package is required for e2e testing but missing from devDependencies.
4-9: Add script for running e2e tests.The scripts section should include a command for running Playwright tests to align with the e2e testing objectives.
package.json (2)
47-48: LGTM: E2E development script added.The script addition aligns with the PR objective of introducing end-to-end tests.
65-65: LGTM: Playwright test framework added.Adding Playwright as a development dependency is appropriate for implementing end-to-end tests.
e2e/dev/public/openapi.json (3)
1-18: LGTM: Well-structured API information and server configuration.The API metadata is comprehensive, including title, description, version, and contact information. The mock server configuration is properly defined.
699-729: Consider enhancing the Error schema.The Error schema follows RFC 7807, which is excellent. Consider adding a
traceIdfield to help with debugging and support requests.
57-82: Consider adding rate limiting information.The API endpoints are well-documented with clear descriptions, parameters, and responses. Consider adding rate limiting headers in the responses to help clients handle API quotas effectively.
enzonotario
force-pushed
the
feature/e2e
branch
from
57f2daf to
66995e6
Compare
enzonotario
force-pushed
the
feature/e2e
branch
from
66995e6 to
74465f5
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (7)
e2e/dev/docs/operations/[operationId].md (2)
14-15:⚠️ Potential issueAdd error handling for invalid operationId.
The operationId is extracted from route params without validation.
Apply this diff:
-const operationId = route.data.params.operationId +const operationId = route.data.params.operationId +if (!operationId) { + throw new Error('Invalid operation ID') +}
12-13: 🧹 Nitpick (assertive)Add type safety for isDark property.
The isDark property should be explicitly typed for better type safety.
Apply this diff:
-const { isDark } = useData() +const { isDark } = useData<{ isDark: boolean }>()e2e/dev/docs/tags/[tag].md (2)
14-15:⚠️ Potential issueAdd error handling for invalid tag.
The tag is extracted from route params without validation.
Apply this diff:
-const tag = route.data.params.tag +const tag = route.data.params.tag +if (!tag) { + throw new Error('Invalid tag') +}
17-17: 🧹 Nitpick (assertive)Add prop type validation for OASpec component.
The tags prop should be validated before passing to the component.
Apply this diff:
-<OASpec :tags="[tag]" :isDark="isDark" hide-info hide-servers hide-paths-summary /> +<OASpec :tags="tag ? [tag] : []" :isDark="!!isDark" hide-info hide-servers hide-paths-summary />e2e/dev/docs/tags/[tag].paths.js (1)
4-17:⚠️ Potential issueAdd error handling and type safety.
The paths configuration needs error handling for spec loading and tag name validation.
Create a new utility file
e2e/dev/docs/utils/spec-validator.tsand modify this file as suggested in the previous review.e2e/dev/docs/operations/[operationId].paths.js (1)
1-17:⚠️ Potential issueRefactor to reduce duplication and add validation.
Consider creating a shared spec validation utility and adding operation ID validation.
Create a shared utility file and modify this file as suggested in the previous review.
e2e/dev/docs/.vitepress/theme/index.ts (1)
11-14: 🧹 Nitpick (assertive)Consider adding configuration options and error handling.
The empty config object might benefit from explicit configuration options.
Apply this diff:
const openapi = useOpenapi({ spec, - config: {}, + config: { + // Add specific configuration options here + // For example: defaultExpanded: true + }, }); + +// Add error handling +if (!spec) { + console.error('Failed to load OpenAPI specification'); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (5)
e2e/home.spec.ts-snapshots/home-1-chromium-linux.pngis excluded by!**/*.pnge2e/home.spec.ts-snapshots/home-1-firefox-linux.pngis excluded by!**/*.pnge2e/one-page.spec.ts-snapshots/one-page-1-chromium-linux.pngis excluded by!**/*.pnge2e/one-page.spec.ts-snapshots/one-page-1-firefox-linux.pngis excluded by!**/*.pngpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
.github/workflows/e2e.yml(1 hunks).gitignore(1 hunks)e2e/dev/.gitignore(1 hunks)e2e/dev/docs/.vitepress/config.ts(1 hunks)e2e/dev/docs/.vitepress/theme/index.ts(1 hunks)e2e/dev/docs/index.md(1 hunks)e2e/dev/docs/introduction.md(1 hunks)e2e/dev/docs/one-page.md(1 hunks)e2e/dev/docs/operations/[operationId].md(1 hunks)e2e/dev/docs/operations/[operationId].paths.js(1 hunks)e2e/dev/docs/tags/[tag].md(1 hunks)e2e/dev/docs/tags/[tag].paths.js(1 hunks)e2e/dev/docs/without-sidebar.md(1 hunks)e2e/dev/package.json(1 hunks)e2e/dev/public/openapi.json(1 hunks)e2e/home.spec.ts(1 hunks)e2e/one-page.spec.ts(1 hunks)package.json(2 hunks)playwright.config.ts(1 hunks)pnpm-workspace.yaml(1 hunks)vite.config.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Publish Any Commit
vite.config.ts
[error] 27-27: Invalid property 'test' in configuration. Object literal may only specify known properties, and 'test' does not exist in type 'UserConfigExport'.
🪛 GitHub Actions: Playwright Tests
vite.config.ts
[error] 27-27: Invalid property 'test' in configuration. Object literal may only specify known properties, and 'test' does not exist in type 'UserConfigExport'.
🔇 Additional comments (28)
e2e/dev/docs/.vitepress/config.ts (1)
6-6: Add explicit type annotation for better type safety.
This feedback was provided previously. Adding a type annotation can help prevent errors and improve maintainability.playwright.config.ts (3)
3-3: Use environment variable for the port.
This was previously suggested. Make the port configurable to accommodate different testing environments.
20-26: Add explicit timeouts for stable tests.
This was previously suggested. Setting action and navigation timeouts helps accommodate slow environments and prevents flaky tests.
66-71: Verify the local dev server command.
This was previously suggested. Ensure thatcd e2e/dev && vitepress dev docs --port ${port}aligns with your desired environment and usage (production vs. development).pnpm-workspace.yaml (1)
3-3: LGTM! Workspace configuration is correct.The addition of
e2e/devto the workspace packages is appropriate for the e2e testing setup.e2e/dev/.gitignore (1)
1-6: Add Playwright-specific ignore patterns for consistency.For consistency with the root
.gitignore, please add the following Playwright-specific patterns:node_modules dist .temp cache .idea .vscode + +# Playwright +/test-results/ +/playwright-report/ +/blob-report/ +/playwright/.cache/e2e/dev/docs/introduction.md (1)
5-7: Verify OpenAPI component imports.The
<OAInfo />and<OAServers />components are used but their imports are not visible. Please ensure:
- The components are properly imported
- Add descriptive text around the components to explain their purpose
.gitignore (1)
11-15: LGTM! Playwright ignore patterns are complete.The added Playwright-specific ignore patterns are comprehensive and follow best practices.
e2e/one-page.spec.ts (3)
3-9: Enhance test robustness with additional assertions.While screenshot comparison is useful, consider adding:
- Accessibility checks using
page.checkA11y()- Loading state verification
- Specific element assertions
- Error handling for navigation failures
1-2: Fix Playwright configuration setup.The test requires proper configuration. Create a
playwright.config.tsfile in the e2e directory with appropriate settings for the test environment.
6-8: 🛠️ Refactor suggestionEstablish screenshot baseline management strategy.
Screenshot-based testing requires careful management:
- Document the process for updating baseline screenshots
- Consider viewport size variations
- Handle dynamic content that might cause flaky tests
Add configuration for screenshot comparison:
await expect(page).toHaveScreenshot({ fullPage: true, + threshold: 0.2, // Allow small variations + animations: 'disabled', + scale: 'device', })Likely invalid or redundant comment.
e2e/dev/docs/one-page.md (1)
6-10: Add error handling for useData hook.The dark mode implementation should handle potential hook failures.
<script setup lang="ts"> import { useData } from 'vitepress' -const { isDark } = useData() +const { isDark = false } = useData() ?? {} </script>e2e/home.spec.ts (1)
3-10: 🛠️ Refactor suggestionEnhance test coverage and assertions.
The test should include:
- Critical element checks
- Performance measurements
- More specific title assertion
test('home', async ({ page }) => { await page.goto('/') - await expect(page).toHaveTitle(/VitePress OpenAPI/) + // More specific title check + await expect(page).toHaveTitle(/^VitePress OpenAPI/) + + // Verify critical elements + await expect(page.locator('nav')).toBeVisible() + await expect(page.locator('.content-container')).toBeVisible() + + // Measure performance + const timing = await page.evaluate(() => performance.timing.toJSON()) + expect(timing.domContentLoadedEventEnd - timing.navigationStart).toBeLessThan(3000) await expect(page).toHaveScreenshot({ fullPage: true, }) })Likely invalid or redundant comment.
e2e/dev/docs/without-sidebar.md (1)
14-14: Add error boundaries for spec rendering.Implement error handling for spec loading failures.
-<OASpec :spec="spec" :isDark="isDark" /> +<ClientOnly> + <Suspense> + <OASpec :spec="spec" :isDark="isDark" /> + <template #fallback>Loading...</template> + </Suspense> +</ClientOnly>e2e/dev/package.json (2)
4-9: Add script for running e2e tests.Include a command for running Playwright tests.
"scripts": { "dev": "vitepress dev docs", "build": "vitepress build docs", "preview": "vitepress preview docs", - "start": "vitepress dev docs" + "start": "vitepress dev docs", + "test:e2e": "playwright test" },
10-13: Add Playwright test dependency.The @playwright/test package is required for e2e testing.
"devDependencies": { "vitepress": "^1.6.3", - "vitepress-openapi": "workspace:*" + "vitepress-openapi": "workspace:*", + "@playwright/test": "^1.50.0" }.github/workflows/e2e.yml (6)
1-11: LGTM! Workflow configuration looks good.The workflow triggers and job configuration are well-defined with appropriate timeout.
12-14: Update checkout action to v4.The checkout action version is outdated.
- uses: actions/checkout@v3 + uses: actions/checkout@v4
14-17: Add Node.js setup step.Missing Node.js setup could lead to version inconsistencies.
- name: Install pnpm uses: pnpm/action-setup@v4.0.0 + - name: Setup Node.js + uses: actions/setup-node@v4 + with: + node-version: 20 + cache: 'pnpm'
18-20: Add caching for dependencies.Missing dependency caching will slow down the workflow.
- name: Install dependencies + env: + HUSKY: 0 + run: | + pnpm config set store-dir .pnpm-store + pnpm install --frozen-lockfile - run: pnpm install + - name: Cache pnpm store + uses: actions/cache@v3 + with: + path: .pnpm-store + key: ${{ runner.os }}-pnpm-${{ hashFiles('**/pnpm-lock.yaml') }} + restore-keys: | + ${{ runner.os }}-pnpm-
27-29: Add test retry configuration.Playwright tests should have retry configuration to handle flaky tests.
- name: Run Playwright tests - run: pnpm exec playwright test + run: pnpm exec playwright test --retries=2
30-37: LGTM! Artifact upload configuration is correct.The artifact upload step is well-configured with appropriate retention period.
package.json (1)
47-48: LGTM! E2E development script is well-defined.The script correctly sets up the development environment for e2e testing.
e2e/dev/public/openapi.json (5)
19-26: Consider adding scope information to the security schemes.While the security schemes are correctly defined, consider adding scope information to the bearer authentication to better define access levels.
"bearerAuth": { "type": "http", - "scheme": "bearer" + "scheme": "bearer", + "bearerFormat": "JWT", + "description": "JWT token obtained from the authentication endpoint" }
27-433: Consider adding rate limiting information.The API endpoints are well-documented with clear descriptions, parameters, and responses. Consider adding rate limiting headers in the responses to help clients handle API quotas effectively.
"responses": { "200": { "description": "OK", "headers": { + "X-RateLimit-Limit": { + "schema": { + "type": "integer" + }, + "description": "Request limit per hour" + }, + "X-RateLimit-Remaining": { + "schema": { + "type": "integer" + }, + "description": "Remaining requests for the hour" + } + }, "content": {
434-731: Consider enhancing the Error schema.The Error schema follows RFC 7807, which is excellent. Consider adding a
traceIdfield to help with debugging and support requests."Error": { "type": "object", "description": "RFC 7807 (https://datatracker.ietf.org/doc/html/rfc7807)", "properties": { + "traceId": { + "type": "string", + "description": "Unique identifier for tracking the error across systems", + "examples": [ + "1234-5678-9012-3456" + ] + }, "type": {
538-546: Enhance password validation.While the password has a minimum length requirement, consider adding additional validation rules for better security.
"password": { "type": "string", "minLength": 8, + "pattern": "^(?=.*[A-Za-z])(?=.*\\d)[A-Za-z\\d@$!%*#?&]{8,}$", + "description": "Password must contain at least 8 characters, including one letter and one number", "examples": [ "i-love-rock", "rock-n-roll" ] }
373-432: Add password requirements in the signup endpoint description.The signup endpoint should document password requirements in its description to guide API consumers.
-"description": "Create a user account to access exclusive content about Argentine Rock legends.", +"description": "Create a user account to access exclusive content about Argentine Rock legends. Password must be at least 8 characters long.",
enzonotario
force-pushed
the
feature/e2e
branch
from
74465f5 to
a74ddc4
Compare
enzonotario
force-pushed
the
feature/e2e
branch
from
a74ddc4 to
35372a5
Compare
enzonotario
force-pushed
the
feature/e2e
branch
from
35372a5 to
a65cb50
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 7
🔭 Outside diff range comments (2)
vite.config.ts (2)
Line range hint
13-14: Add type safety for fileName parameter.The fileName function parameter types could be more specific.
-fileName: (format: string, entryName: string) => `vitepress-openapi.${entryName}.${format}.js`, +type EntryName = 'client' | 'node' +type Format = 'es' | 'umd' | 'cjs' +fileName: (format: Format, entryName: EntryName) => `vitepress-openapi.${entryName}.${format}.js`,
Line range hint
20-24: Expand globals configuration for external dependencies.Consider adding all external dependencies to the globals configuration.
globals: { vue: 'Vue', + vitepress: 'VitePress', + 'vitepress/client': 'VitePressClient', },
♻️ Duplicate comments (13)
e2e/one-page.spec.ts (1)
3-9: 🛠️ Refactor suggestionEnhance test robustness with additional assertions.
The current test relies solely on screenshot comparison. Consider adding more assertions for better test coverage.
test('one-page', async ({ page }) => { await page.goto('/one-page') + + // Wait for content to be fully loaded + await page.waitForSelector('.content-container', { state: 'visible' }) + + // Verify specific elements + await expect(page.locator('.content-container')).toBeVisible() + + // Check accessibility + await page.checkA11y() await expect(page).toHaveScreenshot({ fullPage: true, }) })e2e/dev/docs/one-page.md (1)
6-10: 🧹 Nitpick (assertive)Consider adding error handling for useData hook.
The current implementation might fail if the hook returns undefined.
<script setup lang="ts"> import { useData } from 'vitepress' -const { isDark } = useData() +const { isDark = false } = useData() ?? {} </script>e2e/home.spec.ts (1)
1-10: 🛠️ Refactor suggestion
⚠️ Potential issueFix Playwright test configuration and enhance test coverage.
The test configuration needs to be addressed and the test coverage should be improved.
- First, fix the configuration:
+ // playwright.config.ts + import { defineConfig } from '@playwright/test' + + export default defineConfig({ + testDir: './e2e/tests', + use: { + baseURL: 'http://localhost:5173' + } + })
- Then, enhance the test:
test('home', async ({ page }) => { await page.goto('/') - await expect(page).toHaveTitle(/VitePress OpenAPI/) + // Handle navigation errors + try { + // More specific title check + await expect(page).toHaveTitle(/^VitePress OpenAPI/) + + // Verify critical elements + await expect(page.locator('nav')).toBeVisible() + await expect(page.locator('.content-container')).toBeVisible() + await expect(page.getByRole('link', { name: 'API Reference' })).toBeVisible() + + // Verify navigation + await page.getByRole('link', { name: 'API Reference' }).click() + await expect(page).toHaveURL(/.*\/introduction/) + + await expect(page).toHaveScreenshot({ + fullPage: true, + }) + } catch (error) { + console.error('Home page test failed:', error) + throw error + } })e2e/dev/docs/without-sidebar.md (1)
14-14: 🛠️ Refactor suggestionAdd error boundaries for spec rendering.
The OASpec component needs error handling.
-<OASpec :spec="spec" :isDark="isDark" /> +<ClientOnly> + <Suspense> + <template #default> + <OASpec v-if="spec" :spec="spec" :isDark="isDark" /> + <div v-else class="error-message">Failed to load API specification</div> + </template> + <template #fallback> + <div class="loading">Loading API specification...</div> + </template> + </Suspense> +</ClientOnly>e2e/dev/docs/index.md (1)
11-13: 🛠️ Refactor suggestionAdd security attributes to external link.
External links should include security-related attributes.
- theme: alt text: Documentation - link: https://vitepress-openapi.vercel.app/ + link: https://vitepress-openapi.vercel.app/ + rel: "noopener noreferrer" + target: "_blank"e2e/dev/package.json (2)
4-9:⚠️ Potential issueAdd script for running e2e tests.
The scripts section should include commands for running Playwright tests.
"scripts": { "dev": "vitepress dev docs", "build": "vitepress build docs", "preview": "vitepress preview docs", - "start": "vitepress dev docs" + "start": "vitepress dev docs", + "test:e2e": "playwright test", + "test:e2e:ui": "playwright test --ui" },
10-13:⚠️ Potential issueAdd Playwright test dependency.
The @playwright/test package is required for e2e testing.
"devDependencies": { "vitepress": "^1.6.3", - "vitepress-openapi": "workspace:*" + "vitepress-openapi": "workspace:*", + "@playwright/test": "^1.50.0" }e2e/dev/docs/tags/[tag].md (2)
14-15:⚠️ Potential issueAdd error handling for invalid tag parameter.
The tag is extracted from route params without validation, which could lead to runtime errors.
Apply this diff:
-const tag = route.data.params.tag +const tag = route.data.params.tag +if (!tag) { + throw new Error('Invalid tag') +}
17-17: 🛠️ Refactor suggestionAdd prop type validation for OASpec component.
The component props should be validated to prevent runtime errors.
Apply this diff:
-<OASpec :tags="[tag]" :isDark="isDark" hide-info hide-servers hide-paths-summary /> +<OASpec :tags="tag ? [tag] : []" :isDark="!!isDark" hide-info hide-servers hide-paths-summary />e2e/dev/docs/tags/[tag].paths.js (1)
4-17:⚠️ Potential issueAdd error handling and type safety.
The paths configuration needs validation and type safety improvements.
Create a new utility file
e2e/dev/docs/utils/spec-validator.tsand modify this file as suggested in the previous review.e2e/dev/docs/operations/[operationId].paths.js (1)
4-17: 🛠️ Refactor suggestionRefactor to reduce duplication and add validation.
The paths configuration needs validation and can benefit from shared utilities.
Follow the previous review suggestion to create a shared spec validator utility and implement proper error handling.
e2e/dev/docs/.vitepress/theme/index.ts (2)
11-14: 🧹 Nitpick (assertive)Consider adding configuration options and error handling.
The empty config object could benefit from explicit configuration options.
8-18: 🛠️ Refactor suggestionAdd type safety for the theme configuration.
The theme configuration should be properly typed to prevent runtime errors.
🛑 Comments failed to post (7)
e2e/dev/docs/.vitepress/config.ts (1)
21-51: 🧹 Nitpick (assertive)
Consider dynamic sidebar generation for better maintainability.
The sidebar structure mixes static and dynamic items. Consider fully automating the sidebar generation based on the OpenAPI spec.
sidebar: [ - { - text: 'By Tags', - items: [ - { - text: 'Introduction', - link: '/introduction', - }, - ...sidebar.itemsByTags(), - ], - }, + sidebar.generateSidebarSection('By Tags', [ + { text: 'Introduction', link: '/introduction' }, + ...sidebar.itemsByTags() + ]), // Similar pattern for other sections ]Committable suggestion skipped: line range outside the PR's diff.
playwright.config.ts (1)
67-71: 🛠️ Refactor suggestion
Enhance web server configuration robustness.
The web server configuration could benefit from additional error handling and health checks.
webServer: { command: 'pnpm e2e:dev', port, reuseExistingServer: !process.env.CI, + timeout: 120000, + stdout: 'pipe', + stderr: 'pipe', + readyWhen: { + url: `http://localhost:${port}`, + match: 'ready in', + }, },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.webServer: { command: 'pnpm e2e:dev', port, reuseExistingServer: !process.env.CI, timeout: 120000, stdout: 'pipe', stderr: 'pipe', readyWhen: { url: `http://localhost:${port}`, match: 'ready in', }, },e2e/dev/.gitignore (1)
1-6: 🧹 Nitpick (assertive)
Add additional common ignore patterns.
Consider adding patterns for logs and environment files.
node_modules dist .temp cache .idea .vscode +*.log +.env +.env.local📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.node_modules dist .temp cache .idea .vscode *.log .env .env.localvitest.config.ts (1)
3-7: 🧹 Nitpick (assertive)
Consider enhancing the Vitest configuration.
While the current configuration is functional, consider adding these commonly used options for better test coverage and reporting:
export default defineConfig({ test: { include: ['test/**/*.test.{ts,js}'], + coverage: { + reporter: ['text', 'json', 'html'], + exclude: ['node_modules/', 'test/'], + }, + environment: 'node', + globals: true, }, })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export default defineConfig({ test: { include: ['test/**/*.test.{ts,js}'], coverage: { reporter: ['text', 'json', 'html'], exclude: ['node_modules/', 'test/'], }, environment: 'node', globals: true, }, })e2e/dev/docs/without-sidebar.md (1)
8-12: 🛠️ Refactor suggestion
Add type safety and error handling for spec import.
The spec import and isDark usage could be more robust.
<script setup lang="ts"> import { useData } from 'vitepress' -import spec from '../public/openapi.json' -const { isDark } = useData() +import type { OpenAPISpec } from 'vitepress-openapi' +import { ref, onMounted } from 'vue' + +const spec = ref<OpenAPISpec | null>(null) +const { isDark } = useData<{ isDark: boolean }>() + +onMounted(async () => { + try { + spec.value = await import('../public/openapi.json') + } catch (error) { + console.error('Failed to load OpenAPI spec:', error) + } +})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<script setup lang="ts"> import { useData } from 'vitepress' import type { OpenAPISpec } from 'vitepress-openapi' import { ref, onMounted } from 'vue' const spec = ref<OpenAPISpec | null>(null) const { isDark } = useData<{ isDark: boolean }>() onMounted(async () => { try { spec.value = await import('../public/openapi.json') } catch (error) { console.error('Failed to load OpenAPI spec:', error) } }) </script>e2e/dev/docs/operations/[operationId].md (1)
17-17: 🛠️ Refactor suggestion
Add error boundary for operation rendering.
The OAOperation component needs error handling.
-<OAOperation :operationId="operationId" :isDark="isDark" /> +<ClientOnly> + <Suspense> + <template #default> + <OAOperation + :operationId="operationId" + :isDark="isDark" + @error="(e) => console.error('Operation render error:', e)" + /> + </template> + <template #fallback> + <div class="loading">Loading operation details...</div> + </template> + </Suspense> +</ClientOnly>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<ClientOnly> <Suspense> <template #default> <OAOperation :operationId="operationId" :isDark="isDark" @error="(e) => console.error('Operation render error:', e)" /> </template> <template #fallback> <div class="loading">Loading operation details...</div> </template> </Suspense> </ClientOnly>.github/workflows/e2e.yml (1)
8-11: 🧹 Nitpick (assertive)
Consider reducing the timeout duration.
60 minutes is quite long for e2e tests. Consider reducing it to 30 minutes to catch potential infinite loops or hanging tests earlier.
- timeout-minutes: 60 + timeout-minutes: 30📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.test: timeout-minutes: 30 runs-on: ubuntu-latest steps:
enzonotario
force-pushed
the
feature/e2e
branch
from
a65cb50 to
1e55cae
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 3
♻️ Duplicate comments (1)
e2e/one-page.spec.ts (1)
3-9: 🛠️ Refactor suggestionEnhance test robustness with additional assertions.
While screenshot comparison is useful, consider adding accessibility checks and specific element assertions as suggested in the previous review.
Previous review comment still applies:
- Add accessibility checks using
page.checkA11y()- Add loading state verification
- Include specific element assertions
- Add error handling for navigation failures
🛑 Comments failed to post (3)
playwright.config.ts (1)
29-64: 🧹 Nitpick (assertive)
Consider enabling additional browser configurations.
Only Chromium and Firefox are enabled for testing. Consider enabling other browsers (especially WebKit) and mobile viewports to ensure broader compatibility coverage.
Apply this diff to enable WebKit testing:
projects: [ { name: 'chromium', use: { ...devices['Desktop Chrome'] }, }, { name: 'firefox', use: { ...devices['Desktop Firefox'] }, }, - // { - // name: 'webkit', - // use: { ...devices['Desktop Safari'] }, - // }, + { + name: 'webkit', + use: { ...devices['Desktop Safari'] }, + },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.projects: [ { name: 'chromium', use: { ...devices['Desktop Chrome'] }, }, { name: 'firefox', use: { ...devices['Desktop Firefox'] }, }, { name: 'webkit', use: { ...devices['Desktop Safari'] }, }, /* Test against mobile viewports. */ // { // name: 'Mobile Chrome', // use: { ...devices['Pixel 5'] }, // }, // { // name: 'Mobile Safari', // use: { ...devices['iPhone 12'] }, // }, /* Test against branded browsers. */ // { // name: 'Microsoft Edge', // use: { ...devices['Desktop Edge'], channel: 'msedge' }, // }, // { // name: 'Google Chrome', // use: { ...devices['Desktop Chrome'], channel: 'chrome' }, // }, ],e2e/dev/.gitignore (1)
1-6: 🧹 Nitpick (assertive)
Add missing Playwright-specific entries.
For consistency with the root
.gitignore, add the following Playwright-specific entries:node_modules dist .temp cache .idea .vscode + +# Playwright +/test-results/ +/playwright-report/ +/blob-report/ +/playwright/.cache/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.node_modules dist .temp cache .idea .vscode # Playwright /test-results/ /playwright-report/ /blob-report/ /playwright/.cache/vitest.config.ts (1)
3-7: 🧹 Nitpick (assertive)
Consider adding test coverage configuration.
The current configuration is functional but could benefit from coverage reporting.
export default defineConfig({ test: { include: ['test/**/*.test.{ts,js}'], + coverage: { + provider: 'v8', + reporter: ['text', 'json', 'html'], + exclude: ['e2e/**/*'] + } }, })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export default defineConfig({ test: { include: ['test/**/*.test.{ts,js}'], coverage: { provider: 'v8', reporter: ['text', 'json', 'html'], exclude: ['e2e/**/*'] } }, })
enzonotario
force-pushed
the
feature/e2e
branch
from
1e55cae to
778a93d
Compare
enzonotario
force-pushed
the
feature/e2e
branch
from
778a93d to
8527f3d
Compare
enzonotario
force-pushed
the
feature/e2e
branch
from
bae92af to
6e3d06a
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 8
♻️ Duplicate comments (9)
e2e/one-page.spec.ts (2)
3-9:⚠️ Potential issueAdd proper test setup and assertions before screenshot comparison
The screenshot test is failing, and the test lacks proper setup and assertions. Enhance the test with:
test('one-page', async ({ page }) => { await page.goto('/one-page') + + // Wait for content to be fully loaded + await page.waitForSelector('.content-container', { state: 'visible' }) + + // Verify specific elements + await expect(page.locator('.content-container')).toBeVisible() + + // Check accessibility + await page.checkA11y() await expect(page).toHaveScreenshot({ fullPage: true, + threshold: 0.2 // Add threshold to handle minor rendering differences }) })🧰 Tools
🪛 GitHub Actions: Playwright Tests
[error] 6-8: Screenshot test failed: 76916 pixels different from expected in Chromium browser
[error] 6-8: Screenshot test failed: 53396 pixels different from expected in Firefox browser
1-2:⚠️ Potential issueAdd missing Playwright configuration
The test requires proper configuration setup to run consistently.
Create a
playwright.config.tsfile in the e2e directory:import { defineConfig } from '@playwright/test'; export default defineConfig({ testDir: '.', use: { baseURL: 'http://localhost:5173', }, webServer: { command: 'pnpm run dev', url: 'http://localhost:5173', reuseExistingServer: !process.env.CI, }, });e2e/dev/docs/tags/[tag].paths.js (1)
1-17:⚠️ Potential issueAdd error handling and type safety.
The implementation needs error handling for spec loading and tag name validation, along with TypeScript type definitions.
+import type { OpenAPIV3 } from 'openapi-types' import { usePaths } from 'vitepress-openapi' import spec from '../../../../docs/public/openapi.json' +// Validate spec at runtime +const validateSpec = (spec: unknown): spec is OpenAPIV3.Document => { + return typeof spec === 'object' && spec !== null && 'openapi' in spec +} export default { paths() { + if (!validateSpec(spec)) { + throw new Error('Invalid OpenAPI specification') + } + return usePaths({ spec }) .getTags() .map(({ name }) => { + // Validate tag name + if (!name || typeof name !== 'string') { + throw new Error('Invalid tag name in specification') + } + return { params: { tag: name, pageTitle: `${name} - vitepress-openapi`, }, } }) }, }e2e/dev/docs/operations/[operationId].paths.js (1)
1-17:⚠️ Potential issueRefactor to reduce duplication and add validation.
Consider creating shared spec validation utilities and adding proper error handling.
+import type { OpenAPIV3 } from 'openapi-types' import { usePaths } from 'vitepress-openapi' import spec from '../../../../docs/public/openapi.json' +import { validateSpec } from '../utils/spec-validator' +const validateOperationId = (operationId: unknown): operationId is string => { + return typeof operationId === 'string' && operationId.length > 0 +} export default { paths() { + if (!validateSpec(spec)) { + throw new Error('Invalid OpenAPI specification') + } + return usePaths({ spec }) .getPathsByVerbs() .map(({ operationId, summary }) => { + if (!validateOperationId(operationId)) { + throw new Error(`Invalid operation ID: ${String(operationId)}`) + } + return { params: { operationId, pageTitle: `${summary} - vitepress-openapi`, }, } }) }, }.github/workflows/e2e.yml (4)
12-14:⚠️ Potential issueUpdate checkout action to v4.
The checkout action version is outdated.
- uses: actions/checkout@v3 + uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.4)
13-13: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
14-17:⚠️ Potential issueAdd Node.js setup step.
Missing Node.js setup could lead to version inconsistencies.
- name: Install pnpm uses: pnpm/action-setup@v4.0.0 + - name: Setup Node.js + uses: actions/setup-node@v4 + with: + node-version: 20 + cache: 'pnpm'
18-20: 🛠️ Refactor suggestionAdd caching for dependencies.
Missing dependency caching will slow down the workflow.
- name: Install dependencies + env: + HUSKY: 0 + run: | + pnpm config set store-dir .pnpm-store + pnpm install --frozen-lockfile - run: pnpm install + - name: Cache pnpm store + uses: actions/cache@v3 + with: + path: .pnpm-store + key: ${{ runner.os }}-pnpm-${{ hashFiles('**/pnpm-lock.yaml') }} + restore-keys: | + ${{ runner.os }}-pnpm-
27-29: 🧹 Nitpick (assertive)Add test retry configuration.
Playwright tests should have retry configuration to handle flaky tests.
- name: Run Playwright tests - run: pnpm exec playwright test + run: pnpm exec playwright test --retries=2package.json (1)
65-65: 🧹 Nitpick (assertive)Consider pinning the Playwright version.
Using
^1.50.0allows minor version updates which could introduce breaking changes in tests.- "@playwright/test": "^1.50.0", + "@playwright/test": "1.50.0",
🛑 Comments failed to post (8)
e2e/dev/docs/index.md (2)
7-13: 🧹 Nitpick (assertive)
Add test attributes for action buttons.
To facilitate e2e testing of the action buttons, consider adding test attributes.
actions: - theme: brand text: API Reference link: /introduction + test-id: "action-api-reference" - theme: alt text: Documentation link: https://vitepress-openapi.vercel.app/ + test-id: "action-documentation" rel: "noopener noreferrer"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.actions: - theme: brand text: API Reference link: /introduction test-id: "action-api-reference" - theme: alt text: Documentation link: https://vitepress-openapi.vercel.app/ test-id: "action-documentation" rel: "noopener noreferrer"
4-6: 🧹 Nitpick (assertive)
Add test attributes for e2e testing.
Since this is part of the e2e test setup, consider adding test attributes to make the elements easily selectable in Playwright tests.
hero: - name: "VitePress OpenAPI" - tagline: "Generate documentation from OpenAPI specifications." + name: "VitePress OpenAPI" + name-test-id: "hero-title" + tagline: "Generate documentation from OpenAPI specifications." + tagline-test-id: "hero-tagline"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.hero: name: "VitePress OpenAPI" name-test-id: "hero-title" tagline: "Generate documentation from OpenAPI specifications." tagline-test-id: "hero-tagline"e2e/dev/docs/without-sidebar.md (2)
1-6: 🧹 Nitpick (assertive)
Consider enhancing the page metadata.
The frontmatter configuration could be improved for better documentation structure and SEO:
- Consider expanding outline levels to include more heading depths for detailed API documentation
- Add description metadata for better SEO
--- sidebar: false aside: true -outline: [1, 2] +outline: [1, 2, 3] title: vitepress-openapi +description: OpenAPI documentation without sidebar layout ---📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.--- sidebar: false aside: true outline: [1, 2, 3] title: vitepress-openapi description: OpenAPI documentation without sidebar layout ---
8-12: 🛠️ Refactor suggestion
Add type safety and error handling for spec import.
The script setup could benefit from:
- Type checking for the imported OpenAPI spec
- Error handling for the spec import
<script setup lang="ts"> import { useData } from 'vitepress' -import spec from '@public/openapi.json' +import type { OpenAPIV3 } from 'openapi-types' + +// Ensure type safety for the imported spec +import spec from '@public/openapi.json' assert { type: 'json' } +const validateSpec = (data: unknown): data is OpenAPIV3.Document => { + // Add validation logic here + return true +} + +if (!validateSpec(spec)) { + throw new Error('Invalid OpenAPI specification') +} + const { isDark } = useData() </script>Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
8-8: Inline HTML
Element: script(MD033, no-inline-html)
e2e/dev/docs/.vitepress/theme/index.ts (1)
10-10: 🧹 Nitpick (assertive)
Remove unused parameters from enhanceApp function.
The
routerandsiteDataparameters are defined but never used.- async enhanceApp({ app, router, siteData }) { + async enhanceApp({ app }) {Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 ESLint
[error] 10-10: 'router' is defined but never used. Allowed unused args must match /^_/u.
(unused-imports/no-unused-vars)
[error] 10-10: 'siteData' is defined but never used. Allowed unused args must match /^_/u.
(unused-imports/no-unused-vars)
playwright.config.ts (1)
72-76: 🧹 Nitpick (assertive)
Consider adding health check for the dev server.
The webServer configuration could benefit from a health check to ensure the server is ready.
webServer: { command: 'pnpm e2e:dev', port, reuseExistingServer: !process.env.CI, + // Add health check + timeout: 120 * 1000, + stdout: 'pipe', + stderr: 'pipe', },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.webServer: { command: 'pnpm e2e:dev', port, reuseExistingServer: !process.env.CI, // Add health check timeout: 120 * 1000, stdout: 'pipe', stderr: 'pipe', },🧰 Tools
🪛 ESLint
[error] 75-75: Unexpected use of the global variable 'process'. Use 'require("process")' instead.
(node/prefer-global/process)
e2e/dev/.gitignore (1)
1-6: 🧹 Nitpick (assertive)
Add Playwright-specific entries to .gitignore
While the current entries cover basic Node.js development files, consider adding Playwright-specific entries to maintain consistency with the root .gitignore:
node_modules dist .temp cache .idea .vscode + +# Playwright +/test-results/ +/playwright-report/ +/blob-report/ +/playwright/.cache/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.node_modules dist .temp cache .idea .vscode # Playwright /test-results/ /playwright-report/ /blob-report/ /playwright/.cache/vitest.config.ts (1)
3-7: 🧹 Nitpick (assertive)
Enhance Vitest configuration with additional settings
Consider adding these commonly used Vitest settings to improve test execution:
export default defineConfig({ test: { include: ['test/**/*.test.{ts,js}'], + environment: 'node', + globals: true, + coverage: { + provider: 'v8', + reporter: ['text', 'json', 'html'], + }, }, })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export default defineConfig({ test: { include: ['test/**/*.test.{ts,js}'], environment: 'node', globals: true, coverage: { provider: 'v8', reporter: ['text', 'json', 'html'], }, }, })
Description
Adds e2e tests using Playwright.
Related issues/external references
Types of changes