chore(tests): fix playwright e2e tests#298
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis change restructures the Playwright end-to-end (E2E) test setup by moving test files into a new directory structure, updating configuration and workflow files, and adding a dedicated Docker Compose file for testing. It deletes old test files, introduces new or refactored test suites, and adjusts project settings, caching, and ignore rules to match the new organization. Changes
Possibly related PRs
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. 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (9)
tests/tsconfig.json (1)
1-6: Improve TypeScript compiler configuration for testsConsider enhancing the
tsconfig.jsonwith stricter type checking and proper module resolution, and by including necessary type definitions for Playwright. For example:"compilerOptions": { "baseUrl": ".", "lib": ["ES2022"], + "strict": true, + "moduleResolution": "node", + "types": ["node", "@playwright/test"] }tests/package.json (1)
1-7: Add a convenience script for running testsTo simplify test execution, add a
"scripts"section:{ "type": "module", "devDependencies": { "@playwright/test": "^1.52.0", "@types/node": "^22.15.21" }, + "scripts": { + "test": "playwright test" + } }.gitignore (1)
33-36: Refine ignore patterns for test artifactsUse anchored and recursive patterns to ensure complete coverage of test outputs:
- #tests data - tests/test-results/* - tests/.report/* - tests/.auth/* + # Playwright test artifacts + /tests/test-results/** + /tests/.report/** + /tests/.auth/**docker-compose.test.yml (1)
1-9: Add healthcheck to ensure service readiness before testsIntegrate a health check so Playwright waits for the app to be up:
services: arcane: # ... healthcheck: test: ["CMD", "curl", "-f", "http://localhost:8080/health"] interval: 5s timeout: 2s retries: 5tests/spec/converter.spec.ts (1)
18-21: Replace magic sleeps with deterministic waits
waitForTimeout(300)introduces flakiness and slows CI.
Wait for the section to expand, e.g.:await page.getByRole('region', { name: /Converter/i }).waitFor({ state: 'visible' });Apply the same pattern in the four places above.
Also applies to: 60-62, 100-102, 116-118
tests/spec/volumes.spec.ts (1)
98-98: Prefer regex literal- await expect(page).toHaveURL(new RegExp(`/volumes/.+`)); + await expect(page).toHaveURL(/\/volumes\/.+/);Avoid the
RegExpconstructor for readability and minor perf.🧰 Tools
🪛 Biome (1.9.4)
[error] 98-98: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.(lint/complexity/useRegexLiterals)
tests/spec/images.spec.ts (3)
21-33:page.gotois executed twice per test – drop the duplicate calls
beforeEachalready performspage.goto('/images').
Repeatingpage.goto('/images')at the top of every test adds ~200-300 ms per test and slightly increases flake risk. Consider removing the per-testgotoand only rely on thebeforeEachnavigation.
32-33: Replaceconsole.logwith Playwright’s built-in loggingUse
test.info().log()so messages are scoped to the test and show up only when a test fails or when--outputis requested.- console.log(`Found ${realImages.length} real images for testing`); + test.info().log(`Found ${realImages.length} real images for testing`);
155-166: Avoid duplicated string literals for image nameThe same image reference appears multiple times in this file. Extract it to a constant to ensure consistency when it inevitably changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
.github/workflows/e2e-tests.yml(1 hunks).gitignore(1 hunks)docker-compose.test.yml(1 hunks)frontend/.prettierrc(1 hunks)frontend/src/routes/+layout.svelte(1 hunks)tests/.auth/login.json(2 hunks)tests/auth.setup.ts(0 hunks)tests/converter.spec.ts(0 hunks)tests/images.spec.ts(0 hunks)tests/package.json(1 hunks)tests/playwright.config.ts(1 hunks)tests/setup/auth.setup.ts(1 hunks)tests/spec/converter.spec.ts(1 hunks)tests/spec/images.spec.ts(1 hunks)tests/spec/volumes.spec.ts(1 hunks)tests/tsconfig.json(1 hunks)tests/volumes.spec.ts(0 hunks)
💤 Files with no reviewable changes (4)
- tests/auth.setup.ts
- tests/converter.spec.ts
- tests/volumes.spec.ts
- tests/images.spec.ts
🧰 Additional context used
🪛 Gitleaks (8.26.0)
tests/.auth/login.json
5-5: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
🪛 Biome (1.9.4)
tests/spec/volumes.spec.ts
[error] 98-98: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
(lint/complexity/useRegexLiterals)
🔇 Additional comments (7)
frontend/.prettierrc (2)
2-6: Verify Prettier configuration updates.
The increasedprintWidthand removal of the Tailwind CSS plugin can significantly alter formatting. Ensure these changes are intentional and that droppingprettier-plugin-tailwindcsswon’t break class ordering for your Tailwind-based styles.
7-14: Prettier overrides look good.
The multi-line override for*.sveltewith thesvelteparser is clear and aligns with plugin usage.frontend/src/routes/+layout.svelte (2)
2-74: Skip formatting-only changes.
All modifications in these ranges are stylistic (indentation, quote style) without any behavioral changes.Also applies to: 76-106
75-75: Verify removal ofrichColorsprop from<Toaster />.
Confirm that the default color settings still meet UX requirements and that no test assertions rely on the removedrichColorsprop.tests/.auth/login.json (1)
23-28: Verify new localStorage entry is covered by testsYou’ve added
"collapsible-cards-expanded"to the auth state. Ensure existing tests expect and validate this key where necessary.tests/playwright.config.ts (1)
3-4: Ensure.authexists before tests run
storageState: authFileassumes.auth/login.jsoncan be written/read.
Confirm the directory is created in the setup script (see earlier comment) or add a pre-test hook here.tests/spec/images.spec.ts (1)
84-103: Pull-image test skips onrealImagesbut hard-codes an image nameIf there are zero images, the test is skipped, yet the locator later relies on the presence of
ghcr.io/linuxserver/nginxin the table – unrelated torealImages. Either:
- Look up the first row from
realImagesinstead of the hard-coded string, or- Remove the
realImages.lengthguard and rely solely on the hard-coded locator.Clarify the intent to avoid silent false negatives.
| - PUID=1000 | ||
| - PGID=1000 | ||
| - DATABASE_URL=sqlite:///app/data/arcane.db | ||
| - ENCRYPTION_KEY=3JDIgolks2tJ9ymm1AdqzlYMWu0DUWyt | ||
| - JWT_SECRET=your-super-secret-jwt-key-change-this-in-production | ||
| - APP_ENV=TEST |
There was a problem hiding this comment.
Avoid hardcoding secrets in test configuration
Hardcoded encryption keys and JWT secrets introduce security risk, even in test environments. Inject them via environment variables instead:
environment:
- - ENCRYPTION_KEY=3JDIgolks2tJ9ymm1AdqzlYMWu0DUWyt
- - JWT_SECRET=your-super-secret-jwt-key-change-this-in-production
+ - ENCRYPTION_KEY=${ENCRYPTION_KEY}
+ - JWT_SECRET=${JWT_SECRET}📝 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.
| - PUID=1000 | |
| - PGID=1000 | |
| - DATABASE_URL=sqlite:///app/data/arcane.db | |
| - ENCRYPTION_KEY=3JDIgolks2tJ9ymm1AdqzlYMWu0DUWyt | |
| - JWT_SECRET=your-super-secret-jwt-key-change-this-in-production | |
| - APP_ENV=TEST | |
| - PUID=1000 | |
| - PGID=1000 | |
| - DATABASE_URL=sqlite:///app/data/arcane.db | |
| - - ENCRYPTION_KEY=3JDIgolks2tJ9ymm1AdqzlYMWu0DUWyt | |
| - - JWT_SECRET=your-super-secret-jwt-key-change-this-in-production | |
| + - ENCRYPTION_KEY=${ENCRYPTION_KEY} | |
| + - JWT_SECRET=${JWT_SECRET} | |
| - APP_ENV=TEST |
🤖 Prompt for AI Agents
In docker-compose.test.yml around lines 13 to 18, the encryption key and JWT
secret are hardcoded, which poses a security risk. Remove the hardcoded values
for ENCRYPTION_KEY and JWT_SECRET and modify the configuration to read these
values from environment variables instead, allowing secure injection of secrets
during test runs.
tests/spec/images.spec.ts
Outdated
| const deleteableImage = realImages.find((img) => !img.inUse); | ||
| test.skip(deleteableImage, 'No deletable images available'); | ||
|
|
There was a problem hiding this comment.
Incorrect test.skip condition prevents the test from ever running
test.skip expects a boolean. Passing the object itself will always evaluate as truthy and skip even when a deletable image exists.
- const deleteableImage = realImages.find((img) => !img.inUse);
- test.skip(deleteableImage, 'No deletable images available');
+ const deleteableImage = realImages.find((img) => !img.inUse);
+ test.skip(!deleteableImage, 'No deletable images available');📝 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.
| const deleteableImage = realImages.find((img) => !img.inUse); | |
| test.skip(deleteableImage, 'No deletable images available'); | |
| const deleteableImage = realImages.find((img) => !img.inUse); | |
| test.skip(!deleteableImage, 'No deletable images available'); |
🤖 Prompt for AI Agents
In tests/spec/images.spec.ts around lines 111 to 113, the test.skip call
incorrectly passes an object instead of a boolean, causing the test to always
skip. Change the condition to pass a boolean by negating the existence of
deleteableImage, so the test only skips when no deletable images are available.
| async function fetchImagesWithRetry(page: Page, maxRetries = 3): Promise<any[]> { | ||
| let retries = 0; | ||
| while (retries < maxRetries) { | ||
| try { | ||
| const response = await page.request.get('/api/images'); | ||
| const images = await response.json(); | ||
| return images.data; | ||
| } catch (error) { | ||
| retries++; | ||
| if (retries >= maxRetries) throw error; | ||
| await new Promise((resolve) => setTimeout(resolve, 1000)); | ||
| } | ||
| } | ||
| return []; | ||
| } |
There was a problem hiding this comment.
Harden fetchImagesWithRetry against non-200 responses and malformed JSON
response.json() will throw when the server returns non-JSON or a non-2xx status code, causing an early, unrecoverable failure that the retry loop never catches. Add an explicit status check and safeguard the JSON parsing.
- const response = await page.request.get('/api/images');
- const images = await response.json();
- return images.data;
+ const response = await page.request.get('/api/images');
+ if (!response.ok()) {
+ throw new Error(`GET /api/images returned ${response.status()}`);
+ }
+ const body = await response.json().catch(() => ({}));
+ return Array.isArray(body.data) ? body.data : [];📝 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.
| async function fetchImagesWithRetry(page: Page, maxRetries = 3): Promise<any[]> { | |
| let retries = 0; | |
| while (retries < maxRetries) { | |
| try { | |
| const response = await page.request.get('/api/images'); | |
| const images = await response.json(); | |
| return images.data; | |
| } catch (error) { | |
| retries++; | |
| if (retries >= maxRetries) throw error; | |
| await new Promise((resolve) => setTimeout(resolve, 1000)); | |
| } | |
| } | |
| return []; | |
| } | |
| async function fetchImagesWithRetry(page: Page, maxRetries = 3): Promise<any[]> { | |
| let retries = 0; | |
| while (retries < maxRetries) { | |
| try { | |
| const response = await page.request.get('/api/images'); | |
| if (!response.ok()) { | |
| throw new Error(`GET /api/images returned ${response.status()}`); | |
| } | |
| const body = await response.json().catch(() => ({})); | |
| return Array.isArray(body.data) ? body.data : []; | |
| } catch (error) { | |
| retries++; | |
| if (retries >= maxRetries) throw error; | |
| await new Promise((resolve) => setTimeout(resolve, 1000)); | |
| } | |
| } | |
| return []; | |
| } |
🤖 Prompt for AI Agents
In tests/spec/images.spec.ts around lines 3 to 17, the fetchImagesWithRetry
function does not handle non-200 HTTP responses or malformed JSON, causing
unhandled exceptions that bypass retries. Modify the function to first check if
the response status is 200 before attempting to parse JSON, and wrap the JSON
parsing in a try-catch block to handle parsing errors gracefully, allowing the
retry logic to proceed correctly.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Style
Tests
Chores
.gitignorerules to better target test-related data files.