From d517268dbe430db8abe5ee58941a782fda64c699 Mon Sep 17 00:00:00 2001 From: "dayuan.jiang" Date: Sun, 4 Jan 2026 22:25:50 +0900 Subject: [PATCH] fix: improve test infrastructure based on PR review - Fix double build in CI: remove redundant build from playwright webServer - Export chat helpers from shared module for proper unit testing - Replace waitForTimeout with explicit waits in E2E tests - Add data-testid attributes to settings and new chat buttons - Add list reporter for CI to show failures in logs - Add Playwright browser caching to speed up CI - Add vitest coverage configuration - Fix conditional test assertions to use test.skip() instead of silent pass - Remove unused variables flagged by linter --- .github/workflows/test.yml | 12 ++++ app/api/chat/route.ts | 92 ++----------------------------- components/chat-panel.tsx | 2 + lib/chat-helpers.ts | 89 ++++++++++++++++++++++++++++++ playwright.config.ts | 6 +- tests/e2e/copy-paste.spec.ts | 45 +++++++-------- tests/e2e/file-upload.spec.ts | 36 ++++++------ tests/e2e/history-restore.spec.ts | 78 +++++++++++++------------- tests/e2e/settings.spec.ts | 12 +--- tests/e2e/smoke.spec.ts | 4 +- tests/e2e/theme.spec.ts | 10 ---- tests/unit/chat-helpers.test.ts | 85 ++-------------------------- vitest.config.mts | 6 ++ 13 files changed, 205 insertions(+), 272 deletions(-) create mode 100644 lib/chat-helpers.ts diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 980fc8d..8864ef8 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -43,9 +43,21 @@ jobs: - name: Install dependencies run: npm ci + - name: Cache Playwright browsers + uses: actions/cache@v4 + id: playwright-cache + with: + path: ~/.cache/ms-playwright + key: playwright-${{ runner.os }}-${{ hashFiles('**/package-lock.json') }} + - name: Install Playwright browsers + if: steps.playwright-cache.outputs.cache-hit != 'true' run: npx playwright install chromium --with-deps + - name: Install Playwright deps (cached) + if: steps.playwright-cache.outputs.cache-hit == 'true' + run: npx playwright install-deps chromium + - name: Build app run: npm run build diff --git a/app/api/chat/route.ts b/app/api/chat/route.ts index 55dbbde..872b870 100644 --- a/app/api/chat/route.ts +++ b/app/api/chat/route.ts @@ -18,6 +18,11 @@ import { supportsPromptCaching, } from "@/lib/ai-providers" import { findCachedResponse } from "@/lib/cached-responses" +import { + isMinimalDiagram, + replaceHistoricalToolInputs, + validateFileParts, +} from "@/lib/chat-helpers" import { checkAndIncrementRequest, isQuotaEnabled, @@ -34,93 +39,6 @@ import { getUserIdFromRequest } from "@/lib/user-id" export const maxDuration = 120 -// File upload limits (must match client-side) -const MAX_FILE_SIZE = 2 * 1024 * 1024 // 2MB -const MAX_FILES = 5 - -// Helper function to validate file parts in messages -function validateFileParts(messages: any[]): { - valid: boolean - error?: string -} { - const lastMessage = messages[messages.length - 1] - const fileParts = - lastMessage?.parts?.filter((p: any) => p.type === "file") || [] - - if (fileParts.length > MAX_FILES) { - return { - valid: false, - error: `Too many files. Maximum ${MAX_FILES} allowed.`, - } - } - - for (const filePart of fileParts) { - // Data URLs format: data:image/png;base64, - // Base64 increases size by ~33%, so we check the decoded size - if (filePart.url?.startsWith("data:")) { - const base64Data = filePart.url.split(",")[1] - if (base64Data) { - const sizeInBytes = Math.ceil((base64Data.length * 3) / 4) - if (sizeInBytes > MAX_FILE_SIZE) { - return { - valid: false, - error: `File exceeds ${MAX_FILE_SIZE / 1024 / 1024}MB limit.`, - } - } - } - } - } - - return { valid: true } -} - -// Helper function to check if diagram is minimal/empty -function isMinimalDiagram(xml: string): boolean { - const stripped = xml.replace(/\s/g, "") - return !stripped.includes('id="2"') -} - -// Helper function to replace historical tool call XML with placeholders -// This reduces token usage and forces LLM to rely on the current diagram XML (source of truth) -// Also fixes invalid/undefined inputs from interrupted streaming -function replaceHistoricalToolInputs(messages: any[]): any[] { - return messages.map((msg) => { - if (msg.role !== "assistant" || !Array.isArray(msg.content)) { - return msg - } - const replacedContent = msg.content - .map((part: any) => { - if (part.type === "tool-call") { - const toolName = part.toolName - // Fix invalid/undefined inputs from interrupted streaming - if ( - !part.input || - typeof part.input !== "object" || - Object.keys(part.input).length === 0 - ) { - // Skip tool calls with invalid inputs entirely - return null - } - if ( - toolName === "display_diagram" || - toolName === "edit_diagram" - ) { - return { - ...part, - input: { - placeholder: - "[XML content replaced - see current diagram XML in system context]", - }, - } - } - } - return part - }) - .filter(Boolean) // Remove null entries (invalid tool calls) - return { ...msg, content: replacedContent } - }) -} - // Helper function to create cached stream response function createCachedStreamResponse(xml: string): Response { const toolCallId = `cached-${Date.now()}` diff --git a/components/chat-panel.tsx b/components/chat-panel.tsx index 7aa28a5..acdb81d 100644 --- a/components/chat-panel.tsx +++ b/components/chat-panel.tsx @@ -1185,6 +1185,7 @@ export default function ChatPanel({ status === "streaming" || status === "submitted" } className="hover:bg-accent disabled:opacity-50 disabled:cursor-not-allowed" + data-testid="new-chat-button" > setShowSettingsDialog(true)} className="hover:bg-accent" + data-testid="settings-button" > p.type === "file") || [] + + if (fileParts.length > MAX_FILES) { + return { + valid: false, + error: `Too many files. Maximum ${MAX_FILES} allowed.`, + } + } + + for (const filePart of fileParts) { + // Data URLs format: data:image/png;base64, + // Base64 increases size by ~33%, so we check the decoded size + if (filePart.url?.startsWith("data:")) { + const base64Data = filePart.url.split(",")[1] + if (base64Data) { + const sizeInBytes = Math.ceil((base64Data.length * 3) / 4) + if (sizeInBytes > MAX_FILE_SIZE) { + return { + valid: false, + error: `File exceeds ${MAX_FILE_SIZE / 1024 / 1024}MB limit.`, + } + } + } + } + } + + return { valid: true } +} + +// Helper function to check if diagram is minimal/empty +export function isMinimalDiagram(xml: string): boolean { + const stripped = xml.replace(/\s/g, "") + return !stripped.includes('id="2"') +} + +// Helper function to replace historical tool call XML with placeholders +// This reduces token usage and forces LLM to rely on the current diagram XML (source of truth) +// Also fixes invalid/undefined inputs from interrupted streaming +export function replaceHistoricalToolInputs(messages: any[]): any[] { + return messages.map((msg) => { + if (msg.role !== "assistant" || !Array.isArray(msg.content)) { + return msg + } + const replacedContent = msg.content + .map((part: any) => { + if (part.type === "tool-call") { + const toolName = part.toolName + // Fix invalid/undefined inputs from interrupted streaming + if ( + !part.input || + typeof part.input !== "object" || + Object.keys(part.input).length === 0 + ) { + // Skip tool calls with invalid inputs entirely + return null + } + if ( + toolName === "display_diagram" || + toolName === "edit_diagram" + ) { + return { + ...part, + input: { + placeholder: + "[XML content replaced - see current diagram XML in system context]", + }, + } + } + } + return part + }) + .filter(Boolean) // Remove null entries (invalid tool calls) + return { ...msg, content: replacedContent } + }) +} diff --git a/playwright.config.ts b/playwright.config.ts index c06a582..012b8cd 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -6,11 +6,9 @@ export default defineConfig({ forbidOnly: !!process.env.CI, retries: process.env.CI ? 2 : 0, workers: process.env.CI ? 1 : undefined, - reporter: "html", + reporter: process.env.CI ? [["list"], ["html"]] : "html", webServer: { - command: process.env.CI - ? "npm run build && npm run start" - : "npm run dev", + command: process.env.CI ? "npm run start" : "npm run dev", port: process.env.CI ? 6001 : 6002, reuseExistingServer: !process.env.CI, timeout: 120 * 1000, diff --git a/tests/e2e/copy-paste.spec.ts b/tests/e2e/copy-paste.spec.ts index 5a3326c..15149c1 100644 --- a/tests/e2e/copy-paste.spec.ts +++ b/tests/e2e/copy-paste.spec.ts @@ -65,18 +65,18 @@ test.describe("Copy/Paste Functionality", () => { // Find copy button in message const copyButton = page.locator( - 'button[aria-label*="Copy"], button:has(svg.lucide-copy), button:has(svg.lucide-clipboard)', + '[data-testid="copy-button"], button[aria-label*="Copy"], button:has(svg.lucide-copy), button:has(svg.lucide-clipboard)', ) - if ((await copyButton.count()) > 0) { - await copyButton.first().click() - // Should show copied confirmation (toast or button state change) - await expect( - page - .locator('text="Copied"') - .or(page.locator("svg.lucide-check")), - ).toBeVisible({ timeout: 3000 }) - } + // Skip test if copy button doesn't exist + const buttonCount = await copyButton.count() + test.skip(buttonCount === 0, "Copy button not available") + + await copyButton.first().click() + // Should show copied confirmation (toast or button state change) + await expect( + page.locator('text="Copied"').or(page.locator("svg.lucide-check")), + ).toBeVisible({ timeout: 3000 }) }) test("paste XML into XML input works", async ({ page }) => { @@ -96,22 +96,20 @@ test.describe("Copy/Paste Functionality", () => { ) if ((await xmlToggle.count()) > 0) { await xmlToggle.first().click() - await page.waitForTimeout(500) } - // Check if XML input is now visible - if ( - (await xmlInput.count()) > 0 && - (await xmlInput.first().isVisible()) - ) { - const testXml = ` + // Skip test if XML input feature doesn't exist + const xmlInputCount = await xmlInput.count() + const isXmlVisible = + xmlInputCount > 0 && (await xmlInput.first().isVisible()) + test.skip(!isXmlVisible, "XML input feature not available") + + const testXml = ` ` - await xmlInput.first().fill(testXml) - await expect(xmlInput.first()).toHaveValue(testXml) - } - // Test passes if XML input doesn't exist or isn't accessible + await xmlInput.first().fill(testXml) + await expect(xmlInput.first()).toHaveValue(testXml) }) test("keyboard shortcuts work in chat input", async ({ page }) => { @@ -154,9 +152,8 @@ test.describe("Copy/Paste Functionality", () => { // Undo with Ctrl+Z await chatInput.press("ControlOrMeta+z") - // Value might revert (depends on browser/implementation) - // At minimum, shouldn't crash - await page.waitForTimeout(500) + // Verify page is still functional after undo + await expect(chatInput).toBeVisible() }) test("chat input handles special characters", async ({ page }) => { diff --git a/tests/e2e/file-upload.spec.ts b/tests/e2e/file-upload.spec.ts index 46a9d50..a73c5f9 100644 --- a/tests/e2e/file-upload.spec.ts +++ b/tests/e2e/file-upload.spec.ts @@ -39,11 +39,11 @@ test.describe("File Upload", () => { ), }) - // Wait for file to be processed - await page.waitForTimeout(1000) - - // File input should have processed the file - check any indicator - // This test passes if no error occurs during file selection + // File input should have processed the file + // Check that no error toast appeared + await expect( + page.locator('[role="alert"][data-type="error"]'), + ).not.toBeVisible({ timeout: 2000 }) }) test("can remove uploaded file", async ({ page }) => { @@ -64,18 +64,21 @@ test.describe("File Upload", () => { ), }) - // Wait for file to be processed - await page.waitForTimeout(1000) + // Wait for file preview or no error + await expect( + page.locator('[role="alert"][data-type="error"]'), + ).not.toBeVisible({ timeout: 2000 }) // Find and click remove button if it exists (X icon) const removeButton = page.locator( - 'button[aria-label*="Remove"], button:has(svg.lucide-x)', + '[data-testid="remove-file-button"], button[aria-label*="Remove"], button:has(svg.lucide-x)', ) - if ((await removeButton.count()) > 0) { - await removeButton.first().click() - await page.waitForTimeout(500) - } - // Test passes if no errors during upload and potential removal + const removeButtonCount = await removeButton.count() + test.skip(removeButtonCount === 0, "Remove file button not available") + + await removeButton.first().click() + // Verify button is gone or file preview is removed + await expect(removeButton.first()).not.toBeVisible({ timeout: 2000 }) }) test("sends file with message to API", async ({ page }) => { @@ -170,8 +173,9 @@ test.describe("File Upload", () => { await chatForm.dispatchEvent("dragover", { dataTransfer }) await chatForm.dispatchEvent("drop", { dataTransfer }) - // Should show file preview (or at least not crash) - // File might be rejected due to not being a real image, but UI should handle it - await page.waitForTimeout(1000) // Give UI time to process + // Should not crash - verify page is still functional + await expect( + page.locator('textarea[aria-label="Chat input"]'), + ).toBeVisible({ timeout: 3000 }) }) }) diff --git a/tests/e2e/history-restore.spec.ts b/tests/e2e/history-restore.spec.ts index f217747..3fd92af 100644 --- a/tests/e2e/history-restore.spec.ts +++ b/tests/e2e/history-restore.spec.ts @@ -35,17 +35,19 @@ test.describe("History and Session Restore", () => { // Find and click new chat button const newChatButton = page.locator( - 'button[aria-label*="New"], button:has(svg.lucide-plus), button:has-text("New Chat")', + '[data-testid="new-chat-button"], button[aria-label*="New"], button:has(svg.lucide-plus), button:has-text("New Chat")', ) - if ((await newChatButton.count()) > 0) { - await newChatButton.first().click() + // Skip test if new chat button doesn't exist + const buttonCount = await newChatButton.count() + test.skip(buttonCount === 0, "New chat button not available") - // Conversation should be cleared - await expect( - page.locator('text="Created your test diagram."'), - ).not.toBeVisible({ timeout: 5000 }) - } + await newChatButton.first().click() + + // Conversation should be cleared + await expect( + page.locator('text="Created your test diagram."'), + ).not.toBeVisible({ timeout: 5000 }) }) test("chat history sidebar shows past conversations", async ({ page }) => { @@ -59,17 +61,19 @@ test.describe("History and Session Restore", () => { 'button[aria-label*="History"]:not([disabled]), button:has(svg.lucide-history):not([disabled]), button:has(svg.lucide-menu):not([disabled]), button:has(svg.lucide-sidebar):not([disabled]), button:has(svg.lucide-panel-left):not([disabled])', ) - if ((await historyButton.count()) > 0) { - await historyButton.first().click() - await page.waitForTimeout(500) - } - // Test passes if no error - history feature may or may not be available + // Skip test if history button doesn't exist + const buttonCount = await historyButton.count() + test.skip(buttonCount === 0, "History button not available") + + await historyButton.first().click() + // Wait for sidebar/panel to appear or verify page still works + await expect( + page.locator('textarea[aria-label="Chat input"]'), + ).toBeVisible({ timeout: 3000 }) }) test("conversation persists after page reload", async ({ page }) => { - let requestCount = 0 await page.route("**/api/chat", async (route) => { - requestCount++ await route.fulfill({ status: 200, contentType: "text/event-stream", @@ -103,9 +107,10 @@ test.describe("History and Session Restore", () => { .locator("iframe") .waitFor({ state: "visible", timeout: 30000 }) - // Check if conversation persisted (depends on implementation) - // The message might or might not be there depending on local storage usage - await page.waitForTimeout(1000) + // Verify page is functional after reload + await expect( + page.locator('textarea[aria-label="Chat input"]'), + ).toBeVisible({ timeout: 10000 }) }) test("diagram state persists after reload", async ({ page }) => { @@ -136,9 +141,6 @@ test.describe("History and Session Restore", () => { timeout: 15000, }) - // Wait for diagram to render - await page.waitForTimeout(1000) - // Reload await page.reload({ waitUntil: "networkidle" }) await page @@ -204,13 +206,10 @@ test.describe("History and Session Restore", () => { .waitFor({ state: "visible", timeout: 30000 }) // Open settings - const settingsButton = page.locator( - 'button[aria-label*="Settings"], button:has(svg.lucide-settings)', - ) - await settingsButton.first().click() + const settingsButton = page.locator('[data-testid="settings-button"]') + await settingsButton.click() // Settings dialog should open - await page.waitForTimeout(500) await expect( page.locator('[role="dialog"], [role="menu"], form').first(), ).toBeVisible({ timeout: 5000 }) @@ -225,10 +224,9 @@ test.describe("History and Session Restore", () => { .waitFor({ state: "visible", timeout: 30000 }) // Open settings again - await settingsButton.first().click() + await settingsButton.click() // Settings should still be accessible - await page.waitForTimeout(500) await expect( page.locator('[role="dialog"], [role="menu"], form').first(), ).toBeVisible({ timeout: 5000 }) @@ -245,19 +243,21 @@ test.describe("History and Session Restore", () => { 'button[aria-label*="Model"], [data-testid="model-selector"], button:has-text("Claude")', ) - if ((await modelSelector.count()) > 0) { - const initialModel = await modelSelector.first().textContent() + // Skip test if model selector doesn't exist + const selectorCount = await modelSelector.count() + test.skip(selectorCount === 0, "Model selector not available") - // Reload page - await page.reload({ waitUntil: "networkidle" }) - await page - .locator("iframe") - .waitFor({ state: "visible", timeout: 30000 }) + const initialModel = await modelSelector.first().textContent() - // Check model is still selected - const modelAfterReload = await modelSelector.first().textContent() - expect(modelAfterReload).toBe(initialModel) - } + // Reload page + await page.reload({ waitUntil: "networkidle" }) + await page + .locator("iframe") + .waitFor({ state: "visible", timeout: 30000 }) + + // Check model is still selected + const modelAfterReload = await modelSelector.first().textContent() + expect(modelAfterReload).toBe(initialModel) }) test("handles localStorage quota exceeded gracefully", async ({ page }) => { diff --git a/tests/e2e/settings.spec.ts b/tests/e2e/settings.spec.ts index cb6f4ae..198f6ad 100644 --- a/tests/e2e/settings.spec.ts +++ b/tests/e2e/settings.spec.ts @@ -9,9 +9,7 @@ test.describe("Settings", () => { }) test("settings dialog opens", async ({ page }) => { - const settingsButton = page.locator( - 'button[aria-label*="settings"], button:has(svg[class*="settings"])', - ) + const settingsButton = page.locator('[data-testid="settings-button"]') await expect(settingsButton).toBeVisible() await settingsButton.click() @@ -20,9 +18,7 @@ test.describe("Settings", () => { }) test("language selection is available", async ({ page }) => { - const settingsButton = page.locator( - 'button[aria-label*="settings"], button:has(svg[class*="settings"])', - ) + const settingsButton = page.locator('[data-testid="settings-button"]') await settingsButton.click() const dialog = page.locator('[role="dialog"]') @@ -33,9 +29,7 @@ test.describe("Settings", () => { }) test("draw.io theme toggle exists", async ({ page }) => { - const settingsButton = page.locator( - 'button[aria-label*="settings"], button:has(svg[class*="settings"])', - ) + const settingsButton = page.locator('[data-testid="settings-button"]') await settingsButton.click() const dialog = page.locator('[role="dialog"]') diff --git a/tests/e2e/smoke.spec.ts b/tests/e2e/smoke.spec.ts index 4f8d590..3ba9160 100644 --- a/tests/e2e/smoke.spec.ts +++ b/tests/e2e/smoke.spec.ts @@ -37,9 +37,7 @@ test.describe("Smoke Tests", () => { .waitFor({ state: "visible", timeout: 30000 }) // Click settings button (gear icon) - const settingsButton = page.locator( - 'button[aria-label*="settings"], button:has(svg[class*="settings"])', - ) + const settingsButton = page.locator('[data-testid="settings-button"]') await expect(settingsButton).toBeVisible() await settingsButton.click() diff --git a/tests/e2e/theme.spec.ts b/tests/e2e/theme.spec.ts index 7292fce..66024fc 100644 --- a/tests/e2e/theme.spec.ts +++ b/tests/e2e/theme.spec.ts @@ -87,16 +87,6 @@ test.describe("Theme Switching", () => { ) await settingsButton.first().click() - // Settings dialog should be visible - await page.waitForTimeout(500) - - // Find any theme-related section - various possible labels - const themeSection = page - .locator('text="Draw.io Theme"') - .or(page.locator('text="draw.io"')) - .or(page.locator('text="Theme"')) - .or(page.locator('[aria-label*="theme"]')) - // At least some settings content should be visible await expect( page.locator('[role="dialog"], [role="menu"], form').first(), diff --git a/tests/unit/chat-helpers.test.ts b/tests/unit/chat-helpers.test.ts index 7687728..9bf9d47 100644 --- a/tests/unit/chat-helpers.test.ts +++ b/tests/unit/chat-helpers.test.ts @@ -1,85 +1,10 @@ // @vitest-environment node import { describe, expect, it } from "vitest" - -// Test the helper functions from chat route -// These are re-implemented here for testing since they're not exported - -const MAX_FILE_SIZE = 2 * 1024 * 1024 // 2MB -const MAX_FILES = 5 - -function validateFileParts(messages: any[]): { - valid: boolean - error?: string -} { - const lastMessage = messages[messages.length - 1] - const fileParts = - lastMessage?.parts?.filter((p: any) => p.type === "file") || [] - - if (fileParts.length > MAX_FILES) { - return { - valid: false, - error: `Too many files. Maximum ${MAX_FILES} allowed.`, - } - } - - for (const filePart of fileParts) { - if (filePart.url?.startsWith("data:")) { - const base64Data = filePart.url.split(",")[1] - if (base64Data) { - const sizeInBytes = Math.ceil((base64Data.length * 3) / 4) - if (sizeInBytes > MAX_FILE_SIZE) { - return { - valid: false, - error: `File exceeds ${MAX_FILE_SIZE / 1024 / 1024}MB limit.`, - } - } - } - } - } - - return { valid: true } -} - -function isMinimalDiagram(xml: string): boolean { - const stripped = xml.replace(/\s/g, "") - return !stripped.includes('id="2"') -} - -function replaceHistoricalToolInputs(messages: any[]): any[] { - return messages.map((msg) => { - if (msg.role !== "assistant" || !Array.isArray(msg.content)) { - return msg - } - const replacedContent = msg.content - .map((part: any) => { - if (part.type === "tool-call") { - const toolName = part.toolName - if ( - !part.input || - typeof part.input !== "object" || - Object.keys(part.input).length === 0 - ) { - return null - } - if ( - toolName === "display_diagram" || - toolName === "edit_diagram" - ) { - return { - ...part, - input: { - placeholder: - "[XML content replaced - see current diagram XML in system context]", - }, - } - } - } - return part - }) - .filter(Boolean) - return { ...msg, content: replacedContent } - }) -} +import { + isMinimalDiagram, + replaceHistoricalToolInputs, + validateFileParts, +} from "@/lib/chat-helpers" describe("validateFileParts", () => { it("returns valid for no files", () => { diff --git a/vitest.config.mts b/vitest.config.mts index f454801..790e1e8 100644 --- a/vitest.config.mts +++ b/vitest.config.mts @@ -7,5 +7,11 @@ export default defineConfig({ test: { environment: "jsdom", include: ["tests/**/*.test.{ts,tsx}"], + coverage: { + provider: "v8", + reporter: ["text", "json", "html"], + include: ["lib/**/*.ts", "app/**/*.ts", "app/**/*.tsx"], + exclude: ["**/*.test.ts", "**/*.test.tsx", "**/*.d.ts"], + }, }, })