Skip to content

fix(e2e): repair failing weekly test suite #696

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bramwelt wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix(e2e): repair failing weekly test suite #696

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3804238
fix(e2e): repair failing weekly test suite
bramwelt May 12, 2026
9c30583
fix(review): address PR #696 review feedback
bramwelt May 13, 2026
91989df
fix(e2e): use existing testid in email CTR test
bramwelt May 13, 2026
b9ad717
Merge branch 'main' into fix/weekly-e2e-670
bramwelt May 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions apps/lfx-one/e2e/event-selection-robust.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// SPDX-License-Identifier: MIT

import { expect, Page, test } from '@playwright/test';
import { DEFAULT_LENS, LENS_COOKIE_KEY } from '@lfx-one/shared/constants';

const EMPTY_EVENTS_RESPONSE = { data: [], total: 0, pageSize: 10, offset: 0 };
const EMPTY_COUNTRIES_RESPONSE = { data: [] };
Expand Down Expand Up @@ -55,6 +56,8 @@ async function openDialogWithEmptyEvents(
routeOptions: { probeTotal?: number; mainStatus?: number } = {}
) {
await mockEventRoutes(page, routeOptions);
// Ensure "me" lens is active so lensRedirectGuard doesn't redirect to /foundation/events
await page.context().addCookies([{ name: LENS_COOKIE_KEY, value: DEFAULT_LENS, domain: 'localhost', path: '/' }]);

@MRashad26 MRashad26 May 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem: domain: 'localhost' is hard-coded against the dev server's host. playwright.config.ts currently sets baseURL: 'http://localhost:4200' so this works today, but the cookie is only attached for requests whose host is localhost — it will not be set when the same suite runs against an override target (preview deployment, staging, a remote CI runner using a different hostname). When that happens the lensRedirectGuard redirect kicks back in and the suite regresses, with the failure looking identical to the bug this PR fixes.

Why this is a problem:

  1. The fix is implicit on the test environment — anyone running these tests via BASE_URL=https://preview-xyz.lfx.dev ... (or a similar override) silently loses the cookie protection without a clear error. The next "these tests flake again" investigation has to rediscover what this PR just rediscovered.
  2. The constants LENS_COOKIE_KEY and DEFAULT_LENS are already imported from shared — only the domain is environment-specific. Deriving it from the runtime URL keeps the rest of the cookie definition pinned to the same source of truth.
  3. Other tests in the file may pick up the same redirect bug later — every page.goto('/me/events', ...) is the same vulnerability. If a future test bypasses openDialogWithEmptyEvents() and calls page.goto directly, the cookie won't be set and the flake returns. (Today, all 19 callers go through this helper — confirmed by grep. But that's a constraint the future has to remember to honor.)

Suggested fix: Derive the domain from Playwright's resolved baseURL instead of hard-coding 'localhost'. Two reasonable shapes:

Option A — beforeEach (or fixture)-level cookie setup, so every test in the file is covered, not just ones that go through openDialogWithEmptyEvents():

test.beforeEach(async ({ context, baseURL }) => {
  const domain = baseURL ? new URL(baseURL).hostname : 'localhost';
  await context.addCookies([{ name: LENS_COOKIE_KEY, value: DEFAULT_LENS, domain, path: '/' }]);
});

Option B — keep the per-helper call but derive the domain from baseURL:

async function openDialogWithEmptyEvents(
  page: Page,
  tab: 'visa-letters' | 'travel-funding' = 'visa-letters',
  routeOptions: { probeTotal?: number; mainStatus?: number } = {}
) {
  await mockEventRoutes(page, routeOptions);
  const baseURL = page.context()._options.baseURL ?? 'http://localhost:4200';
  const domain = new URL(baseURL).hostname;
  await page.context().addCookies([{ name: LENS_COOKIE_KEY, value: DEFAULT_LENS, domain, path: '/' }]);
  await page.goto('/me/events', { waitUntil: 'domcontentloaded' });
  // ...
}

A fixture (Option A) is the cleaner long-term shape — it also stops covers-only-this-helper from being a hidden constraint the file enforces.

await page.goto('/me/events', { waitUntil: 'domcontentloaded' });
Comment thread
bramwelt marked this conversation as resolved.
await expect(page).not.toHaveURL(/auth0\.com/);
await page.getByTestId(`filter-pill-${tab}`).click();
Expand Down
17 changes: 11 additions & 6 deletions apps/lfx-one/e2e/marketing-dashboard.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,8 @@ test.describe('Website Visits Drawer', () => {
});

test('shows stats section with data', async ({ page }) => {
await openDrawer(page, 'marketing-card-website-visits', 'website-visits-drawer-stats');
await openDrawer(page, 'marketing-card-website-visits', 'website-visits-drawer-content');
await expect(page.locator('[data-testid="website-visits-drawer-stats"]')).toBeVisible({ timeout: DATA_LOAD_TIMEOUT });

@MRashad26 MRashad26 May 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem: The fix splits a single "open and wait for data" call into two steps and duplicates the second step at each callsite:

await openDrawer(page, 'marketing-card-website-visits', 'website-visits-drawer-content');
await expect(page.locator('[data-testid="website-visits-drawer-stats"]')).toBeVisible({ timeout: DATA_LOAD_TIMEOUT });

The same pair appears 5 times in this file (L194-195, L224-225, L248-249, L273-274, L297-298). The reason — drawer-content is always visible once the drawer opens, drawer-stats appears only after drawerLoading() clears — is correct, but the cleanup landed at the wrong layer: the helper now under-promises ("drawer opened") and every caller hand-rolls the data-load wait.

Why this is a problem:

  1. 5 callsites means 5 places to drift — if the timeout policy changes (e.g. promote DATA_LOAD_TIMEOUT to per-test via fixture, or back-off on retry), all five have to be updated in lockstep. Trivially missable in code review.
  2. The openDrawer JSDoc on L46-48 ("Waits for the drawer content to be visible before returning") now describes "drawer opened", not "drawer ready to test" — a future reader who reuses openDrawer and stops there will write tests that race with the loading skeleton. The bug this PR fixes is exactly that mistake; the public-helper shape now invites it.
  3. Naming asymmetry — the second argument is drawerContentTestId, but the callsite that matters most for assertions is drawerStatsTestId. Threading the right concept through the helper makes it harder to forget.

Suggested fix: Push the data-load wait back into a helper so each test reads as one intent:

/**
 * Open a drawer and wait for its data to load (skeleton cleared).
 * dataTestId is the element behind the drawer's loading gate (typically `*-drawer-stats`).
 */
async function openDrawerAndWaitForData(
  page: Page,
  cardTestId: string,
  drawerContentTestId: string,
  dataTestId: string,
): Promise<void> {
  await openDrawer(page, cardTestId, drawerContentTestId);
  await expect(page.locator(`[data-testid="${dataTestId}"]`)).toBeVisible({ timeout: DATA_LOAD_TIMEOUT });
}

Callsite collapses to:

await openDrawerAndWaitForData(
  page,
  'marketing-card-website-visits',
  'website-visits-drawer-content',
  'website-visits-drawer-stats',
);

The handful of callers that don't need the data wait (the close-button tests, for example) keep using bare openDrawer. The naming asymmetry is gone — "open drawer, wait for data" reads as one operation, which is the intent.

Not a blocker for landing this PR — the duplication is small and the tests are correct. Worth a follow-up cleanup so the next person adding a drawer test inherits the right pattern.

const statCards = page.locator('[data-testid="website-visits-drawer-stats"]').locator('lfx-card');
await expect(statCards).toHaveCount(2);
});
Expand Down Expand Up @@ -220,8 +221,9 @@ test.describe('Email CTR Drawer', () => {
});

test('shows stats and chart sections', async ({ page }) => {
await openDrawer(page, 'marketing-card-email-ctr', 'email-ctr-drawer-stats');
await expect(page.locator('[data-testid="email-ctr-drawer-chart-section"]')).toBeVisible();
await openDrawer(page, 'marketing-card-email-ctr', 'email-ctr-drawer-content');
await expect(page.locator('[data-testid="email-ctr-drawer-stats"]')).toBeVisible({ timeout: DATA_LOAD_TIMEOUT });
await expect(page.locator('[data-testid="email-ctr-drawer-email-section"]')).toBeVisible();
});
Comment on lines 223 to 227

test('closes when close button is clicked', async ({ page }) => {
Expand All @@ -243,7 +245,8 @@ test.describe('Paid Social Reach Drawer', () => {
});

test('shows ROAS and impressions charts', async ({ page }) => {
await openDrawer(page, 'marketing-card-paid-social-reach', 'paid-social-reach-drawer-stats');
await openDrawer(page, 'marketing-card-paid-social-reach', 'paid-social-reach-drawer-content');
await expect(page.locator('[data-testid="paid-social-reach-drawer-stats"]')).toBeVisible({ timeout: DATA_LOAD_TIMEOUT });
await expect(page.locator('[data-testid="paid-social-reach-drawer-roas-chart-section"]')).toBeVisible();
await expect(page.locator('[data-testid="paid-social-reach-drawer-chart-section"]')).toBeVisible();
});
Expand All @@ -267,7 +270,8 @@ test.describe('Social Media Drawer', () => {
});

test('shows stats and platform breakdown', async ({ page }) => {
await openDrawer(page, 'marketing-card-social-media', 'social-media-drawer-stats');
await openDrawer(page, 'marketing-card-social-media', 'social-media-drawer-content');
await expect(page.locator('[data-testid="social-media-drawer-stats"]')).toBeVisible({ timeout: DATA_LOAD_TIMEOUT });
await expect(page.locator('[data-testid="social-media-drawer-platforms-section"]')).toBeVisible();
});

Expand All @@ -290,7 +294,8 @@ test.describe('Member Growth Drawer', () => {
});

test('shows stats section', async ({ page }) => {
await openDrawer(page, 'flywheel-pulse-member-growth', 'member-acquisition-drawer-stats');
await openDrawer(page, 'flywheel-pulse-member-growth', 'member-acquisition-drawer-content');
await expect(page.locator('[data-testid="member-acquisition-drawer-stats"]')).toBeVisible({ timeout: DATA_LOAD_TIMEOUT });
const statCards = page.locator('[data-testid="member-acquisition-drawer-stats"]').locator('lfx-card');
await expect(statCards).toHaveCount(3);
});
Expand Down
Loading