Setting/#187/playwright #188
Conversation
WalkthroughAdds Playwright end-to-end testing: GitHub Actions workflow (uses secret for NEXT_PUBLIC_IMAGE_HOSTNAME), Playwright config, two E2E test suites, npm scripts + devDependency, and .gitignore entries for Playwright artifacts/caches. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant GH as GitHub Actions
participant Runner as CI Runner (ubuntu-latest)
participant Server as Web Server (yarn dev)
participant Playwright as Playwright Test
Dev->>GH: Push / PR to main/master
GH->>Runner: Start "Playwright Tests" job (env from secrets.IMAGE_HOSTNAME)
Runner->>Runner: checkout, setup-node (lts/*), install deps (yarn), playwright install
Runner->>Server: Start webServer (yarn dev) or reuse existing
Runner->>Playwright: Run playwright test (Chromium/Firefox/WebKit)
Playwright-->>Runner: Generate reports (playwright-report/)
Runner->>GH: Upload playwright-report artifact
Estimated code review effort๐ฏ 3 (Moderate) | โฑ๏ธ ~15 minutes Poem
โจ Finishing Touches
๐งช Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. ๐ชง TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 3
๐งน Nitpick comments (5)
.gitignore (1)
41-41: Remove duplicatenode_modules/entry.This entry duplicates the existing
node_modules/ignore pattern on line 4.# Playwright -node_modules/ /test-results/ /playwright-report/ /blob-report/ /playwright/.cache/.github/workflows/playwright.yml (1)
16-17: Consider adding dependency caching for faster CI builds.Adding yarn cache could significantly speed up subsequent workflow runs.
- uses: actions/setup-node@v4 with: node-version: lts/* + cache: 'yarn' - name: Install dependencies - run: npm install -g yarn && yarn + run: yarntests/e2e/example.spec.ts (1)
22-25: LGTM! Good application-specific test structure.This test correctly validates your application's home page functionality using the configured baseURL.
Consider adding more comprehensive tests for your application:
- Navigation testing
- Form interactions
- User authentication flows
- Core feature validation
playwright.config.ts (2)
20-24: Clean up inconsistent retry configuration.There are conflicting retry configurations - one active and one commented.
- retries: 1, // ์คํจ ์ ์ฌ์๋ ํ์ - - // retries: process.env.CI ? 2 : 0, + retries: process.env.CI ? 1 : 0,
31-31: Consider making headless mode CI-specific.Running tests in headed mode during local development can be useful for debugging.
- headless: true, // CI ํ๊ฒฝ์์๋ headless ๊ถ์ฅ + headless: !!process.env.CI,
๐ Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
โ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
๐ Files selected for processing (5)
.github/workflows/playwright.yml(1 hunks).gitignore(1 hunks)package.json(2 hunks)playwright.config.ts(1 hunks)tests/e2e/example.spec.ts(1 hunks)
๐ Additional comments (6)
.gitignore (1)
42-45: LGTM! Proper Playwright artifact patterns.The ignore patterns for Playwright test results, reports, blob reports, and cache directory are correctly configured.
package.json (2)
15-16: LGTM! Well-structured E2E test scripts.The scripts are properly configured:
test:e2efor local development with full reportingtest:e2e:ciwith line reporter for CI environments
48-48: LGTM! Latest Playwright version added.The @playwright/test dependency version ^1.54.2 is recent and appropriate for the devDependencies.
.github/workflows/playwright.yml (1)
8-27: LGTM! Well-configured Playwright CI workflow.The workflow includes:
- Appropriate timeout (60 minutes)
- Latest action versions
- Proper browser installation with dependencies
- Conditional artifact upload with reasonable retention
playwright.config.ts (2)
76-80: LGTM! Proper web server configuration.The webServer setup correctly:
- Starts the dev server before tests
- Reuses existing server in local development
- Uses appropriate URL and command
38-52: LGTM! Comprehensive cross-browser testing setup.The projects configuration enables testing across major browsers using appropriate device presets.
| @@ -11,7 +11,9 @@ | |||
| "cz": "./node_modules/cz-customizable/standalone.js", | |||
| "test": "jest", | |||
| "test:watch": "jeset --watchAll", | |||
There was a problem hiding this comment.
Fix typo in test watch script.
There's a typo: "jeset" should be "jest".
- "test:watch": "jeset --watchAll",
+ "test:watch": "jest --watchAll",๐ 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:watch": "jeset --watchAll", | |
| "test:watch": "jest --watchAll", |
๐ค Prompt for AI Agents
In package.json around line 13, the "test:watch" script contains a typo ("jeset"
instead of "jest"); update the script value to use "jest --watchAll" so the
correct test runner is invoked when running npm run test:watch.
| 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.
๐ ๏ธ Refactor suggestion
Consider removing external site tests from CI.
Testing external sites like playwright.dev can cause CI failures due to network issues or site changes outside your control.
Consider moving external site tests to a separate file or removing them from the main test suite:
-test('has title', async ({ page }) => {
- await page.goto('https://playwright.dev/');
-
- // Expect a title "to contain" a substring.
- await expect(page).toHaveTitle(/Playwright/);
-});๐ 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/); | |
| }); |
๐ค Prompt for AI Agents
In tests/e2e/example.spec.ts around lines 3 to 8 there is a test that navigates
to an external site (https://playwright.dev/) which can cause flaky CI; remove
or move this external-site test out of the main CI suite by either deleting the
test, relocating it to a separate e2e-external.spec.ts file that is excluded
from CI, or marking it to skip in CI using test.skip or a custom tag/env-check;
alternatively replace the external navigation with a local/stubbed page (serve a
fixture HTML or use a mocked response) so the test no longer depends on the
external network.
| 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.
๐ ๏ธ Refactor suggestion
Remove external site test that may cause CI instability.
This test relies on an external site that could change or be unavailable, causing false CI failures.
-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();
-});๐ 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('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(); | |
| }); |
๐ค Prompt for AI Agents
In tests/e2e/example.spec.ts around lines 10 to 20, the test navigates to an
external site (playwright.dev) which can cause CI instability; remove or replace
the external-site usage by either deleting the test entirely or changing it to a
local deterministic scenario: use page.setContent(...) or serve a local static
HTML fixture containing the "Get started" link and "Installation" heading, click
the local link, and assert the heading is visible; ensure the test no longer
calls page.goto with an external URL and update any test name/comments
accordingly.
chamny20
added
๐ chaemin
There was a problem hiding this comment.
Actionable comments posted: 5
๐งน Nitpick comments (2)
tests/e2e/home.spec.ts (2)
24-31: Rely on auto-waiting assertions and prefer modern locator APIsThe explicit wait followed by toBeVisible is redundant. Playwrightโs expect auto-waits. Also prefer getByText (or getByRole/test IDs) over legacy "text=" selector for stability.
- await page - .locator('text=์๊ฐ์ ๋ด๋ ์๋ก์ด ๋ฐฉ๋ฒ') - .first() - .waitFor({ state: 'visible' }); - await expect( - page.locator('text=์๊ฐ์ ๋ด๋ ์๋ก์ด ๋ฐฉ๋ฒ').first(), - ).toBeVisible(); + const heroText = page.getByText('์๊ฐ์ ๋ด๋ ์๋ก์ด ๋ฐฉ๋ฒ', { exact: true }); + await expect(heroText).toBeVisible();
32-33: Prefer role-based selectors (or test IDs) over text-only locators for click targetsText-only locators are brittle when copy changes or multiple matches exist. Role-based targeting improves resilience and readability.
- await page.locator('text=ํ์ํ ์คํธ ์์ํ๊ธฐ').last().click(); + // ์ญํ ๊ธฐ๋ฐ ์ ํ์ ๊ถ์ฅ: ์ค์ ๋งํฌ์ ์ ๋ง์ถฐ 'button' ๋๋ 'link'๋ก ์กฐ์ ํ์ธ์. + await page.getByRole('button', { name: 'ํ์ํ ์คํธ ์์ํ๊ธฐ' }).click();If this element is a link (), change 'button' to 'link'.
๐ Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
๐ Files selected for processing (1)
tests/e2e/home.spec.ts(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: test
๐ Additional comments (1)
tests/e2e/home.spec.ts (1)
10-14: LGTM on the title assertionThe smoke assertion is simple and robust. With explicit navigation added, this test should be very reliable across browsers.
| test.beforeEach(async ({ page }) => { | ||
| // ๊ธฐ๋ณธ ํ ํ์ด์ง๋ก ์ด๋ | ||
| await page.goto('/'); | ||
| }); |
There was a problem hiding this comment.
๐ ๏ธ Refactor suggestion
Avoid navigating in beforeEach; navigate explicitly per test to prevent flakiness and reduce overhead
Preloading the page in beforeEach forces an extra navigation, and can race with token setup/clearing inside individual tests. Itโs safer and faster to navigate explicitly inside each test after youโve prepared storage state.
Apply this diff to remove the global navigation:
- test.beforeEach(async ({ page }) => {
- // ๊ธฐ๋ณธ ํ ํ์ด์ง๋ก ์ด๋
- await page.goto('/');
- });Then add explicit navigation inside each test as shown in the targeted diffs below.
๐ 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.beforeEach(async ({ page }) => { | |
| // ๊ธฐ๋ณธ ํ ํ์ด์ง๋ก ์ด๋ | |
| await page.goto('/'); | |
| }); |
๐ค Prompt for AI Agents
In tests/e2e/home.spec.ts around lines 5 to 8, remove the global await
page.goto('/') call from the test.beforeEach hook and instead call
page.goto('/') explicitly inside each individual test after any storage/token
setup or clearing is done; update every test in this file to perform its own
navigation at the point it knows the app state is ready to avoid races and extra
navigation overhead.
| test('ํ ํ์ด์ง๊ฐ ์ ์ ๋ก๋๋๊ณ ํ์ดํ์ TimeToast ํฌํจ', async ({ | ||
| page, | ||
| }) => { | ||
| await expect(page).toHaveTitle(/TimeToast/i); | ||
| }); |
There was a problem hiding this comment.
๐ ๏ธ Refactor suggestion
Add explicit navigation in the smoke test if removing beforeEach
If you remove the global navigation, ensure the smoke test navigates to the homepage first.
test('ํ ํ์ด์ง๊ฐ ์ ์ ๋ก๋๋๊ณ ํ์ดํ์ TimeToast ํฌํจ', async ({
page,
}) => {
+ await page.goto('/');
await expect(page).toHaveTitle(/TimeToast/i);
});๐ 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('ํ ํ์ด์ง๊ฐ ์ ์ ๋ก๋๋๊ณ ํ์ดํ์ TimeToast ํฌํจ', async ({ | |
| page, | |
| }) => { | |
| await expect(page).toHaveTitle(/TimeToast/i); | |
| }); | |
| test('ํ ํ์ด์ง๊ฐ ์ ์ ๋ก๋๋๊ณ ํ์ดํ์ TimeToast ํฌํจ', async ({ | |
| page, | |
| }) => { | |
| await page.goto('/'); | |
| await expect(page).toHaveTitle(/TimeToast/i); | |
| }); |
๐ค Prompt for AI Agents
In tests/e2e/home.spec.ts around lines 10 to 14, the test assumes a prior
navigation (removed beforeEach) so it only asserts the title; add an explicit
navigation step at the start of the test by awaiting page.goto('/') (or the
app's base URL) before the expect call so the home page is actually loaded prior
to checking the title.
| await page.evaluate(() => { | ||
| sessionStorage.clear(); | ||
| localStorage.clear(); | ||
| }); | ||
|
|
||
| await page.goto('/'); // reload ๋์ goto ๊ถ์ฅ | ||
|
|
There was a problem hiding this comment.
๐ ๏ธ Refactor suggestion
Initialize storage state before navigation with addInitScript (more reliable than clearing after load)
Using page.addInitScript ensures storage is set/cleared before any app code runs on first load. This avoids races and eliminates the need for the initial navigation in beforeEach.
- await page.evaluate(() => {
- sessionStorage.clear();
- localStorage.clear();
- });
-
- await page.goto('/'); // reload ๋์ goto ๊ถ์ฅ
+ await page.addInitScript(() => {
+ sessionStorage.clear();
+ localStorage.clear();
+ });
+ await page.goto('/');๐ 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.
| await page.evaluate(() => { | |
| sessionStorage.clear(); | |
| localStorage.clear(); | |
| }); | |
| await page.goto('/'); // reload ๋์ goto ๊ถ์ฅ | |
| await page.addInitScript(() => { | |
| sessionStorage.clear(); | |
| localStorage.clear(); | |
| }); | |
| await page.goto('/'); |
๐ค Prompt for AI Agents
In tests/e2e/home.spec.ts around lines 17 to 23, the test clears
sessionStorage/localStorage via page.evaluate after the page is created which
can race with app code; replace that with page.addInitScript(() => {
sessionStorage.clear(); localStorage.clear(); }); called before any navigation
(e.g., in the beforeEach that creates the page) and then remove the
post-creation page.evaluate and the extra initial goto so storage is reliably
cleared before the app loads.
| await page.evaluate(() => { | ||
| sessionStorage.setItem('accessToken', 'dummy-access-token'); | ||
| localStorage.setItem('refreshToken', 'dummy-refresh-token'); | ||
| }); |
There was a problem hiding this comment.
๐ ๏ธ Refactor suggestion
Set tokens before first navigation using addInitScript to avoid race conditions
Setting storage via addInitScript guarantees tokens exist before the appโs boot/redirect logic runs. This is more reliable than modifying storage on a loaded page.
- await page.evaluate(() => {
- sessionStorage.setItem('accessToken', 'dummy-access-token');
- localStorage.setItem('refreshToken', 'dummy-refresh-token');
- });
+ await page.addInitScript(() => {
+ sessionStorage.setItem('accessToken', 'dummy-access-token');
+ localStorage.setItem('refreshToken', 'dummy-refresh-token');
+ });๐ 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.
| await page.evaluate(() => { | |
| sessionStorage.setItem('accessToken', 'dummy-access-token'); | |
| localStorage.setItem('refreshToken', 'dummy-refresh-token'); | |
| }); | |
| await page.addInitScript(() => { | |
| sessionStorage.setItem('accessToken', 'dummy-access-token'); | |
| localStorage.setItem('refreshToken', 'dummy-refresh-token'); | |
| }); |
๐ค Prompt for AI Agents
In tests/e2e/home.spec.ts around lines 38 to 41, replace the runtime
page.evaluate storage writes with a page.addInitScript that sets
sessionStorage.setItem('accessToken', 'dummy-access-token') and
localStorage.setItem('refreshToken', 'dummy-refresh-token') so the tokens are in
place before the app boot/redirect logic runs; add the addInitScript call before
any page.goto/navigation so the script executes on initial document creation.
| await page.waitForURL(/\/home/, { timeout: 20000 }); | ||
| await page.waitForLoadState('networkidle'); | ||
| await expect(page).toHaveURL(/\/home/); |
There was a problem hiding this comment.
Remove networkidle wait; itโs flaky with SPAs and persistent connections
Waiting for 'networkidle' can hang or be flaky with apps that keep long-lived connections (e.g., Next.js, SSE, WS). Rely on URL assertions with an explicit timeout instead.
- await page.waitForURL(/\/home/, { timeout: 20000 });
- await page.waitForLoadState('networkidle');
- await expect(page).toHaveURL(/\/home/);
+ await expect(page).toHaveURL(/\/home/, { timeout: 20000 });๐ 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.
| await page.waitForURL(/\/home/, { timeout: 20000 }); | |
| await page.waitForLoadState('networkidle'); | |
| await expect(page).toHaveURL(/\/home/); | |
| await expect(page).toHaveURL(/\/home/, { timeout: 20000 }); |
๐ค Prompt for AI Agents
In tests/e2e/home.spec.ts around lines 44 to 46, remove the flaky await
page.waitForLoadState('networkidle') call; instead rely on the explicit URL
assertion (await page.waitForURL(/\/home/, { timeout: 20000 })) or increase its
timeout and/or add a stable DOM check (await
page.waitForSelector('selector-for-home', { timeout: X })) to ensure the SPA has
reached the expected state without using networkidle; delete the networkidle
line and either keep the existing waitForURL (optionally increase timeout) or
add a short waitForSelector for a deterministic element on the home page.
1 participant
๐์์ฝ(Summary)
์ด์ ๋ฒํธ : #187
๐จ๋ณ๊ฒฝ ์ฌํญ(Changes)
[์ฐ์ ์์]
๋น์ฆ๋์ค ํต์ฌ ๋ก์ง > ๋ ธ์ถ ๋น๋ > ์ด๋ฏธ์ง, ํํฐ ๋ฑ ์์ผ๋ก e2e test ์งํ
=> ํด๋น PR์์๋ ํํ์ด์ง ๋ก๋ ๋ถ๋ถ์ ๋ํด์๋ง ํ ์คํธ๋ฅผ ์งํ ํ checkout ํ main test ์งํํ ์์
๐๋ฆฌ๋ทฐ ์๊ตฌ์ฌํญ
๐ํ์ธ ๋ฐฉ๋ฒ (์ ํ)
๐ PR ์งํ ์ ์ด๋ฌํ ์ ๋ค์ ์ฐธ๊ณ ํด ์ฃผ์ธ์
Summary by CodeRabbit