diff --git a/IMPLEMENTATION_PLAN.md b/IMPLEMENTATION_PLAN.md index 5ca8bac..655a0fe 100644 --- a/IMPLEMENTATION_PLAN.md +++ b/IMPLEMENTATION_PLAN.md @@ -147,6 +147,11 @@ These are optional enhancements to improve E2E coverage. Not required for featur ## Revision History +- 2026-01-13: Fixed E2E test reliability issues: + - Race conditions: Changed to single worker execution (tests share test user state) + - Stale data: GET /api/user and GET /api/cycle/current now fetch fresh data from database instead of stale auth cache + - Timing: Replaced fixed waitForTimeout calls with retry-based Playwright assertions + - Mobile test: Fixed strict mode violation by using exact heading match - 2026-01-13: Marked notifications.spec.ts as redundant (notification preferences already covered in settings.spec.ts) - 2026-01-13: Added dark-mode.spec.ts with 2 E2E tests (system preference detection for light/dark mode) - 2026-01-13: Added 4 Garmin E2E tests (network error recovery for save, disconnect, status fetch, retry) diff --git a/e2e/dashboard.spec.ts b/e2e/dashboard.spec.ts index 5df483d..ff52fbb 100644 --- a/e2e/dashboard.spec.ts +++ b/e2e/dashboard.spec.ts @@ -129,12 +129,13 @@ test.describe("dashboard", () => { // Click the toggle await toggleCheckbox.click(); - // Wait a moment for the API call - await page.waitForTimeout(500); - - // Toggle should change state - const afterChecked = await toggleCheckbox.isChecked(); - expect(afterChecked).not.toBe(initialChecked); + // Wait for the checkbox state to change using retry-based assertion + // The API call completes and React re-renders asynchronously + if (initialChecked) { + await expect(toggleCheckbox).not.toBeChecked({ timeout: 5000 }); + } else { + await expect(toggleCheckbox).toBeChecked({ timeout: 5000 }); + } } else { test.skip(); } @@ -244,59 +245,39 @@ test.describe("dashboard", () => { }); test("displays cycle day in 'Day X' format", async ({ page }) => { + // Wait for dashboard to finish loading await page.waitForLoadState("networkidle"); - // Look for "Day" followed by a number - const cycleDayText = page.getByText(/Day \d+/i); - const hasCycleDay = await cycleDayText - .first() - .isVisible() - .catch(() => false); + // Wait for either cycle day or onboarding - both are valid states + // Use Playwright's expect with retry for reliable detection + const cycleDayText = page.getByText(/Day \d+/i).first(); + const onboarding = page.getByText(/set.*period|log.*period/i).first(); - // Either has cycle day or onboarding (both valid states) - if (!hasCycleDay) { - const onboarding = page.getByText(/set.*period|log.*period/i); - const hasOnboarding = await onboarding - .first() - .isVisible() - .catch(() => false); - expect(hasCycleDay || hasOnboarding).toBe(true); + try { + // First try waiting for cycle day with a short timeout + await expect(cycleDayText).toBeVisible({ timeout: 5000 }); + } catch { + // If no cycle day, expect onboarding banner + await expect(onboarding).toBeVisible({ timeout: 5000 }); } }); test("displays current phase name", async ({ page }) => { + // Wait for dashboard to finish loading await page.waitForLoadState("networkidle"); - // Look for phase names - const phaseNames = [ - "MENSTRUAL", - "FOLLICULAR", - "OVULATION", - "EARLY_LUTEAL", - "LATE_LUTEAL", - ]; - let foundPhase = false; + // Wait for either a phase name or onboarding - both are valid states + const phaseRegex = + /MENSTRUAL|FOLLICULAR|OVULATION|EARLY_LUTEAL|LATE_LUTEAL/i; + const phaseText = page.getByText(phaseRegex).first(); + const onboarding = page.getByText(/set.*period|log.*period/i).first(); - for (const phase of phaseNames) { - const phaseText = page.getByText(new RegExp(phase, "i")); - const isVisible = await phaseText - .first() - .isVisible() - .catch(() => false); - if (isVisible) { - foundPhase = true; - break; - } - } - - // Either has phase or shows onboarding - if (!foundPhase) { - const onboarding = page.getByText(/set.*period|log.*period/i); - const hasOnboarding = await onboarding - .first() - .isVisible() - .catch(() => false); - expect(foundPhase || hasOnboarding).toBe(true); + try { + // First try waiting for a phase name with a short timeout + await expect(phaseText).toBeVisible({ timeout: 5000 }); + } catch { + // If no phase, expect onboarding banner + await expect(onboarding).toBeVisible({ timeout: 5000 }); } }); @@ -381,81 +362,62 @@ test.describe("dashboard", () => { }); test("displays seed cycling recommendation", async ({ page }) => { + // Wait for dashboard to finish loading await page.waitForLoadState("networkidle"); - // Look for seed names (flax, pumpkin, sesame, sunflower) - const seedText = page.getByText(/flax|pumpkin|sesame|sunflower/i); - const hasSeeds = await seedText - .first() - .isVisible() - .catch(() => false); + // Wait for either seed info or onboarding - both are valid states + const seedText = page.getByText(/flax|pumpkin|sesame|sunflower/i).first(); + const onboarding = page.getByText(/set.*period|log.*period/i).first(); - // Either has seeds info or onboarding - if (!hasSeeds) { - const onboarding = page.getByText(/set.*period|log.*period/i); - const hasOnboarding = await onboarding - .first() - .isVisible() - .catch(() => false); - expect(hasSeeds || hasOnboarding).toBe(true); + try { + await expect(seedText).toBeVisible({ timeout: 5000 }); + } catch { + await expect(onboarding).toBeVisible({ timeout: 5000 }); } }); test("displays carbohydrate range", async ({ page }) => { + // Wait for dashboard to finish loading await page.waitForLoadState("networkidle"); - // Look for carb-related text - const carbText = page.getByText(/carb|carbohydrate/i); - const hasCarbs = await carbText - .first() - .isVisible() - .catch(() => false); + // Wait for either carb info or onboarding - both are valid states + const carbText = page.getByText(/carb|carbohydrate/i).first(); + const onboarding = page.getByText(/set.*period|log.*period/i).first(); - if (!hasCarbs) { - const onboarding = page.getByText(/set.*period|log.*period/i); - const hasOnboarding = await onboarding - .first() - .isVisible() - .catch(() => false); - expect(hasCarbs || hasOnboarding).toBe(true); + try { + await expect(carbText).toBeVisible({ timeout: 5000 }); + } catch { + await expect(onboarding).toBeVisible({ timeout: 5000 }); } }); test("displays keto guidance", async ({ page }) => { + // Wait for dashboard to finish loading await page.waitForLoadState("networkidle"); - // Look for keto-related text - const ketoText = page.getByText(/keto/i); - const hasKeto = await ketoText - .first() - .isVisible() - .catch(() => false); + // Wait for either keto info or onboarding - both are valid states + const ketoText = page.getByText(/keto/i).first(); + const onboarding = page.getByText(/set.*period|log.*period/i).first(); - if (!hasKeto) { - const onboarding = page.getByText(/set.*period|log.*period/i); - const hasOnboarding = await onboarding - .first() - .isVisible() - .catch(() => false); - expect(hasKeto || hasOnboarding).toBe(true); + try { + await expect(ketoText).toBeVisible({ timeout: 5000 }); + } catch { + await expect(onboarding).toBeVisible({ timeout: 5000 }); } }); test("displays nutrition section header", async ({ page }) => { + // Wait for dashboard to finish loading await page.waitForLoadState("networkidle"); - // Nutrition panel should have a header + // Wait for nutrition header or text const nutritionHeader = page.getByRole("heading", { name: /nutrition/i }); - const hasHeader = await nutritionHeader.isVisible().catch(() => false); + const nutritionText = page.getByText(/nutrition/i).first(); - if (!hasHeader) { - // May be text label instead of heading - const nutritionText = page.getByText(/nutrition/i); - const hasText = await nutritionText - .first() - .isVisible() - .catch(() => false); - expect(hasHeader || hasText).toBe(true); + try { + await expect(nutritionHeader).toBeVisible({ timeout: 5000 }); + } catch { + await expect(nutritionText).toBeVisible({ timeout: 5000 }); } }); }); diff --git a/e2e/garmin.spec.ts b/e2e/garmin.spec.ts index 70c4b20..2792496 100644 --- a/e2e/garmin.spec.ts +++ b/e2e/garmin.spec.ts @@ -40,6 +40,19 @@ test.describe("garmin connection", () => { await page.waitForURL("/", { timeout: 10000 }); await page.goto("/settings/garmin"); await page.waitForLoadState("networkidle"); + + // Clean up: Disconnect if already connected to ensure clean state + const disconnectButton = page.getByRole("button", { + name: /disconnect/i, + }); + const isConnected = await disconnectButton.isVisible().catch(() => false); + + if (isConnected) { + await disconnectButton.click(); + await page.waitForTimeout(1000); + // Wait for disconnect to complete + await page.waitForLoadState("networkidle"); + } }); test("shows not connected initially for new user", async ({ page }) => { diff --git a/e2e/mobile.spec.ts b/e2e/mobile.spec.ts index d034976..b90e671 100644 --- a/e2e/mobile.spec.ts +++ b/e2e/mobile.spec.ts @@ -140,15 +140,18 @@ test.describe("mobile viewport", () => { const viewportSize = page.viewportSize(); expect(viewportSize?.width).toBe(375); - // Calendar heading should be visible - const heading = page.getByRole("heading", { name: /calendar/i }); - await expect(heading).toBeVisible(); + // Calendar page title heading should be visible (exact match to avoid "Calendar Subscription") + const heading = page.getByRole("heading", { + name: "Calendar", + exact: true, + }); + await expect(heading).toBeVisible({ timeout: 10000 }); // Calendar grid should be visible const calendarGrid = page .getByRole("grid") .or(page.locator('[data-testid="month-view"]')); - await expect(calendarGrid).toBeVisible(); + await expect(calendarGrid).toBeVisible({ timeout: 5000 }); // Month navigation should be visible const monthYear = page.getByText( diff --git a/e2e/plan.spec.ts b/e2e/plan.spec.ts index 783b1d0..46e18dc 100644 --- a/e2e/plan.spec.ts +++ b/e2e/plan.spec.ts @@ -53,31 +53,32 @@ test.describe("plan page", () => { test("shows current cycle status section", async ({ page }) => { await page.waitForLoadState("networkidle"); - // Look for Current Status section + // Wait for page to finish loading - look for Current Status or error state const statusSection = page.getByRole("heading", { name: "Current Status", }); - const hasStatus = await statusSection.isVisible().catch(() => false); + // Use text content to find error alert (avoid Next.js route announcer) + const errorAlert = page.getByText(/error:/i); - if (hasStatus) { - await expect(statusSection).toBeVisible(); + try { + // Wait for Current Status section to be visible (data loaded successfully) + await expect(statusSection).toBeVisible({ timeout: 10000 }); // Should show day number - await expect(page.getByText(/day \d+/i)).toBeVisible(); + await expect(page.getByText(/day \d+/i)).toBeVisible({ timeout: 5000 }); // Should show training type - await expect(page.getByText(/training type:/i)).toBeVisible(); + await expect(page.getByText(/training type:/i)).toBeVisible({ + timeout: 5000, + }); // Should show weekly limit - await expect(page.getByText(/weekly limit:/i)).toBeVisible(); - } else { - // If no status, should see loading or error state - const loading = page.getByText(/loading/i); - const error = page.getByRole("alert"); - const hasLoading = await loading.isVisible().catch(() => false); - const hasError = await error.isVisible().catch(() => false); - - expect(hasLoading || hasError).toBe(true); + await expect(page.getByText(/weekly limit:/i)).toBeVisible({ + timeout: 5000, + }); + } catch { + // If status section not visible, check for error alert + await expect(errorAlert).toBeVisible({ timeout: 5000 }); } }); diff --git a/e2e/pocketbase-harness.ts b/e2e/pocketbase-harness.ts index ab26991..804eaff 100644 --- a/e2e/pocketbase-harness.ts +++ b/e2e/pocketbase-harness.ts @@ -140,6 +140,12 @@ async function addUserFields(pb: PocketBase): Promise { * Sets up API rules for collections to allow user access. */ async function setupApiRules(pb: PocketBase): Promise { + // Allow users to update their own user record + const usersCollection = await pb.collections.getOne("users"); + await pb.collections.update(usersCollection.id, { + updateRule: "id = @request.auth.id", + }); + // Allow users to read/write their own period_logs const periodLogs = await pb.collections.getOne("period_logs"); await pb.collections.update(periodLogs.id, { @@ -202,6 +208,7 @@ async function createTestUser( verified: true, lastPeriodDate, cycleLength: 28, + notificationTime: "07:00", timezone: "UTC", }); diff --git a/e2e/settings.spec.ts b/e2e/settings.spec.ts index 2496380..fc91db5 100644 --- a/e2e/settings.spec.ts +++ b/e2e/settings.spec.ts @@ -435,10 +435,12 @@ test.describe("settings", () => { const newValue = originalValue === "08:00" ? "09:00" : "08:00"; await notificationTimeInput.fill(newValue); - // Save + // Save and wait for success toast const saveButton = page.getByRole("button", { name: /save/i }); await saveButton.click(); - await page.waitForTimeout(1500); + await expect(page.getByText(/settings saved successfully/i)).toBeVisible({ + timeout: 10000, + }); // Reload the page await page.reload(); @@ -453,7 +455,9 @@ test.describe("settings", () => { // Restore original value await notificationTimeAfter.fill(originalValue); await saveButton.click(); - await page.waitForTimeout(500); + await expect(page.getByText(/settings saved successfully/i)).toBeVisible({ + timeout: 10000, + }); }); test("timezone changes persist after page reload", async ({ page }) => { @@ -475,10 +479,12 @@ test.describe("settings", () => { : "America/New_York"; await timezoneInput.fill(newValue); - // Save + // Save and wait for success toast const saveButton = page.getByRole("button", { name: /save/i }); await saveButton.click(); - await page.waitForTimeout(1500); + await expect(page.getByText(/settings saved successfully/i)).toBeVisible({ + timeout: 10000, + }); // Reload the page await page.reload(); @@ -493,7 +499,9 @@ test.describe("settings", () => { // Restore original value await timezoneAfter.fill(originalValue); await saveButton.click(); - await page.waitForTimeout(500); + await expect(page.getByText(/settings saved successfully/i)).toBeVisible({ + timeout: 10000, + }); }); test("multiple settings changes persist after page reload", async ({ @@ -536,10 +544,12 @@ test.describe("settings", () => { await notificationTimeInput.fill(newNotificationTime); await timezoneInput.fill(newTimezone); - // Save all changes at once + // Save all changes at once and wait for success toast const saveButton = page.getByRole("button", { name: /save/i }); await saveButton.click(); - await page.waitForTimeout(1500); + await expect(page.getByText(/settings saved successfully/i)).toBeVisible({ + timeout: 10000, + }); // Reload the page await page.reload(); @@ -561,7 +571,9 @@ test.describe("settings", () => { await notificationTimeAfter.fill(originalNotificationTime); await timezoneAfter.fill(originalTimezone); await saveButton.click(); - await page.waitForTimeout(500); + await expect(page.getByText(/settings saved successfully/i)).toBeVisible({ + timeout: 10000, + }); }); test("cycle length persistence verifies exact saved value", async ({ @@ -582,10 +594,12 @@ test.describe("settings", () => { const newValue = originalValue === "28" ? "31" : "28"; await cycleLengthInput.fill(newValue); - // Save + // Save and wait for success toast const saveButton = page.getByRole("button", { name: /save/i }); await saveButton.click(); - await page.waitForTimeout(1500); + await expect(page.getByText(/settings saved successfully/i)).toBeVisible({ + timeout: 10000, + }); // Reload the page await page.reload(); @@ -600,7 +614,9 @@ test.describe("settings", () => { // Restore original value await cycleLengthAfter.fill(originalValue); await saveButton.click(); - await page.waitForTimeout(500); + await expect(page.getByText(/settings saved successfully/i)).toBeVisible({ + timeout: 10000, + }); }); test("settings form shows correct values after save without reload", async ({ diff --git a/playwright.config.ts b/playwright.config.ts index 5cbb6e1..26cd9a4 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -22,8 +22,9 @@ export default defineConfig({ // Retry failed tests on CI only retries: process.env.CI ? 2 : 0, - // Limit parallel workers on CI to avoid resource issues - workers: process.env.CI ? 1 : undefined, + // Run tests sequentially since all tests share the same test user + // Parallel execution causes race conditions when tests modify user state + workers: 1, // Reporter configuration reporter: [["html", { open: "never" }], ["list"]], diff --git a/src/app/api/cycle/current/route.test.ts b/src/app/api/cycle/current/route.test.ts index 44a95d2..2484b8c 100644 --- a/src/app/api/cycle/current/route.test.ts +++ b/src/app/api/cycle/current/route.test.ts @@ -9,9 +9,29 @@ import type { User } from "@/types"; // Module-level variable to control mock user in tests let currentMockUser: User | null = null; +// Create mock PocketBase getOne function that returns fresh user data +const mockPbGetOne = vi.fn().mockImplementation(() => { + if (!currentMockUser) { + throw new Error("User not found"); + } + return Promise.resolve({ + id: currentMockUser.id, + email: currentMockUser.email, + lastPeriodDate: currentMockUser.lastPeriodDate?.toISOString(), + cycleLength: currentMockUser.cycleLength, + }); +}); + +// Create mock PocketBase client +const mockPb = { + collection: vi.fn(() => ({ + getOne: mockPbGetOne, + })), +}; + // Mock PocketBase client for database operations vi.mock("@/lib/pocketbase", () => ({ - createPocketBaseClient: vi.fn(() => ({})), + createPocketBaseClient: vi.fn(() => mockPb), loadAuthFromCookies: vi.fn(), isAuthenticated: vi.fn(() => currentMockUser !== null), getCurrentUser: vi.fn(() => currentMockUser), @@ -24,7 +44,7 @@ vi.mock("@/lib/auth-middleware", () => ({ if (!currentMockUser) { return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); } - return handler(request, currentMockUser); + return handler(request, currentMockUser, mockPb); }; }), })); diff --git a/src/app/api/cycle/current/route.ts b/src/app/api/cycle/current/route.ts index 1c47aa4..49eaf7c 100644 --- a/src/app/api/cycle/current/route.ts +++ b/src/app/api/cycle/current/route.ts @@ -40,9 +40,18 @@ function getDaysUntilNextPhase(cycleDay: number, cycleLength: number): number { return nextPhaseStart - cycleDay; } -export const GET = withAuth(async (_request, user) => { +export const GET = withAuth(async (_request, user, pb) => { + // Fetch fresh user data from database to get latest values + // The user param from withAuth is from auth store cache which may be stale + const freshUser = await pb.collection("users").getOne(user.id); + // Validate user has required cycle data - if (!user.lastPeriodDate) { + const lastPeriodDate = freshUser.lastPeriodDate + ? new Date(freshUser.lastPeriodDate as string) + : null; + const cycleLength = (freshUser.cycleLength as number) || 28; + + if (!lastPeriodDate) { return NextResponse.json( { error: @@ -53,20 +62,16 @@ export const GET = withAuth(async (_request, user) => { } // Calculate current cycle position - const cycleDay = getCycleDay( - user.lastPeriodDate, - user.cycleLength, - new Date(), - ); - const phase = getPhase(cycleDay, user.cycleLength); + const cycleDay = getCycleDay(lastPeriodDate, cycleLength, new Date()); + const phase = getPhase(cycleDay, cycleLength); const phaseConfig = getPhaseConfig(phase); - const daysUntilNextPhase = getDaysUntilNextPhase(cycleDay, user.cycleLength); + const daysUntilNextPhase = getDaysUntilNextPhase(cycleDay, cycleLength); return NextResponse.json({ cycleDay, phase, phaseConfig, daysUntilNextPhase, - cycleLength: user.cycleLength, + cycleLength, }); }); diff --git a/src/app/api/user/route.test.ts b/src/app/api/user/route.test.ts index 60b8d0f..3dc840d 100644 --- a/src/app/api/user/route.test.ts +++ b/src/app/api/user/route.test.ts @@ -12,10 +12,28 @@ let currentMockUser: User | null = null; // Track PocketBase update calls const mockPbUpdate = vi.fn().mockResolvedValue({}); +// Track PocketBase getOne calls - returns the current mock user data +const mockPbGetOne = vi.fn().mockImplementation(() => { + if (!currentMockUser) { + throw new Error("User not found"); + } + return Promise.resolve({ + id: currentMockUser.id, + email: currentMockUser.email, + garminConnected: currentMockUser.garminConnected, + lastPeriodDate: currentMockUser.lastPeriodDate?.toISOString(), + cycleLength: currentMockUser.cycleLength, + notificationTime: currentMockUser.notificationTime, + timezone: currentMockUser.timezone, + activeOverrides: currentMockUser.activeOverrides, + }); +}); + // Create mock PocketBase client const mockPb = { collection: vi.fn(() => ({ update: mockPbUpdate, + getOne: mockPbGetOne, })), }; @@ -55,6 +73,7 @@ describe("GET /api/user", () => { vi.clearAllMocks(); currentMockUser = null; mockPbUpdate.mockClear(); + mockPbGetOne.mockClear(); }); it("returns user profile when authenticated", async () => { @@ -133,6 +152,7 @@ describe("PATCH /api/user", () => { vi.clearAllMocks(); currentMockUser = null; mockPbUpdate.mockClear(); + mockPbGetOne.mockClear(); }); // Helper to create mock request with JSON body diff --git a/src/app/api/user/route.ts b/src/app/api/user/route.ts index 0a56fa7..c43c082 100644 --- a/src/app/api/user/route.ts +++ b/src/app/api/user/route.ts @@ -13,23 +13,28 @@ const TIME_FORMAT_REGEX = /^([01]\d|2[0-3]):([0-5]\d)$/; /** * GET /api/user * Returns the authenticated user's profile. + * Fetches fresh data from database to ensure updates are reflected. * Excludes sensitive fields like encrypted tokens. */ -export const GET = withAuth(async (_request, user, _pb) => { +export const GET = withAuth(async (_request, user, pb) => { + // Fetch fresh user data from database to get latest values + // The user param from withAuth is from auth store cache which may be stale + const freshUser = await pb.collection("users").getOne(user.id); + // Format date for consistent API response - const lastPeriodDate = user.lastPeriodDate - ? user.lastPeriodDate.toISOString().split("T")[0] + const lastPeriodDate = freshUser.lastPeriodDate + ? new Date(freshUser.lastPeriodDate as string).toISOString().split("T")[0] : null; return NextResponse.json({ - id: user.id, - email: user.email, - garminConnected: user.garminConnected, - cycleLength: user.cycleLength, + id: freshUser.id, + email: freshUser.email, + garminConnected: freshUser.garminConnected ?? false, + cycleLength: freshUser.cycleLength, lastPeriodDate, - notificationTime: user.notificationTime, - timezone: user.timezone, - activeOverrides: user.activeOverrides, + notificationTime: freshUser.notificationTime, + timezone: freshUser.timezone, + activeOverrides: freshUser.activeOverrides ?? [], }); });