Add comprehensive E2E test suite #350
Conversation
This commit adds a comprehensive end-to-end test suite using Playwright to verify the functionality of all major UI components and user flows. It includes: - `data-testid` attributes added to all interactive UI elements for reliable testing. - New test files for the header, map, sidebar, and mobile-specific UI. - Expanded tests for the chat functionality, including file attachments and new chat creation. - Fixes for placeholder code and removal of unnecessary test files. - A fix for a build error caused by an incorrect import path in `mapbox_mcp/hooks.ts`.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Comment |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||
|
@coderabbitai can you figure out how to achieve a comprehensive test suite of the whole applications current states and functionality and review the existing code. |
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||||
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3069 🏁 Script executed: Length of output: 395 🏁 Script executed: Length of output: 10393 🏁 Script executed: Length of output: 684 🏁 Script executed: Length of output: 3191 🏁 Script executed: Length of output: 31645 🏁 Script executed: Length of output: 11962 🏁 Script executed: Length of output: 9510 🏁 Script executed: Length of output: 4862 🏁 Script executed: Length of output: 48 @ngoiyaeric, I'll provide a comprehensive analysis of the test suite and recommendations for achieving complete coverage of your application. Current Test Coverage AnalysisThe PR adds a solid foundation with 5 E2E test files covering:
Critical Gaps in Test Coverage1. API Endpoints (Not Tested)// tests/api/chat.spec.ts - NEW FILE NEEDED
import { test, expect } from '@playwright/test';
test.describe('Chat API', () => {
test('POST /api/chat creates a new chat', async ({ request }) => {
const response = await request.post('/api/chat', {
data: {
title: 'Test Chat',
initialMessageContent: 'Hello API',
role: 'user'
}
});
expect(response.status()).toBe(201);
const data = await response.json();
expect(data.chatId).toBeDefined();
});
test('GET /api/chats/all returns user chats', async ({ request }) => {
const response = await request.get('/api/chats/all');
expect(response.ok()).toBeTruthy();
const chats = await response.json();
expect(Array.isArray(chats)).toBeTruthy();
});
});2. Search Functionality (Not Tested)// tests/search.spec.ts - NEW FILE NEEDED
import { test, expect } from '@playwright/test';
test.describe('Search Functionality', () => {
test('should display search results for a query', async ({ page }) => {
await page.goto('/');
await page.fill('[data-testid="chat-input"]', 'climate change effects');
await page.click('[data-testid="chat-submit"]');
// Wait for search results to appear
await expect(page.locator('.search-results')).toBeVisible({ timeout: 20000 });
await expect(page.locator('.search-results .result-item')).toHaveCount.greaterThan(0);
});
test('should display image results', async ({ page }) => {
await page.goto('/');
await page.fill('[data-testid="chat-input"]', 'show me images of forests');
await page.click('[data-testid="chat-submit"]');
await expect(page.locator('[data-testid="search-images"]')).toBeVisible({ timeout: 20000 });
});
});3. Calendar/Notepad Features (Not Tested)// tests/calendar.spec.ts - NEW FILE NEEDED
import { test, expect } from '@playwright/test';
test.describe('Calendar and Notes', () => {
test.beforeEach(async ({ page }) => {
await page.goto('/');
await page.click('[data-testid="calendar-toggle"]');
await expect(page.locator('[data-testid="calendar-notepad"]')).toBeVisible();
});
test('should add a note to current date', async ({ page }) => {
const noteContent = 'Test note content';
await page.fill('textarea[placeholder*="Add note"]', noteContent);
await page.keyboard.press('Meta+Enter');
await expect(page.locator(`text=${noteContent}`)).toBeVisible();
});
test('should tag location in note', async ({ page }) => {
await page.fill('textarea[placeholder*="Add note"]', 'Location test');
await page.click('button:has-text("MapPin")');
await expect(page.locator('textarea')).toContainText('#location');
});
test('should navigate between dates', async ({ page }) => {
const nextWeekButton = page.locator('button:has(svg.lucide-chevron-right)');
await nextWeekButton.click();
// Verify date changed (check for different date numbers)
await page.waitForTimeout(500);
});
});4. Authentication & Authorization (Not Tested)// tests/auth.spec.ts - NEW FILE NEEDED
import { test, expect } from '@playwright/test';
test.describe('Authentication', () => {
test('should redirect unauthenticated users', async ({ page }) => {
// Clear cookies/storage
await page.context().clearCookies();
await page.goto('/');
// Verify redirect or login prompt
await expect(page).toHaveURL(/login|auth/);
});
test('should access protected API routes when authenticated', async ({ request }) => {
const response = await request.get('/api/chats/all');
expect(response.status()).not.toBe(401);
});
});5. Error Handling & Edge Cases (Not Tested)// tests/error-handling.spec.ts - NEW FILE NEEDED
import { test, expect } from '@playwright/test';
test.describe('Error Handling', () => {
test('should handle network errors gracefully', async ({ page, context }) => {
await page.goto('/');
// Simulate offline
await context.setOffline(true);
await page.fill('[data-testid="chat-input"]', 'Test message');
await page.click('[data-testid="chat-submit"]');
// Verify error message or retry behavior
await expect(page.locator('text=/error|failed|retry/i')).toBeVisible({ timeout: 10000 });
});
test('should handle file upload size limit', async ({ page }) => {
await page.goto('/');
// Try to upload file > 10MB (would need actual large file)
const fileInput = page.locator('input[type="file"]');
// This would trigger the alert in chat-panel.tsx
});
test('should prevent empty message submission', async ({ page }) => {
await page.goto('/');
const submitButton = page.locator('[data-testid="chat-submit"]');
await expect(submitButton).toBeDisabled();
await page.fill('[data-testid="chat-input"]', ' '); // Only spaces
await expect(submitButton).toBeDisabled();
});
});6. Map Drawing & GeoJSON Features (Not Tested)// tests/map-advanced.spec.ts - NEW FILE NEEDED
import { test, expect } from '@playwright/test';
test.describe('Advanced Map Features', () => {
test('should save drawn features', async ({ page }) => {
await page.goto('/');
await page.click('[data-testid="map-toggle"]');
await page.click('[data-testid="map-mode-draw"]');
// Wait for draw controls
await expect(page.locator('.mapboxgl-ctrl-draw-btn')).toBeVisible();
// Simulate drawing (complex - may need custom helpers)
// Verify features are saved to context
});
test('should load GeoJSON layers', async ({ page }) => {
await page.goto('/');
// Send a message that triggers GeoJSON display
await page.fill('[data-testid="chat-input"]', 'Show me earthquake data');
await page.click('[data-testid="chat-submit"]');
// Verify GeoJSON layer is rendered on map
await page.waitForTimeout(3000);
const canvas = page.locator('.mapboxgl-canvas');
await expect(canvas).toBeVisible();
});
});7. Settings & Profile Management (Incomplete)// tests/settings.spec.ts - NEW FILE NEEDED
import { test, expect } from '@playwright/test';
test.describe('Settings', () => {
test('should navigate through all settings sections', async ({ page }) => {
await page.goto('/');
await page.click('[data-testid="profile-toggle"]');
// Test each section
await page.click('[data-testid="profile-settings"]');
await expect(page.locator('text=/model|settings/i')).toBeVisible();
await page.click('[data-testid="profile-toggle"]');
await page.click('[data-testid="profile-appearance"]');
await expect(page.locator('text=/appearance|theme/i')).toBeVisible();
await page.click('[data-testid="profile-toggle"]');
await page.click('[data-testid="profile-security"]');
await expect(page.locator('text=/security|password/i')).toBeVisible();
});
});8. Accessibility Tests (Missing)// tests/accessibility.spec.ts - NEW FILE NEEDED
import { test, expect } from '@playwright/test';
import AxeBuilder from '@axe-core/playwright';
test.describe('Accessibility', () => {
test('should have no accessibility violations on home page', async ({ page }) => {
await page.goto('/');
const accessibilityScanResults = await new AxeBuilder({ page }).analyze();
expect(accessibilityScanResults.violations).toEqual([]);
});
test('should be keyboard navigable', async ({ page }) => {
await page.goto('/');
// Tab through interactive elements
await page.keyboard.press('Tab');
await expect(page.locator('[data-testid="profile-toggle"]')).toBeFocused();
await page.keyboard.press('Tab');
// Continue testing keyboard navigation
});
test('should have proper ARIA labels', async ({ page }) => {
await page.goto('/');
const submitButton = page.locator('[data-testid="chat-submit"]');
await expect(submitButton).toHaveAttribute('aria-label', 'Send message');
});
});Test Quality ImprovementsIssues with Current Tests:
// ❌ BAD: Arbitrary timeout
await page.waitForTimeout(500);
// ✅ GOOD: Wait for specific condition
await page.waitForFunction(() => (window as any).map.getZoom() > initialZoom);
// ❌ BAD: Assumes global window.map exists
const getZoom = () => page.evaluate(() => (window as any).map.getZoom());
// ✅ GOOD: Check if map exists first, or use data attributes
await page.waitForSelector('.mapboxgl-canvas');
const zoomLevel = await page.getAttribute('[data-map-zoom]', 'value');
// ❌ BAD: Creates file but doesn't clean up
require('fs').writeFileSync(filePath, 'This is a test file.');
// ✅ GOOD: Use fixtures and cleanup
import { test as base } from '@playwright/test';
import fs from 'fs/promises';
const test = base.extend({
testFile: async ({}, use) => {
const filePath = path.join(__dirname, 'test-file.txt');
await fs.writeFile(filePath, 'This is a test file.');
await use(filePath);
await fs.unlink(filePath).catch(() => {}); // Cleanup
}
});Recommended Testing Strategy1. Add Test Fixtures// tests/fixtures/test-data.ts
export const testMessages = [
{ role: 'user', content: 'Hello' },
{ role: 'assistant', content: 'Hi there!' }
];
export const testChat = {
id: 'test-chat-id',
title: 'Test Chat',
messages: testMessages
};2. Mock API Responses// tests/chat.spec.ts
test('should display mocked chat response', async ({ page }) => {
await page.route('**/api/chat', route => {
route.fulfill({
status: 200,
body: JSON.stringify({ message: 'Mocked response' })
});
});
await page.goto('/');
await page.fill('[data-testid="chat-input"]', 'Test');
await page.click('[data-testid="chat-submit"]');
await expect(page.locator('text=Mocked response')).toBeVisible();
});3. Add Visual Regression Tests// playwright.config.ts - add to config
expect: {
toHaveScreenshot: { maxDiffPixels: 100 }
}
// tests/visual.spec.ts
test('homepage matches screenshot', async ({ page }) => {
await page.goto('/');
await expect(page).toHaveScreenshot('homepage.png');
});4. Add Performance Tests// tests/performance.spec.ts
import { test, expect } from '@playwright/test';
test('page should load within 3 seconds', async ({ page }) => {
const startTime = Date.now();
await page.goto('/');
await page.waitForLoadState('networkidle');
const loadTime = Date.now() - startTime;
expect(loadTime).toBeLessThan(3000);
});5. Integration Test Example// tests/integration/chat-to-map.spec.ts
test('should update map when sending location query', async ({ page }) => {
await page.goto('/');
// Send message with location
await page.fill('[data-testid="chat-input"]', 'Show me New York');
await page.click('[data-testid="chat-submit"]');
// Wait for bot response
await page.waitForSelector('div.items-start', { timeout: 15000 });
// Verify map updated (check for map markers or center change)
await expect(page.locator('.mapboxgl-marker')).toBeVisible();
});Recommended Test OrganizationImmediate Action Items
Would you like me to generate any of these specific test files, or would you prefer I create an issue to track this comprehensive testing roadmap? |
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
Several E2E tests are brittle or incorrect: the mobile UI test queries a non-existent .mobile-icons-bar selector, and the mobile profile button is inert while the test expects a dropdown to open. Theme assertions rely on exact class equality, which is fragile, and the map zoom test relies on a global window.map that may not exist. The chat attachment test writes a file into the repo without cleanup; use the test output directory instead.
Additional notes (1)
- Readability |
tests/sidebar.spec.ts:26-28
This final assertion is prone to race conditions when clearing history asynchronously and only checks visibility. Verifying that the list has zero items is more robust and self-documenting.
Summary of changes
- Added
data-testidattributes across multiple UI components (chat panel, header, history, map toggle, mobile icons bar, mode/profile toggles) to support E2E testing. - Introduced Playwright test infrastructure (
playwright.config.ts) with multi-browser/device projects and a dev web server. - Added comprehensive E2E tests for chat, header, map, mobile, and sidebar flows.
- Fixed an import path in
mapbox_mcp/hooks.tsfromQCX/lib/utilsto@/lib/utils. - Updated
.gitignoreto exclude Playwright artifacts and logs; added@playwright/testto devDependencies.
components/mobile-icons-bar.tsx
Outdated
| <Plus className="h-[1.2rem] w-[1.2rem]" /> | ||
| </Button> | ||
| <Button variant="ghost" size="icon"> | ||
| <Button variant="ghost" size="icon" data-testid="mobile-profile-button"> |
There was a problem hiding this comment.
This mobile profile button does not open any menu; it’s just an inert icon button. Your mobile.spec.ts expects a profile menu to open and check [data-testid="profile-account"], which will fail. Reuse the existing ProfileToggle (as in the header) to provide the dropdown on mobile as well.
Suggestion
Replace the standalone profile button with the shared ProfileToggle component:
// at top of the file
import { ProfileToggle } from './profile-toggle'
// in the JSX, replace the button with:
<ProfileToggle />Update the mobile test to click [data-testid="profile-toggle"] if needed. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change
components/mobile-icons-bar.tsx
Outdated
| <Paperclip className="h-[1.2rem] w-[1.2rem] transition-all rotate-0 scale-100" /> | ||
| </Button> | ||
| <Button variant="ghost" size="icon"> | ||
| <Button variant="ghost" size="icon" data-testid="mobile-submit-button"> |
There was a problem hiding this comment.
This submit button on mobile has no onClick and thus does nothing; adding a data-testid to it may encourage tests to click it expecting to send messages. Either wire it to the actual submit logic or hide/disable it until implemented to avoid misleading tests and UX.
Suggestion
If the submit action is not implemented yet, disable the button to avoid confusion:
<Button variant="ghost" size="icon" data-testid="mobile-submit-button" disabled aria-disabled="true">
<ArrowRight className="h-[1.2rem] w-[1.2rem]" />
</Button>Or, preferably, wire it to the message submit handler (e.g., by passing an onSubmit prop). Reply with "@CharlieHelps yes please" if you'd like me to add a commit with the disabling change
tests/header.spec.ts
Outdated
| const html = page.locator('html'); | ||
| await expect(html).toHaveAttribute('class', 'dark'); | ||
|
|
||
| await page.click('[data-testid="theme-toggle"]'); | ||
| await page.click('[data-testid="theme-light"]'); | ||
| await expect(html).not.toHaveAttribute('class', 'dark'); |
There was a problem hiding this comment.
Using exact toHaveAttribute('class', 'dark') is brittle, since the <html> element may have multiple classes (e.g., Tailwind injects others). This can make the test flaky or falsely failing. Prefer checking classList contains dark.
Suggestion
Update the assertions to check for the presence/absence of the dark class rather than exact equality:
await expect(html).toHaveClass(/(^|\s)dark(\s|$)/);
// ... later
await expect(html).not.toHaveClass(/(^|\s)dark(\s|$)/);Alternatively:
await expect(await html.evaluate(el => el.classList.contains('dark'))).toBe(true);
await expect(await html.evaluate(el => el.classList.contains('dark'))).toBe(false);Reply with "@CharlieHelps yes please" if you'd like me to add a commit updating the tests
tests/chat.spec.ts
Outdated
| const filePath = path.join(__dirname, 'test-file.txt'); | ||
| // Create a dummy file for the test | ||
| require('fs').writeFileSync(filePath, 'This is a test file.'); | ||
|
|
There was a problem hiding this comment.
This test writes a file into the source tree via require('fs').writeFileSync(...) and never cleans it up. It mixes require with ES imports and leaves local artifacts behind, which can pollute the working tree and cause flaky runs. Use Playwright’s test output folder and async FS APIs.
Suggestion
Use the test output directory and async FS:
import { promises as fs } from 'fs';
const filePath = test.info().outputPath('test-file.txt');
await fs.writeFile(filePath, 'This is a test file.');This keeps artifacts out of the repo and ensures per-test isolation. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change
| const getZoom = () => page.evaluate(() => (window as any).map.getZoom()); | ||
|
|
There was a problem hiding this comment.
This test assumes a global window.map to read zoom, which is fragile and will throw if the app doesn’t expose the Mapbox instance globally. Tests should not depend on hidden globals; either instrument the app in test mode to expose the map or guard/skip when unavailable.
Suggestion
Guard against missing globals or expose the map in test mode. For a minimal test-side fix:
const hasMap = await page.evaluate(() => Boolean((window as any).map));
if (!hasMap) test.skip(true, 'Map instance not available on window for E2E');
const getZoom = () => page.evaluate(() => (window as any).map.getZoom());Alternatively, instrument the app (in dev/test only) to assign the map instance to window.map after initialization. Reply with "@CharlieHelps yes please" if you'd like me to add a commit to apply the test-side guard
| webServer: { | ||
| command: 'bun run dev', | ||
| url: 'http://localhost:3000', | ||
| reuseExistingServer: !process.env.CI, | ||
| timeout: 600000, // 10 minutes | ||
| }, |
There was a problem hiding this comment.
Running E2E against the dev server (bun run dev) in CI tends to be slower and less deterministic (HMR, additional logging, non-optimized builds). Using a production build for CI improves reliability and performance.
Suggestion
Switch to a production server in CI while keeping dev locally:
webServer: {
command: process.env.CI ? 'bun run build && bun run start' : 'bun run dev',
url: 'http://localhost:3000',
reuseExistingServer: !process.env.CI,
timeout: 600000,
},Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
This commit adds a comprehensive and robust end-to-end test suite using Playwright to verify the functionality of all major UI components and user flows. It includes: - `data-testid` attributes added to all interactive UI elements for reliable testing. - New test files for the header, map, sidebar, and mobile-specific UI. - Expanded tests for the chat functionality, including file attachments and new chat creation. - Fixes for placeholder code and removal of unnecessary test files. - A fix for a build error caused by an incorrect import path in `mapbox_mcp/hooks.ts`. - Significant improvements to the quality and reliability of the tests based on detailed code review feedback. This includes: - Using more robust assertions. - Handling temporary files correctly. - Adding guards for fragile tests. - Improving the Playwright configuration for CI environments. - Fixing bugs in the UI that were discovered during testing.
2 participants
User description
This change introduces a comprehensive end-to-end test suite to the project. It adds
data-testidattributes to all interactive UI elements and implements a wide range of tests for the header, chat, map, sidebar, and mobile UI. This will help to prevent regressions and ensure the stability of the application. It also includes a fix for a build error that was discovered during the verification process.PR created automatically by Jules for task 12672328258465556184
PR Type
Tests, Enhancement, Bug fix
Description
Add comprehensive Playwright E2E test suite with tests for chat, header, map, sidebar, and mobile UI
Add
data-testidattributes to all interactive UI elements for reliable test targetingFix incorrect import path in
mapbox_mcp/hooks.tsfromQCX/lib/utilsto@/lib/utilsConfigure Playwright with multi-browser support (Chromium, Firefox, WebKit) and mobile device testing
Diagram Walkthrough
File Walkthrough
1 files
Fix incorrect import path in hooks6 files
Add Playwright configuration for E2E testingAdd comprehensive chat functionality testsAdd header and navigation testsAdd map functionality and controls testsAdd mobile UI interaction testsAdd sidebar and chat history tests9 files
Add data-testid attributes to chat elementsAdd data-testid to header buttonsAdd data-testid to history item linkAdd data-testid to history panel elementsAdd data-testid to map toggle menu itemsAdd data-testid to mobile icon buttonsAdd data-testid to theme toggle elementsAdd data-testid to profile menu itemsAdd data-testid to history clear dialog buttons1 files
Add Playwright test dependency