ci(web): set up playwright #1342
Conversation
WalkthroughThe changes involve updates to the CI workflow configuration in Changes
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
✅ Deploy Preview for reearth-cms ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
web/playwright.config.ts (1)
Line range hint
6-19: Consider adding additional Playwright configurations.The current configuration could benefit from some additional settings:
- Timeout configurations to prevent hanging tests
- Browser configurations to ensure consistent testing environments
Consider adding these configurations:
const config: PlaywrightTestConfig = { workers: process.env.CI ? 1 : undefined, retries: 2, + timeout: 30000, + expect: { + timeout: 5000, + }, use: { baseURL: process.env.REEARTH_CMS_E2E_BASEURL || "http://localhost:3000/", screenshot: "only-on-failure", video: "retain-on-failure", locale: "en-US", + viewport: { width: 1280, height: 720 }, + actionTimeout: 15000, },.github/workflows/ci_web.yml (1)
30-31: Consider caching Playwright browsers.Installing browsers on every run can be time-consuming. Consider caching them to speed up the CI pipeline.
Add caching for Playwright browsers:
- name: Install Playwright Browsers + if: steps.playwright-cache.outputs.cache-hit != 'true' run: yarn playwright install --with-deps + - name: Cache Playwright browsers + id: playwright-cache + uses: actions/cache@v3 + with: + path: ~/.cache/ms-playwright + key: playwright-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci_web.yml(1 hunks)web/playwright.config.ts(1 hunks)
🔇 Additional comments (2)
web/playwright.config.ts (1)
7-7: LGTM! Setting workers=1 in CI is a good practice.
This helps prevent resource contention and ensures stable test execution in CI environments.
.github/workflows/ci_web.yml (1)
35-41: Verify the environment variables configuration.
Let's verify that all required environment variables are properly configured in the repository secrets.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
web/e2e/utils/config.ts (1)
1-7: Consider adding type safety for environment variablesThe config object could benefit from runtime validation of required environment variables.
+import { z } from 'zod'; +const configSchema = z.object({ + api: z.string(), + baseUrl: z.string(), + userName: z.string(), + password: z.string(), +}); -export const config = { +export const config = configSchema.parse({ api: process.env["REEARTH_CMS_API"], baseUrl: process.env["REEARTH_CMS_E2E_BASEURL"], userName: process.env["REEARTH_CMS_E2E_USERNAME"], password: process.env["REEARTH_CMS_E2E_PASSWORD"], -}; +});web/playwright.config.ts (1)
14-14: Consider CI parallelization strategyWhile setting
workers: 1in CI is a safe start, consider enabling parallelization once the test suite grows.You can later optimize CI performance by:
- Using Playwright's sharding capability
- Implementing test isolation
- Utilizing matrix strategy in GitHub Actions
web/e2e/auth.setup.ts (2)
5-20: Enhance error handling and test stabilityWhile the authentication test is well-structured, consider these improvements:
test("authenticate", async ({ page }) => { const baseUrl = process.env.REEARTH_CMS_E2E_BASEURL; const username = process.env.REEARTH_CMS_E2E_USERNAME; const password = process.env.REEARTH_CMS_E2E_PASSWORD; - expect(baseUrl).not.toBe(undefined); - expect(username).not.toBe(undefined); - expect(password).not.toBe(undefined); + if (!baseUrl || !username || !password) { + throw new Error('Required environment variables are not set'); + } - await page.goto(baseUrl as string); + await page.goto(baseUrl); + + // Wait for login form to be ready + await page.waitForSelector('input[placeholder="username/email"]'); + await page.getByPlaceholder("username/email").fill(username as string); await page.getByPlaceholder("your password").fill(password as string); + + // Add error handling for failed login + const loginPromise = Promise.all([ + page.waitForResponse(response => response.url().includes('/login') && response.status() === 200), + page.getByText("LOG IN").click() + ]); - await page.getByText("LOG IN").click(); + await loginPromise; await expect(page.getByRole("button", { name: "New Project" })).toBeVisible({ timeout: 10 * 1000, }); await page.context().storageState({ path: authFile }); });
16-18: Consider adjusting the timeout based on CI environmentThe 10-second timeout might need adjustment in CI environments where performance can be slower.
+const TIMEOUT = process.env.CI ? 30_000 : 10_000; await expect(page.getByRole("button", { name: "New Project" })).toBeVisible({ - timeout: 10 * 1000, + timeout: TIMEOUT, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/ci_web.yml(1 hunks)web/.gitignore(1 hunks)web/e2e/auth.setup.ts(1 hunks)web/e2e/dashboard.spec.ts(0 hunks)web/e2e/utils/config.ts(1 hunks)web/e2e/utils/login.ts(0 hunks)web/e2e/utils/setup.ts(0 hunks)web/playwright.config.ts(1 hunks)
💤 Files with no reviewable changes (3)
- web/e2e/dashboard.spec.ts
- web/e2e/utils/setup.ts
- web/e2e/utils/login.ts
✅ Files skipped from review due to trivial changes (1)
- web/.gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci_web.yml
🔇 Additional comments (2)
web/e2e/utils/config.ts (1)
1-7: Verify the impact of removed auth configuration
The removal of auth-related configuration (workspaceId, authAudience, authClientId, authUrl) suggests a shift to a simpler authentication approach. Please ensure this doesn't affect any existing API calls or authentication flows.
✅ Verification successful
Removal of auth config from E2E tests is correct
The removal of auth-related configuration from web/e2e/utils/config.ts is appropriate as the E2E tests now use a simpler authentication approach with just username/password via Playwright's built-in auth handling (auth.setup.ts). The removed Auth0 configurations are still present in the main application (web/src/config/index.ts and web/src/env.d.ts) where they are actually needed, not in E2E tests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of removed auth config
rg -l "REEARTH_CMS_AUTH0_" --type ts
rg -l "REEARTH_CMS_E2E_WORKSPACE_ID" --type ts
Length of output: 124
Script:
#!/bin/bash
# Check the content of files that still use Auth0 config
echo "=== web/src/env.d.ts ==="
cat web/src/env.d.ts
echo -e "\n=== web/src/config/index.ts ==="
cat web/src/config/index.ts
# Search for any auth-related function usage in e2e tests
echo -e "\n=== Auth related function usage in e2e tests ==="
rg -A 5 "auth" web/e2e/
Length of output: 4212
web/playwright.config.ts (1)
25-35: Well-structured project configuration
The project configuration is well organized with:
- Separate setup phase for authentication
- Proper test dependencies
- Correct storage state management
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
web/playwright.config.ts (1)
14-14: LGTM: Appropriate worker configuration for CI/local environmentsSetting workers to 1 in CI prevents resource contention, while allowing optimal parallelization in local development.
Consider adding a comment explaining the rationale for single-worker CI configuration to help future maintainers understand this decision.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci_web.yml(1 hunks)web/e2e/auth.setup.ts(1 hunks)web/playwright.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci_web.yml
- web/e2e/auth.setup.ts
🔇 Additional comments (3)
web/playwright.config.ts (3)
1-10: LGTM: Path handling setup follows ESM best practices
The implementation correctly uses fileURLToPath and dirname for ESM-compatible path resolution.
11-11: Verify .gitignore settings for auth file
The auth file likely contains sensitive authentication state. Let's ensure it's properly excluded from version control.
✅ Verification successful
Auth file directory is properly gitignored
The /e2e/utils/.auth directory is explicitly excluded in the web/.gitignore file, which ensures that authentication state files won't be committed to version control.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if auth file directory is properly gitignored
cat .gitignore | grep -q "\.auth" || echo "Warning: .auth directory not found in .gitignore"
# Check if any auth files are already tracked
git ls-files | grep -q "\.auth/user\.json" && echo "Warning: auth file is tracked in git"
Length of output: 187
Script:
#!/bin/bash
# Let's check the actual content of .gitignore to see all patterns
echo "=== .gitignore contents ==="
cat .gitignore
# Also check if there are any other gitignore patterns in the web directory
echo -e "\n=== web/.gitignore contents (if exists) ==="
test -f web/.gitignore && cat web/.gitignore || echo "No web/.gitignore found"
# Check if the auth directory or any similar patterns exist
echo -e "\n=== Files matching auth patterns ==="
fd -H "auth|user\.json" -t f
Length of output: 1959
25-35: Verify setup file implementation
The configuration references setup files (*.setup.ts) which are crucial for test initialization. Let's verify their implementation.
✅ Verification successful
Let me check the content of the auth.setup.ts file to verify its implementation.
Setup file implementation is properly configured and functional ✅
The auth.setup.ts file is correctly implemented with proper authentication flow:
- Uses environment variables for credentials and base URL
- Implements login flow with proper assertions
- Saves authentication state to the configured storage file
- Matches the configuration in playwright.config.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existence of setup files
fd -e ts -p ".*\.setup\.ts$" web/e2e/
# Look for authentication setup patterns in setup files
ast-grep --pattern 'async function setup() {
$$$
await page.goto($_);
$$$
await page.fill($_, $_);
$$$
}'
Length of output: 179
Script:
#!/bin/bash
# Display the content of auth.setup.ts
cat web/e2e/auth.setup.ts
Length of output: 908
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
web/playwright.config.ts (2)
19-24: Consider adding retry delay for better test stability.While the current configuration is good, adding a retry delay can help with test stability, especially in CI environments.
use: { baseURL, screenshot: "only-on-failure", video: process.env.CI ? "on-first-retry" : "retain-on-failure", locale: "en-US", + retryDelay: 300, },
27-37: Consider adding viewport configuration for consistency.The projects configuration is well-structured, but adding a fixed viewport size would ensure consistent test behavior across different environments.
use: { ...devices["Desktop Chrome"], storageState: authFile, + viewport: { width: 1280, height: 720 }, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci_web.yml(2 hunks)web/e2e/auth.setup.ts(1 hunks)web/playwright.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/e2e/auth.setup.ts
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/ci_web.yml
50-50: key "if" is duplicated in element of "steps" section. previously defined at line:45,col:9
(syntax-check)
51-51: key "with" is duplicated in element of "steps" section. previously defined at line:46,col:9
(syntax-check)
🪛 yamllint (1.35.1)
.github/workflows/ci_web.yml
[error] 50-50: duplication of key "if" in mapping
(key-duplicates)
[error] 51-51: duplication of key "with" in mapping
(key-duplicates)
🔇 Additional comments (3)
web/playwright.config.ts (2)
1-13: LGTM! Path handling and configuration setup looks good.
The implementation properly handles cross-platform path resolution and includes a fallback for baseURL.
16-16: LGTM! Appropriate worker configuration for CI/local environments.
Setting workers to 1 in CI prevents resource contention, while allowing optimal parallelization locally.
.github/workflows/ci_web.yml (1)
30-38: 🛠️ Refactor suggestion
Add error handling and test artifacts collection.
While the basic setup is correct, we should improve error handling and collect test artifacts for debugging failures.
- name: Run Playwright tests
run: yarn e2e
+ continue-on-error: true
+ id: playwright-test
env:
REEARTH_CMS_API: https://api.cms.test.reearth.dev/api
REEARTH_CMS_E2E_BASEURL: https://cms.test.reearth.dev
REEARTH_CMS_E2E_USERNAME: ${{ secrets.REEARTH_WEB_E2E_USERNAME }}
REEARTH_CMS_E2E_PASSWORD: ${{ secrets.REEARTH_WEB_E2E_PASSWORD }}
+ - name: Upload test artifacts
+ if: steps.playwright-test.outcome == 'failure'
+ uses: actions/upload-artifact@v4
+ with:
+ name: playwright-report
+ path: |
+ web/playwright-report/
+ web/test-results/
+ retention-days: 7
+ - name: Exit on test failure
+ if: steps.playwright-test.outcome == 'failure'
+ run: exit 1Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
.github/workflows/ci_web.yml (1)
34-42: Enhance Playwright test configurationConsider these improvements:
- Install additional browsers (Firefox, WebKit) for cross-browser testing
- Make API and base URLs configurable via secrets/variables
- Add retry mechanism for potentially flaky tests
- name: Install Playwright Browsers - run: yarn playwright install --with-deps chromium + run: yarn playwright install --with-deps - name: Run Playwright tests run: yarn e2e + retries: 2 env: - REEARTH_CMS_API: https://api.cms.test.reearth.dev/api - REEARTH_CMS_E2E_BASEURL: https://cms.test.reearth.dev + REEARTH_CMS_API: ${{ secrets.REEARTH_CMS_API_URL }} + REEARTH_CMS_E2E_BASEURL: ${{ secrets.REEARTH_CMS_BASE_URL }} REEARTH_CMS_E2E_USERNAME: "${{ secrets.REEARTH_WEB_E2E_USERNAME }}" REEARTH_CMS_E2E_PASSWORD: "${{ secrets.REEARTH_WEB_E2E_PASSWORD }}"web/e2e/utils/config.ts (1)
Line range hint
10-20: Consider using Playwright's built-in storage state for authWhile the current token management functions work, Playwright provides built-in mechanisms for handling authentication state. Consider refactoring to use Playwright's
storageStatefeature for better integration.Example implementation:
// In your test file or auth.setup.ts await page.context().storageState({ path: './auth.json' }); // In playwright.config.ts use: { storageState: './auth.json' }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci_web.yml(1 hunks)web/e2e/utils/config.ts(1 hunks)
🔇 Additional comments (4)
.github/workflows/ci_web.yml (2)
43-48: Enhance test artifacts configuration
Building on the previous review comment, consider these additional improvements:
- Increase retention period to at least 7 days for better debugging window
- Include screenshots and traces in addition to test results
- Add step to upload test report even on success for historical tracking
- uses: actions/upload-artifact@v4
if: failure()
with:
name: test-results
- path: web/test-results/
- retention-days: 1
+ path: |
+ web/test-results/
+ web/playwright-report/
+ web/test-results/**/*.png
+ web/test-results/**/*.webm
+ retention-days: 720-29:
Verify the removal of core quality checks
Several important quality checks have been commented out:
- Linting
- Unit tests and coverage reporting
- Translation checks
While adding Playwright tests is valuable, these core checks should typically remain active. Please clarify if this is intentional and if there's a plan to reinstate these checks.
web/e2e/utils/config.ts (2)
1-3: Verify .env file handling in CI environment
The dotenv setup looks good, but we should ensure proper environment variable handling in CI.
✅ Verification successful
Environment variable setup is properly configured for CI/CD
The dotenv setup is correctly implemented with proper handling of environment variables:
.env.examplefile exists in the web directory for local development- CI workflow (
ci_web.yml) explicitly sets all required environment variables - E2E test credentials are securely managed through GitHub secrets
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for .env related files and CI workflow configuration
# Check if .env example file exists
fd -t f "^\.env" -x echo "Found .env related file: {}"
# Check if CI workflow handles environment variables
rg -l "REEARTH_CMS" .github/workflows/
Length of output: 438
Script:
#!/bin/bash
# Check CI workflow content for environment variable handling
rg -A 5 "env:" .github/workflows/ci_web.yml
# Check for .env files including hidden ones
fd -H -t f "^\.env" -x echo "Found .env related file: {}"
# Look for environment variable setup in e2e test configuration or documentation
rg -l "env" web/e2e/
Length of output: 624
6-8: Verify the new authentication flow implementation
The removal of auth-related properties (authUrl, authClientId, etc.) suggests a change in the authentication approach. Let's verify the new auth implementation.
✅ Verification successful
Let me check the content of the auth setup file to verify the new authentication implementation.
Let me check how this auth setup is being used in the test configuration.
Let me check the correct path for the playwright config file.
Let me check the content of the playwright config file to see how the auth setup is configured.
Authentication implementation has been properly migrated to Playwright's recommended approach
The removal of auth-related properties from the config file is justified as the authentication has been properly implemented using Playwright's best practices:
- A dedicated
auth.setup.tshandles the authentication flow and stores the auth state - The Playwright config properly configures auth state sharing across test files using
storageState - All test files will automatically receive the authenticated state through the config's project dependencies
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for new auth implementation files and usage
# Look for new auth-related files
rg -l "auth" web/e2e/
# Check for Playwright-specific auth implementations
ast-grep --pattern 'test.use({ storageState:'
# Look for auth setup files
fd -t f "auth.setup" web/e2e/
Length of output: 143
Script:
#!/bin/bash
# Check the content of auth.setup.ts
cat web/e2e/auth.setup.ts
# Look for usage of auth setup in test files
rg -l "auth.setup" web/e2e/
Length of output: 785
Script:
#!/bin/bash
# Check the playwright config file for auth setup integration
cat ../playwright.config.ts
# Look for test files that might be using the auth setup
rg -l "storageState" web/e2e/
Length of output: 137
Script:
#!/bin/bash
# Find the playwright config file
fd playwright.config
# Check for any test files that might be importing the auth setup
rg -l "import.*auth\.setup" web/e2e/
Length of output: 86
Script:
#!/bin/bash
# Check the content of the playwright config
cat web/playwright.config.ts
# Look for test files in the e2e directory to see test setup
rg -l "test\(" web/e2e/ --type ts
Length of output: 2562
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.github/workflows/ci_web.yml (1)
32-33: Consider testing with multiple browsersCurrently only installing Chromium. Consider adding Firefox and Webkit for better cross-browser coverage.
- run: yarn playwright install --with-deps chromium + run: yarn playwright install --with-deps
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci_web.yml(1 hunks)
🔇 Additional comments (3)
.github/workflows/ci_web.yml (3)
30-31:
Remove debug step exposing secrets
Echoing secrets in CI logs, even if obfuscated, is a security risk. This appears to be a temporary debug step that should be removed.
- - name: test secrets
- run: echo "${{secrets.REEARTH_WEB_E2E_USERNAME }}"41-46: 🛠️ Refactor suggestion
Improve test artifact handling
The current artifact configuration needs improvements:
- Consider longer retention for debugging
- Include playwright-report directory
- Add explicit failure step
- uses: actions/upload-artifact@v4
if: failure()
with:
name: test-results
- path: web/test-results/
- retention-days: 1
+ path: |
+ web/test-results/
+ web/playwright-report/
+ retention-days: 30
+ - name: Exit if tests failed
+ if: steps.playwright-tests.outcome == 'failure'
+ run: exit 134-40: 🛠️ Refactor suggestion
Add error handling and verify environment configuration
The test step needs better error handling and environment validation.
- name: Run Playwright tests
- run: yarn e2e
+ run: yarn e2e
+ continue-on-error: true
+ id: playwright-tests
env:
REEARTH_CMS_API: https://api.cms.test.reearth.dev/api
REEARTH_CMS_E2E_BASEURL: https://cms.test.reearth.dev
REEARTH_CMS_E2E_USERNAME: "${{ secrets.REEARTH_WEB_E2E_USERNAME }}"
REEARTH_CMS_E2E_PASSWORD: "${{ secrets.REEARTH_WEB_E2E_PASSWORD }}"Let's verify the environment configuration:
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci.yml(1 hunks).github/workflows/ci_web.yml(2 hunks)web/e2e/utils/config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci_web.yml
- web/e2e/utils/config.ts
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
50-52: LGTM! Secure handling of E2E test credentials.
The secrets for Playwright E2E testing are properly configured using GitHub's secrets context and are only passed to the required workflow.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (12)
web/e2e/project/request.spec.ts (1)
Line range hint
142-161: Consider improving test maintainability.The test case "Navigating between item and request has succeeded" handles multiple concerns and could benefit from:
- Adding descriptive comments to explain the test flow and assertions
- Breaking down into smaller, focused test cases (e.g., separate tests for navigation and field updates)
- Making prerequisites more explicit within the test case instead of relying heavily on the global setup
Example refactor:
test.describe('Item and Request Navigation', () => { test('should preserve item state when navigating from request', async ({ page }) => { // Setup: Create request with item await page.getByRole('button', { name: requestTitle }).first().click(); // Action: Navigate to item await page.getByRole('button', { name: itemTitle }).last().click(); // Assert: Item state is preserved await expect(page.getByLabel(`${titleFieldName}Title`)).not.toBeEmpty(); }); test('should handle item updates after navigation', async ({ page }) => { // ... split the field update logic into a separate test }); });web/e2e/project/accessibility.spec.ts (2)
19-19: Good defensive check, but could be more descriptiveThe empty check is a good practice to prevent flaky tests. Consider adding a custom error message to make debugging easier.
- await expect(page.getByRole("textbox")).not.toBeEmpty(); + await expect(page.getByRole("textbox"), "Alias textbox should not be empty").not.toBeEmpty();
18-18: Fix typo in test descriptionThere's a typo in "Accesibility" (should be "Accessibility").
-test("Update settings on Accesibility page has succeeded", async ({ page }) => { +test("Update settings on Accessibility page has succeeded", async ({ page }) => {web/e2e/project/item/fields/asset.spec.ts (2)
165-167: Consider consolidating tooltip assertionsWhile the assertions are correct, they could be made more robust and maintainable.
Consider consolidating the assertions using Playwright's more specific selectors and a single expect block:
- await expect(page.getByRole("tooltip")).toContainText(`new asset1`); - await expect(page.getByRole("tooltip").locator("p").first()).toContainText(uploadFileName_2); - await expect(page.getByRole("tooltip").locator("p").last()).toContainText(uploadFileName_1); + const tooltip = page.getByRole("tooltip"); + await expect(tooltip).toHaveText(new RegExp(`.*${uploadFileName_2}.*${uploadFileName_1}.*`));This approach:
- Reduces the number of assertions
- Makes the test less brittle to UI structure changes
- Still verifies the presence of both filenames in the correct order
Line range hint
1-167: Consider refactoring test structure for better maintainabilityWhile the tests are functional and well-written, there are opportunities for improvement in the test architecture.
Consider:
- Extracting common steps into helper functions, such as:
- Asset upload steps
- Field creation steps
- Verification steps
- Using test.describe() blocks to group related tests
- Using beforeEach() to set up common test state
Example structure:
// Helper functions async function uploadAsset(page: Page, url: string) { await page.getByRole("button", { name: "upload Upload Asset" }).click(); await page.getByRole("tab", { name: "URL" }).click(); await page.getByPlaceholder("Please input a valid URL").fill(url); await page.getByRole("button", { name: "Upload and Link" }).click(); await closeNotification(page); } test.describe('Asset Field', () => { test.beforeEach(async ({ page }) => { // Common setup }); test('creating and updating', async ({ page }) => { // Shorter, more focused test }); test('editing', async ({ page }) => { // Shorter, more focused test }); });This would:
- Reduce code duplication
- Make tests easier to maintain
- Improve readability
- Make test failures easier to debug
web/e2e/project/item/metadata/boolean.spec.ts (1)
70-70: Consider breaking down the complex test caseWhile marking the test as slow is appropriate, consider breaking it down into smaller, more focused test cases. This would improve maintainability and make it easier to identify failures.
web/e2e/project/item/metadata/url.spec.ts (2)
75-75: Consider breaking down the complex test caseSimilar to the boolean tests, consider breaking this large test into smaller, more focused test cases for better maintainability.
Line range hint
1-1: Consider creating shared test utilitiesGiven the common patterns across test files, consider:
- Creating a custom test fixture for handling common wait conditions
- Implementing page object models to encapsulate UI interactions
- Breaking down large tests into smaller, more focused test cases using
describeblocksThis would improve maintainability and reduce code duplication.
web/e2e/project/item/metadata/checkbox.spec.ts (1)
73-73: Document performance characteristics with test.slow()Good practice marking slower tests with test.slow(). Consider adding a comment explaining why this test needs more time to complete.
+ // This test is marked as slow due to multiple checkbox interactions and UI updates test.slow();web/e2e/project/item/metadata/tag.spec.ts (1)
75-76: Standardize wait strategy across test filesThis file shows the same timeout pattern seen in other test files, indicating a systemic issue with handling UI updates.
Consider implementing a global solution:
- Create a custom test fixture that handles common wait patterns
- Add data attributes for loading states to components
- Use these attributes in the fixture to determine when the UI is ready
This would eliminate the need for timeouts across all test files and make the tests more reliable.
web/e2e/project/item/metadata/date.spec.ts (2)
78-78: Consider breaking down the large test caseThe need for
test.slow()suggests this test case might be doing too much. Consider:
- Breaking it into smaller, focused test cases
- Moving common setup steps to beforeEach hooks
- Reducing redundant assertions
This will improve test maintenance and debugging while potentially eliminating the need for extended timeouts.
178-180: Extract repeated date input logic into a helper functionThe pattern of clicking, filling, and pressing Enter on date inputs is repeated throughout the test. Consider extracting this into a helper function:
async function fillDateInput(page: Page, index: number, value: string) { const textbox = page.getByRole("textbox").nth(index); await textbox.click(); await textbox.fill(value); await textbox.press("Enter"); }This would make the test more maintainable and reduce duplication:
- await page.getByRole("textbox").nth(4).click(); - await page.getByRole("textbox").nth(4).fill("2024-01-07"); - await page.getByRole("textbox").nth(4).press("Enter"); + await fillDateInput(page, 4, "2024-01-07");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
web/e2e/project/accessibility.spec.ts(3 hunks)web/e2e/project/item/fields/asset.spec.ts(3 hunks)web/e2e/project/item/metadata/boolean.spec.ts(3 hunks)web/e2e/project/item/metadata/checkbox.spec.ts(3 hunks)web/e2e/project/item/metadata/date.spec.ts(3 hunks)web/e2e/project/item/metadata/tag.spec.ts(6 hunks)web/e2e/project/item/metadata/text.spec.ts(3 hunks)web/e2e/project/item/metadata/url.spec.ts(3 hunks)web/e2e/project/request.spec.ts(1 hunks)web/e2e/settings/settings.spec.ts(2 hunks)
🔇 Additional comments (6)
web/e2e/project/request.spec.ts (1)
143-143: LGTM! The new assertion improves test coverage.
The added check verifies that the title field is not empty after navigation, strengthening the test case by ensuring proper state preservation.
web/e2e/project/accessibility.spec.ts (2)
4-5: LGTM: Good practice using configuration import
The addition of the config import helps remove hardcoded values and makes the tests more maintainable.
30-30: LGTM: Good use of dynamic configuration
Using config.api instead of a hardcoded URL makes the tests more maintainable and environment-independent, which is crucial for CI/CD pipelines.
web/e2e/project/item/fields/asset.spec.ts (2)
54-54: LGTM: Consistent use of filename constant
The assertion correctly uses the defined constant uploadFileName_1 instead of a hardcoded value, maintaining consistency and improving maintainability.
70-70: LGTM: Consistent use of filename constant
The assertion correctly uses the defined constant uploadFileName_2, following the same pattern as the previous assertion.
web/e2e/project/item/metadata/url.spec.ts (1)
159-160: Good use of role-based selectors
The use of role-based selectors (getByRole) is a best practice as it makes tests more resilient to UI changes and follows accessibility standards.
Overview
This PR set up playwright for ci.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
.gitignoreto exclude specific authentication files.