test: playwright boilerplate #65
Conversation
WalkthroughThe changes in this pull request encompass modifications to several files, primarily focusing on enhancing end-to-end testing capabilities using Playwright. Key updates include the addition of a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AuthSetup
participant GitHub
participant BankFeature
User->>AuthSetup: Start Authentication
AuthSetup->>GitHub: Sign in with GitHub
GitHub-->>AuthSetup: Authentication Response
AuthSetup->>User: Authentication Successful
User->>BankFeature: Initiate MFX Transaction
BankFeature-->>User: Transaction Confirmation
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #65 +/- ##
=======================================
Coverage 60.37% 60.37%
=======================================
Files 148 148
Lines 14757 14757
=======================================
Hits 8909 8909
Misses 5848 5848 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (11)
e2e/landing.spec.ts (2)
3-6: Consider using exact title match instead of regexThe regex pattern
/Alberto - Blockchain App/might be too permissive and could pass with unexpected titles. Consider using an exact match for more reliable testing.- await expect(page).toHaveTitle(/Alberto - Blockchain App/); + await expect(page).toHaveTitle('Alberto - Blockchain App');
1-12: Improve test organization and reusabilityConsider restructuring the tests using
test.describefor better organization andbeforeEachfor common setup.import { test, expect } from '@playwright/test'; test.describe('Landing Page', () => { let page; test.beforeEach(async ({ page: p }) => { page = p; await page.goto('/'); }); test('has correct title', async () => { await expect(page).toHaveTitle('Alberto - Blockchain App'); }); test('learn how it works button', async () => { const button = page.getByTestId('learn-more-button'); await expect(button).toBeVisible(); await button.click(); // Add assertions for expected behavior after click }); });.gitignore (1)
39-39: Remove duplicate node_modules entryThe
node_modules/entry is already covered by/node_modulesat the top of the file.-node_modules/e2e/bank.spec.ts (1)
2-2: Remove unused importThe
signInWithGitHubutility is imported but never used in the test.-import { signInWithGitHub } from '@/e2e/utils/auth';playwright.config.ts (2)
14-33: Consider adding timeout settings and verify baseURL.While the configuration is good, consider:
- Adding global timeout settings for CI environments where tests might run slower
- Making baseURL configurable via environment variables for different environments
use: { - baseURL: 'http://127.0.0.1:3000', + baseURL: process.env.PLAYWRIGHT_BASE_URL || 'http://127.0.0.1:3000', trace: 'on-first-retry', + // Add timeout settings + navigationTimeout: 30000, + actionTimeout: 15000, },
35-78: Consider enabling Firefox for cross-browser testing.Currently, only Chromium is enabled. While this is a good start, consider enabling Firefox as well for better cross-browser coverage. Firefox often helps catch CSS and JavaScript compatibility issues early.
projects: [ { name: 'setup', testMatch: /.*\.setup\.ts/ }, { name: 'chromium', use: { ...devices['Desktop Chrome'], storageState: 'playwright/.auth/user.json' }, dependencies: ['setup'], }, - // { - // name: 'firefox', - // use: { ...devices['Desktop Firefox'] }, - // }, + { + name: 'firefox', + use: { ...devices['Desktop Firefox'], storageState: 'playwright/.auth/user.json' }, + dependencies: ['setup'], + },package.json (1)
14-15: Enhance test scripts with useful options.The current test scripts could benefit from additional options for better usability:
- "test:playwright": "bunx playwright test", + "test:playwright": "bunx playwright test", + "test:playwright:ui": "bunx playwright test --ui", + "test:playwright:debug": "bunx playwright test --debug", + "test:playwright:report": "bunx playwright show-report",e2e/auth.setup.ts (3)
7-10: Validate GitHub credentials before proceedingThe GitHub email and password are retrieved from environment variables without checking if they are provided. If the environment variables are missing, the credentials default to empty strings, which could cause authentication to fail without a clear error message. Consider adding validation to ensure that the credentials are present.
You can add a validation check:
const github = { email: process.env.GITHUB_USER_EMAIL ?? '', password: process.env.GITHUB_USER_PASSWORD ?? '', }; + if (!github.email || !github.password) { + throw new Error('GitHub credentials are not provided in environment variables.'); + }
24-30: Use resilient selectors for reauthorization handlingThe selector
'button:has-text("Authorize iam-login")'is tied to specific button text, which may change if the application name changes or GitHub updates their UI. This could cause the test to fail unexpectedly.Consider using a more robust selector:
- await page1.waitForSelector('button:has-text("Authorize iam-login")', { + await page1.waitForSelector('form[action^="/login/oauth/authorize"] button[type="submit"]', { state: 'visible', timeout: 10000, });
36-41: Use resilient selectors for 2FA setup handlingSimilar to the previous comment, the selector
'text=Skip for now'may fail if the button text changes. Using a more reliable selector can make the test less brittle.Consider updating the selector:
- await page1.waitForSelector('text=Skip for now', { + await page1.waitForSelector('button[data-testid="2fa-skip-button"]', { state: 'visible', timeout: 10000, });e2e/utils/auth.ts (1)
28-30: Log errors during sign-in for better troubleshootingWhen an error occurs during the sign-in process, it is silently caught, making it difficult to debug issues.
Modify the catch block to log the error:
} catch (error) { + console.error('Error during GitHub sign-in:', error); return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
bun.lockbis excluded by!**/bun.lockb
📒 Files selected for processing (7)
.gitignore(1 hunks)e2e/auth.setup.ts(1 hunks)e2e/bank.spec.ts(1 hunks)e2e/landing.spec.ts(1 hunks)e2e/utils/auth.ts(1 hunks)package.json(3 hunks)playwright.config.ts(1 hunks)
🔇 Additional comments (1)
playwright.config.ts (1)
1-9: LGTM! Good environment setup.
The configuration properly separates test environment variables using a dedicated .env.playwright file.
| const button = page.locator('text=Learn how it works'); | ||
| await expect(button).toBeVisible(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use data-testid for more reliable element selection
Using text content for element selection can be fragile as it may change with UI updates or internationalization. Consider using data-testid attributes.
- const button = page.locator('text=Learn how it works');
+ const button = page.getByTestId('learn-more-button');Also, consider adding a test for the button's click interaction and its expected behavior.
Committable suggestion skipped: line range outside the PR's diff.
| .getByPlaceholder('Enter address') | ||
| .fill('manifest1hj5fveer5cjtn4wd6wstzugjfdxzl0xp8ws9ct'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move test data to a separate constants file
Hardcoded values like addresses should be moved to a separate test constants file for better maintainability.
Create a new file e2e/constants.ts:
export const TEST_CONSTANTS = {
RECIPIENT_ADDRESS: 'manifest1hj5fveer5cjtn4wd6wstzugjfdxzl0xp8ws9ct',
TEST_AMOUNT: '1',
TEST_MEMO: 'E2E bank test'
};Then update the test to use these constants.
| test('send mfx', async ({ page }) => { | ||
| await page.goto('/bank'); | ||
| await expect(page.getByText('mfx', { exact: true })).toBeVisible(); | ||
| await page.getByLabel('send-mfx').click(); | ||
| await page.getByPlaceholder('0.00').click(); | ||
| await page.getByPlaceholder('0.00').fill('1'); | ||
| await page.getByPlaceholder('Enter address').click(); | ||
| await page | ||
| .getByPlaceholder('Enter address') | ||
| .fill('manifest1hj5fveer5cjtn4wd6wstzugjfdxzl0xp8ws9ct'); | ||
| await page.getByPlaceholder('Memo').click(); | ||
| await page.getByPlaceholder('Memo').fill('E2E bank test'); | ||
| await page.getByLabel('send-btn').click(); | ||
| await page.getByRole('button', { name: 'Approve' }).click(); | ||
| await page.waitForSelector('text=Transaction Successful'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test robustness and organization
The test needs several improvements:
- Add error handling for transaction steps
- Validate transaction details before approval
- Break down the long test into smaller, focused test cases
- Add proper assertions for the transaction state
Here's a suggested refactor:
import { test, expect } from '@playwright/test';
import { TEST_CONSTANTS } from './constants';
test.describe('Bank Transactions', () => {
test.beforeEach(async ({ page }) => {
await page.goto('/bank');
});
test('should display MFX balance', async ({ page }) => {
await expect(page.getByTestId('mfx-balance')).toBeVisible();
});
test('should validate transaction form', async ({ page }) => {
await page.getByLabel('send-mfx').click();
// Test form validation
await page.getByLabel('send-btn').click();
await expect(page.getByText('Amount is required')).toBeVisible();
// Fill invalid amount
await page.getByPlaceholder('0.00').fill('-1');
await expect(page.getByText('Amount must be positive')).toBeVisible();
});
test('should complete MFX transaction', async ({ page }) => {
await page.getByLabel('send-mfx').click();
// Fill transaction details
await page.getByPlaceholder('0.00').fill(TEST_CONSTANTS.TEST_AMOUNT);
await page.getByPlaceholder('Enter address').fill(TEST_CONSTANTS.RECIPIENT_ADDRESS);
await page.getByPlaceholder('Memo').fill(TEST_CONSTANTS.TEST_MEMO);
// Verify transaction details in confirmation dialog
await page.getByLabel('send-btn').click();
await expect(page.getByText(`Amount: ${TEST_CONSTANTS.TEST_AMOUNT} MFX`)).toBeVisible();
await expect(page.getByText(`To: ${TEST_CONSTANTS.RECIPIENT_ADDRESS}`)).toBeVisible();
// Complete transaction
await page.getByRole('button', { name: 'Approve' }).click();
// Verify success and balance update
await expect(page.getByText('Transaction Successful')).toBeVisible();
// Add assertion for updated balance
});
});| await page.getByPlaceholder('Memo').fill('E2E bank test'); | ||
| await page.getByLabel('send-btn').click(); | ||
| await page.getByRole('button', { name: 'Approve' }).click(); | ||
| await page.waitForSelector('text=Transaction Successful'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout and error handling for transaction confirmation
The waitForSelector call should include a reasonable timeout and error handling for failed transactions.
- await page.waitForSelector('text=Transaction Successful');
+ try {
+ await page.waitForSelector('text=Transaction Successful', { timeout: 30000 });
+ } catch (error) {
+ throw new Error('Transaction did not complete within 30 seconds');
+ }📝 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.waitForSelector('text=Transaction Successful'); | |
| try { | |
| await page.waitForSelector('text=Transaction Successful', { timeout: 30000 }); | |
| } catch (error) { | |
| throw new Error('Transaction did not complete within 30 seconds'); | |
| } |
| /* Run your local dev server before starting the tests */ | ||
| webServer: { | ||
| command: 'bun run start', | ||
| url: 'http://127.0.0.1:3000', | ||
| reuseExistingServer: !process.env.CI, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add health check URL for reliable server startup.
The webServer configuration should include a health check URL to ensure the server is fully ready before tests begin.
webServer: {
command: 'bun run start',
url: 'http://127.0.0.1:3000',
+ reuseExistingServer: !process.env.CI,
+ stdout: 'pipe',
+ stderr: 'pipe',
+ timeout: 120000,
},Committable suggestion skipped: line range outside the PR's diff.
| @@ -84,13 +86,15 @@ | |||
| "@types/bad-words": "^3.0.3", | |||
| "@types/crypto-js": "^4.2.2", | |||
| "@types/identicon.js": "^2.3.4", | |||
| "@types/node": "^22.9.1", | |||
There was a problem hiding this comment.
Verify @types/node version compatibility.
The @types/node version (^22.9.1) seems unusually high and might be incorrect, as Node.js itself is currently at version 20.x. This could lead to type definition incompatibilities.
- "@types/node": "^22.9.1",
+ "@types/node": "^20.11.0",📝 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.
| "@types/node": "^22.9.1", | |
| "@types/node": "^20.11.0", |
| timeout: 20000, | ||
| }); | ||
|
|
||
| await page.context().storageState({ path: authFile }); |
There was a problem hiding this comment.
💡 Codebase verification
Add playwright/.auth to .gitignore to prevent committing sensitive authentication data
Add the following line to .gitignore:
playwright/.auth
This will prevent accidentally committing authentication state files that may contain sensitive information like session tokens.
🔗 Analysis chain
Ensure authentication state file is excluded from version control
The authentication state is saved to ../playwright/.auth/user.json, which may contain sensitive information like session tokens. To prevent accidental commits of sensitive data, ensure this path is listed in .gitignore.
Run the following script to verify that playwright/.auth is included in .gitignore:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if 'playwright/.auth' is listed in .gitignore
if ! grep -q '^playwright/.auth' .gitignore; then
echo "'playwright/.auth' is not in .gitignore. Please add it to prevent committing sensitive data."
else
echo "'playwright/.auth' is correctly ignored."
fi
Length of output: 241
| setup('authenticate', async ({ page }) => { | ||
| setup.setTimeout(120000); | ||
|
|
||
| await signInWithGitHub({ page, github }); |
There was a problem hiding this comment.
Handle sign-in failure from signInWithGitHub
The function signInWithGitHub returns a boolean indicating success or failure, but the return value is not checked. If the sign-in fails, the script will proceed, potentially causing unexpected errors later. Consider handling the result of the sign-in attempt.
Modify the code to check the return value:
- await signInWithGitHub({ page, github });
+ const signInSuccess = await signInWithGitHub({ page, github });
+ if (!signInSuccess) {
+ throw new Error('GitHub sign-in failed.');
+ }📝 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 signInWithGitHub({ page, github }); | |
| const signInSuccess = await signInWithGitHub({ page, github }); | |
| if (!signInSuccess) { | |
| throw new Error('GitHub sign-in failed.'); | |
| } |
| await page.waitForSelector('text=Create repository'); | ||
| expect(page.isVisible('text=Create repository')).toBe(true); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a more reliable check for successful login
Waiting for 'text=Create repository' assumes the user's landing page contains this text, which may not always be true (e.g., for new accounts or if GitHub changes their UI). This could lead to false negatives in login verification.
Consider verifying the existence of the user's avatar or a consistent element in the navbar:
- await page.waitForSelector('text=Create repository');
- expect(page.isVisible('text=Create repository')).toBe(true);
+ await page.waitForSelector('summary[aria-label="View profile and more"]', {
+ state: 'visible',
+ });
+ expect(await page.isVisible('summary[aria-label="View profile and more"]')).toBe(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.
| await page.waitForSelector('text=Create repository'); | |
| expect(page.isVisible('text=Create repository')).toBe(true); | |
| await page.waitForSelector('summary[aria-label="View profile and more"]', { | |
| state: 'visible', | |
| }); | |
| expect(await page.isVisible('summary[aria-label="View profile and more"]')).toBe(true); |
This PR adds PR support for e2e testing.
Create a
.env.playwrightand set theGITHUB_USER_EMAILandGITHUB_USER_PASSWORDvariables to the appropriate testing account. Make sure the account has MFX funds.This gives us the basics to add more e2e tests.