-
Notifications
You must be signed in to change notification settings - Fork 1
playwright ci 설정 초기화 및 테스트 #112
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis update introduces end-to-end testing infrastructure using Playwright and Lighthouse CI, adds related test scripts and configuration, and implements new test suites for key application flows. Several mock data files are removed from the main source and reintroduced under a new tests/mocks directory. Accessibility improvements are made to modal components, and minor code cleanups and path alias adjustments are included. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as GitHub Actions
participant DevServer as Local Dev Server
participant Playwright as Playwright Test Runner
participant Lighthouse as Lighthouse CI
CI->>DevServer: Start server (npm run dev)
CI->>Playwright: Run E2E tests
Playwright->>DevServer: Interact with app for tests
CI->>Lighthouse: Run Lighthouse audits
Lighthouse->>DevServer: Collect performance/accessibility data
CI->>CI: Upload Playwright and Lighthouse reports as artifacts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (8)
playwright.config.js (3)
15-26: Good CI-aware configuration with minor optimization suggestion.The configuration properly handles CI environment differences. However, consider allowing more parallelism on CI for faster test execution.
Consider increasing CI workers for better performance:
- workers: process.env.CI ? 1 : undefined, + workers: process.env.CI ? 2 : undefined,
28-34: Consider enabling baseURL for consistent test navigation.The trace configuration is appropriate. However, consider uncommenting and configuring the baseURL for more reliable test navigation.
If your application runs on a consistent local URL during testing, enable baseURL:
- // baseURL: 'http://localhost:3000', + baseURL: 'http://localhost:3000', // Update with your actual dev server URL
74-80: Consider enabling webServer for integrated testing.The webServer configuration is commented out but might be valuable for running tests against your actual application.
If you need to test against your local development server, consider enabling this configuration:
- // webServer: { - // command: 'npm run start', - // url: 'http://localhost:3000', - // reuseExistingServer: !process.env.CI, - // }, + webServer: { + command: 'npm run dev', // Update to match your dev script + url: 'http://localhost:3000', // Update to match your dev server URL + reuseExistingServer: !process.env.CI, + },.github/workflows/playwright.yml (2)
3-5: Consider adding push trigger for main branch.The workflow only runs on pull requests. Consider also running on pushes to main to catch issues in the main branch.
on: pull_request: branches: [main] + push: + branches: [main]
19-26: Consider adding caching for better performance.The dependency installation is correct, but adding cache could improve CI performance.
Add caching between the Node setup and dependency installation:
- uses: actions/setup-node@v4 with: node-version: 'lts/*' + cache: 'npm' - name: Install dependencies run: npm citests-examples/demo-todo-app.spec.js (3)
59-59: Add missing semicolon.Missing semicolon after the variable declaration.
- const todoCount = page.getByTestId('todo-count') + const todoCount = page.getByTestId('todo-count');
151-151: Fix comment indentation.Inconsistent indentation in the comment.
- // create a new todo locator + // create a new todo locator
266-266: Add missing semicolon.Missing semicolon after the variable declaration.
- const todoCount = page.getByTestId('todo-count') + const todoCount = page.getByTestId('todo-count');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.github/workflows/playwright.yml(1 hunks).gitignore(1 hunks)package.json(2 hunks)playwright.config.js(1 hunks)tests-examples/demo-todo-app.spec.js(1 hunks)tests/example.spec.js(1 hunks)
🔇 Additional comments (17)
.gitignore (1)
27-31: LGTM! Proper Playwright artifact exclusions.The added .gitignore entries correctly exclude standard Playwright-generated directories from version control. These are the appropriate artifacts to ignore for a Playwright testing setup.
playwright.config.js (1)
37-51: Excellent cross-browser testing setup.The configuration covers all major browser engines (Chromium, Firefox, WebKit) which provides comprehensive cross-browser test coverage.
package.json (3)
11-12: Excellent separation of unit and e2e test scripts.The script separation properly distinguishes between unit tests (vitest) and end-to-end tests (playwright), allowing for targeted test execution in different scenarios.
42-42: Node types addition supports TypeScript configuration.The @types/node addition is appropriate for supporting TypeScript in the Playwright configuration file.
39-39: Playwright version verified as up-to-date@playwright/test is already at version 1.54.1 (released July 11, 2025), which is the latest stable release, and there are no published security advisories as of July 2025. No further action required.
.github/workflows/playwright.yml (2)
12-17: Good use of latest actions and LTS Node.js.The workflow uses the latest action versions and LTS Node.js, which is a good practice for stability and security.
28-33: Excellent artifact handling with proper retention.The workflow correctly uploads test reports regardless of test outcome and sets appropriate retention period.
tests-examples/demo-todo-app.spec.js (10)
1-12: LGTM! Clean test setup with proper TypeScript checking.The setup includes appropriate imports, TypeScript checking, and reusable test data constants. The beforeEach hook correctly navigates to the demo URL for each test.
15-70: Excellent test coverage for todo creation functionality.The tests comprehensively cover adding todos, input field clearing, and list ordering. Good use of Playwright locators and proper validation of both UI state and localStorage persistence.
73-121: Comprehensive testing of mark all completed functionality.Well-structured test group with proper setup/teardown hooks. Tests thoroughly validate the mark all complete feature including UI state, class changes, and localStorage persistence.
125-193: Solid test coverage for individual item operations.The tests effectively validate marking items as complete/incomplete and editing functionality. Good use of locators and proper assertions for UI state validation.
195-258: Excellent comprehensive testing of editing functionality.This test group thoroughly covers editing edge cases including UI state during editing, blur saves, text trimming, empty item removal, and escape cancellation. Well-structured with proper setup hooks.
261-277: Good test coverage for counter functionality.The test properly validates that the todo counter updates correctly as items are added. Includes appropriate localStorage validation.
280-303: Thorough testing of clear completed button functionality.The tests comprehensively cover the clear completed button including display conditions, removal functionality, and proper hiding behavior when no completed items exist.
305-331: Important test for data persistence validation.This test properly validates that todo data persists across page reloads, including completion status and UI state. Critical functionality for ensuring data integrity.
333-409: Excellent routing and navigation test coverage.The routing tests comprehensively cover filtering functionality, back button behavior, and filter highlighting. Good use of
test.stepfor organizing complex test flows and clear test structure.
411-449: Well-implemented helper functions with proper typing.The helper functions provide excellent utilities for localStorage validation with proper JSDoc typing. Functions are well-structured and handle the React TodoMVC localStorage format correctly.
tests/example.spec.js
Outdated
| test('has title', async ({ page }) => { | ||
| await page.goto('https://playwright.dev/'); | ||
|
|
||
| // Expect a title "to contain" a substring. | ||
| await expect(page).toHaveTitle(/Playwright/); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider testing your actual application instead of external sites.
While this example test demonstrates Playwright usage correctly, consider creating tests for your actual application rather than external sites to avoid flaky tests due to external dependencies.
Replace with tests for your application:
- test('has title', async ({ page }) => {
- await page.goto('https://playwright.dev/');
- await expect(page).toHaveTitle(/Playwright/);
- });
+ test('has title', async ({ page }) => {
+ await page.goto('/'); // Assumes baseURL is configured
+ await expect(page).toHaveTitle(/Your App Title/);
+ });📝 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('has title', async ({ page }) => { | |
| await page.goto('https://playwright.dev/'); | |
| // Expect a title "to contain" a substring. | |
| await expect(page).toHaveTitle(/Playwright/); | |
| }); | |
| test('has title', async ({ page }) => { | |
| await page.goto('/'); // Assumes baseURL is configured | |
| await expect(page).toHaveTitle(/Your App Title/); | |
| }); |
🤖 Prompt for AI Agents
In tests/example.spec.js around lines 4 to 9, the test currently navigates to an
external site which can cause flaky tests due to external dependencies. Replace
the test to navigate to a local or internal page of your actual application and
assert the title or other relevant elements there to ensure stable and
meaningful test coverage.
tests/example.spec.js
Outdated
| test('get started link', async ({ page }) => { | ||
| await page.goto('https://playwright.dev/'); | ||
|
|
||
| // Click the get started link. | ||
| await page.getByRole('link', { name: 'Get started' }).click(); | ||
|
|
||
| // Expects page to have a heading with the name of Installation. | ||
| await expect(page.getByRole('heading', { name: 'Installation' })).toBeVisible(); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Good use of role-based selectors, but test your own application.
The test demonstrates proper use of Playwright's role-based selectors, which are accessibility-friendly and resilient. However, focus on testing your actual application functionality.
🤖 Prompt for AI Agents
In tests/example.spec.js around lines 11 to 19, the test currently targets an
external site (playwright.dev) instead of your own application. Modify the test
to navigate to your application's URL and update selectors as needed to verify
your app's functionality using role-based selectors, maintaining accessibility
best practices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/home.spec.js (2)
9-20: Consider improvements for test reliability.The test logic is sound, but there are potential reliability issues:
- The regex text matcher with Korean characters might be fragile
- Mouse wheel scrolling can be inconsistent across environments
- Missing navigation wait before URL assertion
Consider these improvements:
- await expect(page.getByText(/'One a Day' HARUHAN/i)).toBeVisible(); + await expect(page.getByText("'One a Day' HARUHAN")).toBeVisible(); - await page.mouse.wheel(0, 100); + await page.evaluate(() => window.scrollBy(0, 100)); await page.getByText('팀 소개').click(); + await page.waitForURL('/teamInfo'); - await expect(page).toHaveURL('/teamInfo'); + await expect(page).toHaveURL('/teamInfo');
27-51: Comprehensive subscription test with room for improvement.The test covers the subscription flow well but could be enhanced for better reliability and completeness.
Consider these improvements:
await page.getByRole('button', { name: '구독하기' }).first().click(); + await expect(page.getByRole('dialog')).toBeVisible(); await expect(page.getByRole('heading', { name: 'Haruhan 지식 구독' })).toBeVisible(); await page.getByText('하루 하나 (월~금)').click(); await page.getByText('오전 7시').click(); const emailInput = page.getByPlaceholder('example@naver.com'); await emailInput.fill('test@example.com'); await expect(emailInput).toHaveValue('test@example.com'); - await page.getByText('개인정보취급방침에 동의합니다').click(); + const privacyCheckbox = page.getByText('개인정보취급방침에 동의합니다'); + await privacyCheckbox.click(); + await expect(privacyCheckbox).toBeChecked(); - await page.mouse.wheel(0, 100); + await page.evaluate(() => window.scrollBy(0, 100)); const modal = page.getByRole('dialog'); - await modal.getByRole('button', { name: '구독하기' }).click(); + await Promise.all([ + page.waitForResponse(response => response.url().includes('/subscribe')), + modal.getByRole('button', { name: '구독하기' }).click() + ]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
playwright.config.js(1 hunks)src/features/modal/Subscription.jsx(1 hunks)src/features/modal/SubscriptionModal.jsx(6 hunks)tests/home.spec.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/features/modal/Subscription.jsx
🚧 Files skipped from review as they are similar to previous changes (1)
- playwright.config.js
🔇 Additional comments (4)
src/features/modal/SubscriptionModal.jsx (3)
22-22: LGTM! Good whitespace cleanup.Removing trailing spaces improves code consistency and cleanliness.
Also applies to: 26-26, 32-32, 48-48, 52-52, 58-58, 84-84, 92-92, 99-99, 103-103, 118-118, 124-124, 135-135
167-167: Excellent accessibility improvement!The addition of proper ARIA attributes (
role="dialog",aria-modal="true",aria-labelledby="dialog-title") significantly enhances the modal's accessibility for screen readers and assistive technologies.
168-168: Good accessibility enhancement for the close button.Adding
aria-label="모달 닫기"provides clear context for screen readers, and the explicittype="button"follows best practices.tests/home.spec.js (1)
1-8: Well-structured test file with proper organization.The test structure follows Playwright best practices with proper imports, describe blocks, and consistent beforeEach setup.
Also applies to: 22-26
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/content.spec.js (1)
54-57: Bookmark tab test could be more comprehensive.While the test verifies bookmark tab functionality, it only checks the visibility of the first bookmark item. Consider testing that non-bookmarked content is hidden when the bookmark tab is active.
test('북마크 탭을 클릭하면 북마크된 콘텐츠가 보여야 한다', async ({ page }) => { await page.getByText(`북마크한 지식 ${mockBookmarks.length}`).click(); - await expect(page.getByText(mockBookmarks[0].title)).toBeVisible(); + + // 북마크된 콘텐츠가 표시되는지 확인 + for (const bookmark of mockBookmarks) { + await expect(page.getByText(bookmark.title)).toBeVisible(); + } + + // 북마크되지 않은 콘텐츠가 숨겨지는지 확인 + const nonBookmarkedContent = mockContents.filter( + content => !mockBookmarks.some(bookmark => bookmark.id === content.id) + ); + for (const content of nonBookmarkedContent) { + await expect(page.getByText(content.title)).not.toBeVisible(); + } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
.github/workflows/E2ETest_Lighthouse.yml(1 hunks)lighthouserc.json(1 hunks)package.json(2 hunks)src/hooks/useQueryParams.js(1 hunks)src/layout/index.js(1 hunks)src/mocks/addContentRequest.js(0 hunks)src/mocks/contentData.js(0 hunks)src/mocks/dummyEmails.js(0 hunks)src/mocks/popularityData.js(0 hunks)src/mocks/teamInfoData.js(0 hunks)src/pages/NotFound.jsx(1 hunks)tests/content.spec.js(1 hunks)tests/mocks/bookmarkData.js(1 hunks)tests/mocks/contentData.js(1 hunks)tests/mocks/index.js(1 hunks)vite.config.js(1 hunks)
💤 Files with no reviewable changes (5)
- src/mocks/teamInfoData.js
- src/mocks/popularityData.js
- src/mocks/dummyEmails.js
- src/mocks/contentData.js
- src/mocks/addContentRequest.js
✅ Files skipped from review due to trivial changes (8)
- src/layout/index.js
- tests/mocks/contentData.js
- src/pages/NotFound.jsx
- src/hooks/useQueryParams.js
- tests/mocks/bookmarkData.js
- lighthouserc.json
- tests/mocks/index.js
- .github/workflows/E2ETest_Lighthouse.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/content.spec.js (2)
tests/mocks/contentData.js (2)
mockContents(1-63)mockContents(1-63)tests/mocks/bookmarkData.js (2)
mockBookmarks(1-20)mockBookmarks(1-20)
🔇 Additional comments (4)
tests/content.spec.js (4)
1-3: LGTM! Clean imports and proper TypeScript checking.The imports are well-structured with TypeScript checking enabled and proper mock data imports.
6-42: Excellent API mocking setup with proper resource type filtering.The beforeEach setup correctly mocks both content and bookmark API endpoints with realistic response structures. The resource type filtering (
xhrorfetch) ensures only API calls are intercepted while allowing other resources to load normally.
44-52: Comprehensive content visibility test with good use of mock data.The test effectively verifies that all content titles are displayed and uses dynamic counts from the mock data. The loop structure ensures all content items are validated.
59-88: Well-structured navigation test with comprehensive content verification.The test properly mocks the detail API, verifies URL navigation, and checks multiple content sections. The approach of testing both the navigation and the rendered content is thorough and follows good E2E testing practices.
| '@hooks': path.resolve(__dirname, 'src/hooks'), | ||
| '@pages': path.resolve(__dirname, 'src/pages'), | ||
| '@common': path.resolve(__dirname, 'src/common'), | ||
| '@mocks': path.resolve(__dirname, 'src/mocks'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Verify the '@Mocks' alias path matches the actual mock file location.
Based on the AI summary, mock files have been moved from src/mocks/ to tests/mocks/, but this alias still points to src/mocks. This could cause import resolution issues.
🏁 Script executed:
#!/bin/bash
# Description: Check if mock files exist in src/mocks or tests/mocks directory
# Expected: Mock files should be in tests/mocks based on the summary
echo "Checking for mock files in src/mocks:"
fd . src/mocks/ 2>/dev/null || echo "src/mocks directory not found"
echo -e "\nChecking for mock files in tests/mocks:"
fd . tests/mocks/ 2>/dev/null || echo "tests/mocks directory not found"
echo -e "\nSearching for @mocks alias usage:"
rg "@mocks" --type js --type ts -A 2Length of output: 551
Update the @mocks alias to point at the new mocks directory
The project still has one file in src/mocks (knowledgeData.js), but the bulk of your mock files now live under tests/mocks. The current alias in vite.config.js (pointing to src/mocks) will break any imports of the relocated mocks.
Please update the alias:
vite.config.js (around line 56)
- '@mocks': path.resolve(__dirname, 'src/mocks'),
+ '@mocks': path.resolve(__dirname, 'tests/mocks'),Then verify there are no lingering imports targeting the old path.
📝 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.
| '@mocks': path.resolve(__dirname, 'src/mocks'), | |
| '@mocks': path.resolve(__dirname, 'tests/mocks'), |
🤖 Prompt for AI Agents
In vite.config.js at line 56, update the '@mocks' alias to point to the new
mocks directory under tests/mocks instead of src/mocks. Change the path.resolve
argument accordingly to reflect this new location. After updating the alias,
search the codebase for any imports still referencing the old src/mocks path and
update them to use the new alias or correct path to prevent import errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/E2ETest_Lighthouse.yml (1)
19-23: Consider caching node_modules & Playwright browsers to speed up CIEach run performs a fresh
npm ciandplaywright install, adding several minutes.
Add anactions/cachestep keyed onpackage-lock.jsonandPLAYWRIGHT_BROWSERS_PATHto cut job time by ~70 %.This is optional but highly recommended for frequent PRs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/E2ETest_Lighthouse.yml(1 hunks)
⏰ 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: e2e-and-lighthouse
🔇 Additional comments (1)
.github/workflows/E2ETest_Lighthouse.yml (1)
46-52:jqpath may not match Playwright JSON structureThe Playwright JSON reporter nests results under
suites[*].tests[*](nospecslevel unless shard-mode); using.suites[].specs[].tests[]will yieldnulland return 0 counts.Please verify the schema produced by your Playwright version and adjust the query accordingly.
|
|
||
| # 5. 프로젝트 빌드 | ||
| - name: Build project | ||
| run: npm run build | ||
|
|
||
| # 6. Playwright 테스트 실행 (JSON 리포트 저장) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Playwright/Lighthouse failures will short-circuit the rest of the job
Both “Run Playwright tests” (line 28) and “Run Lighthouse CI” (line 31) will exit with a non-zero status when tests or assertions fail.
Because continue-on-error: isn’t set, the subsequent parsing steps (and the PR comment) will be skipped, leaving no feedback artefacts when they are most useful.
- - name: Run Playwright tests
- run: npx playwright test --reporter=json > playwright-result.json
+ - name: Run Playwright tests
+ run: npx playwright test --reporter=json > playwright-result.json
+ continue-on-error: true
...
- - name: Run Lighthouse CI
- run: npx lhci autorun --config=./lighthouserc.json --output=json
+ - name: Run Lighthouse CI
+ run: npx lhci autorun --config=./lighthouserc.json --output=json
+ continue-on-error: 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.
| # 5. 프로젝트 빌드 | |
| - name: Build project | |
| run: npm run build | |
| # 6. Playwright 테스트 실행 (JSON 리포트 저장) | |
| # 6. Playwright 테스트 실행 (JSON 리포트 저장) | |
| - name: Run Playwright tests | |
| run: npx playwright test --reporter=json > playwright-result.json | |
| continue-on-error: true | |
| # 7. Run Lighthouse CI | |
| - name: Run Lighthouse CI | |
| run: npx lhci autorun --config=./lighthouserc.json --output=json | |
| continue-on-error: true |
🤖 Prompt for AI Agents
In .github/workflows/E2ETest_Lighthouse.yml around lines 28 to 33, the steps
"Run Playwright tests" and "Run Lighthouse CI" can fail and stop the entire job,
preventing later steps like parsing results and posting PR comments from
running. To fix this, add `continue-on-error: true` to these steps so the
workflow continues even if tests fail, ensuring feedback steps always run.
E2E Test & Lighthouse Report
|
E2E Test & Lighthouse Report
|
E2E Test & Lighthouse Report
|
E2E Test & Lighthouse Report
|
E2E Test & Lighthouse Report
|
E2E Test & Lighthouse Report
|
Summary by CodeRabbit
New Features
Chores
.gitignoreto exclude Playwright-related files and reports.Refactor
Style
Tests
Documentation
Other