From cf6eecc11e25c0a9fa9d72119725f7ace0eb594e Mon Sep 17 00:00:00 2001 From: James Scott Date: Fri, 5 Jun 2026 19:16:26 +0000 Subject: [PATCH 1/2] e2e: Fix flaky login and redirect tests Resolve flakiness in E2E tests by using deterministic waiting mechanisms for backend API responses. - Login Test (`login.spec.ts`): - Make the syncing state screenshot test deterministic by intercepting the `/v1/users/me/ping` API request and blocking it using a Promise until both light and dark theme screenshots are captured. This prevents the UI from prematurely transitioning to the idle state, which was causing race conditions and snapshot mismatches (especially in WebKit). - Redirect Tests (`feature-page.spec.ts`, `utils.ts`): - Synchronize E2E tests by waiting for backend API GET responses before asserting redirects, rather than relying on page load events or timing out. - In `feature-page.spec.ts`: - Update `redirects for a moved feature` test to wait for both the redirect response (`old-feature`) and the redirect target response (`new-feature`), filtering both by `GET` method to avoid matching CORS `OPTIONS` preflights. This ensures the test doesn't assert the URL before the redirect target has finished loading, resolving failures on cold start cache misses. - Update `shows gone page for a split feature` to wait for the `before-split-feature` GET response before asserting the 410 URL. - In `utils.ts`: - Update `goTo404Page` helper to wait for the feature's GET response before asserting the 404 URL. This shifts network wait times to the safe 30-second default timeout of `waitForResponse`, preventing them from eating into the strict 5-second assertion timeout of `toHaveURL` in resource-constrained CI environments. --- e2e/tests/feature-page.spec.ts | 22 ++++++++++++++++++---- e2e/tests/login.spec.ts | 14 ++++++++++++-- e2e/tests/utils.ts | 7 +++++++ 3 files changed, 37 insertions(+), 6 deletions(-) diff --git a/e2e/tests/feature-page.spec.ts b/e2e/tests/feature-page.spec.ts index 980e02a60..2538b35a5 100644 --- a/e2e/tests/feature-page.spec.ts +++ b/e2e/tests/feature-page.spec.ts @@ -214,12 +214,19 @@ test('date range changes are preserved in the URL', async ({page}) => { test('redirects for a moved feature', async ({page, browserName}) => { test.skip(browserName === 'webkit', 'Skipping webkit due to flakiness'); - // Wait for the app to fetch the old feature data which triggers the redirect. - const responsePromise = page.waitForResponse(response => - response.url().includes('/v1/features/old-feature'), + const oldResponsePromise = page.waitForResponse( + response => + response.url().includes('/v1/features/old-feature') && + response.request().method() === 'GET', + ); + const newResponsePromise = page.waitForResponse( + response => + response.url().includes('/v1/features/new-feature') && + response.request().method() === 'GET', ); await page.goto('http://localhost:5555/features/old-feature'); - await responsePromise; + await oldResponsePromise; + await newResponsePromise; // Expect the URL to be updated to the new feature's URL. await expect(page).toHaveURL( @@ -248,7 +255,14 @@ test('redirects for a moved feature', async ({page, browserName}) => { }); test('shows gone page for a split feature', async ({page}) => { + // Wait for the API request to complete before checking for redirect. + const responsePromise = page.waitForResponse( + response => + response.url().includes('/v1/features/before-split-feature') && + response.request().method() === 'GET', + ); await page.goto('http://localhost:5555/features/before-split-feature'); + await responsePromise; // Expect to be redirected to the 'feature-gone-split' page. await expect(page).toHaveURL( diff --git a/e2e/tests/login.spec.ts b/e2e/tests/login.spec.ts index fd308bf24..8fe919fd9 100644 --- a/e2e/tests/login.spec.ts +++ b/e2e/tests/login.spec.ts @@ -28,9 +28,14 @@ test.describe('Login Component States', () => { test('displays spinner and is disabled during profile sync', async ({ page, }) => { - // Intercept the pingUser request to introduce a delay. + let resolvePing: (() => void) | undefined; + const pingPromise = new Promise(resolve => { + resolvePing = resolve; + }); + + // Intercept the pingUser request and hold it. await page.route('**/v1/users/me/ping', async route => { - await new Promise(resolve => setTimeout(resolve, 1000)); // Delay to ensure 'syncing' state is capturable + await pingPromise; await route.continue(); }); @@ -58,6 +63,11 @@ test.describe('Login Component States', () => { }, ); + // Resolve the ping to let the sync complete. + if (resolvePing) { + resolvePing(); + } + // Now, wait for the sync to complete and verify the final state. await expect(loginButton.locator('sl-spinner')).toBeHidden(); await expect(loginButton).not.toBeDisabled(); diff --git a/e2e/tests/utils.ts b/e2e/tests/utils.ts index b4345f452..79122238e 100644 --- a/e2e/tests/utils.ts +++ b/e2e/tests/utils.ts @@ -261,7 +261,14 @@ export async function freezeAnimations(page: Page) { } export async function goTo404Page(page: Page, query: string): Promise { + const responsePromise = page.waitForResponse( + response => + response.url().includes(`/v1/features/${query}`) && + response.request().method() === 'GET', + ); await page.goto(`${BASE_URL}/features/${query}`); + await responsePromise; + await expect(page).toHaveURL( `${BASE_URL}/errors-404/feature-not-found?q=${query}`, ); From 774143a3b1d6c79d66dddf541594a96a483e05c2 Mon Sep 17 00:00:00 2001 From: James Scott Date: Thu, 11 Jun 2026 14:43:39 +0000 Subject: [PATCH 2/2] final touchups --- e2e/tests/notification-channels.spec.ts | 54 +++++++++++++++++++- util/cmd/load_fake_data/main.go | 67 ++++++++++++++++++++++--- 2 files changed, 114 insertions(+), 7 deletions(-) diff --git a/e2e/tests/notification-channels.spec.ts b/e2e/tests/notification-channels.spec.ts index a7d5ffa5f..0d418986d 100644 --- a/e2e/tests/notification-channels.spec.ts +++ b/e2e/tests/notification-channels.spec.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import {test, expect} from '@playwright/test'; +import {test, expect, type Page} from '@playwright/test'; import { loginAsUser, BASE_URL, @@ -23,6 +23,14 @@ import { resetUserData, } from './utils'; +function waitForChannelsRefetch(page: Page) { + return page.waitForResponse( + response => + response.url().includes('/v1/users/me/notification-channels') && + response.request().method() === 'GET', + ); +} + test('redirects unauthenticated user to home and shows toast', async ({ page, }) => { @@ -37,8 +45,19 @@ test.describe('Notification Channels Page', () => { test.beforeEach(async ({page}) => { await resetUserData(); await loginAsUser(page, 'test user 1'); + + // Listen for the initial data load GET request + const initialLoad = page.waitForResponse( + response => + response.url().includes('/v1/users/me/notification-channels') && + response.request().method() === 'GET', + ); + await page.goto(`${BASE_URL}/settings/notification-channels`); await waitForSidebarLoaded(page); + + // Wait for the initial load to complete + await initialLoad; }); test.afterAll(async () => { @@ -114,8 +133,14 @@ test.describe('Notification Channels Page', () => { .getByRole('textbox', {name: 'Slack Webhook URL'}) .fill(webhookUrl); + // Setup refetch listener before click + const refetch = waitForChannelsRefetch(page); + await dialog.getByRole('button', {name: 'Create', exact: true}).click(); + // Wait for refetch to complete + await refetch; + // Verify it's in the list. await expect(dialog.locator('sl-dialog')).not.toBeVisible(); const channelItem = webhookPanel.locator('.channel-item', { @@ -123,6 +148,9 @@ test.describe('Notification Channels Page', () => { }); await expect(channelItem).toBeVisible(); + // Setup refetch listener before delete click + const deleteRefetch = waitForChannelsRefetch(page); + await channelItem.locator('sl-button[label="Delete"]').click(); const deleteDialog = webhookPanel.locator('sl-dialog[open]'); @@ -131,6 +159,9 @@ test.describe('Notification Channels Page', () => { .getByRole('button', {name: 'Delete', exact: true}) .click(); + // Wait for deletion refetch + await deleteRefetch; + // Verify it's gone. await expect(channelItem).not.toBeVisible(); }); @@ -165,8 +196,15 @@ test.describe('Notification Channels Page', () => { await dialog .getByRole('textbox', {name: 'Slack Webhook URL'}) .fill(originalUrl); + + // Setup refetch listener for initial creation + const createRefetch = waitForChannelsRefetch(page); + await dialog.getByRole('button', {name: 'Create', exact: true}).click(); + // Wait for creation refetch + await createRefetch; + // Verify it was created. await expect(dialog.locator('sl-dialog')).not.toBeVisible(); const originalItem = webhookPanel.locator('.channel-item', { @@ -193,8 +231,14 @@ test.describe('Notification Channels Page', () => { .getByRole('textbox', {name: 'Slack Webhook URL'}) .fill(updatedUrl); + // Setup refetch listener for save + const saveRefetch = waitForChannelsRefetch(page); + await dialog.getByRole('button', {name: 'Save', exact: true}).click(); + // Wait for save refetch + await saveRefetch; + // Verify it was updated. await expect(dialog.locator('sl-dialog')).not.toBeVisible(); const updatedItem = webhookPanel.locator('.channel-item', { @@ -209,9 +253,17 @@ test.describe('Notification Channels Page', () => { const deleteDialog = webhookPanel.locator('sl-dialog[open]'); await expect(deleteDialog).toBeVisible(); + + // Setup refetch listener for delete + const deleteRefetch = waitForChannelsRefetch(page); + await deleteDialog .getByRole('button', {name: 'Delete', exact: true}) .click(); + + // Wait for deletion refetch + await deleteRefetch; + await expect(updatedItem).not.toBeVisible(); }); }); diff --git a/util/cmd/load_fake_data/main.go b/util/cmd/load_fake_data/main.go index f4b965f72..d3d1b9963 100644 --- a/util/cmd/load_fake_data/main.go +++ b/util/cmd/load_fake_data/main.go @@ -238,7 +238,7 @@ func (h featuresHelper) Features() []gcpspanner.WebFeature { } func resetTestData(ctx context.Context, spannerClient *gcpspanner.Client, authClient *auth.Client) error { - slog.InfoContext(ctx, "Resetting test user saved searches and bookmarks...") + slog.InfoContext(ctx, "Resetting test user data...") userIDs := make([]string, len(testUserEmails)) for idx, email := range testUserEmails { userID, err := findUserIDByEmail(ctx, email, authClient) @@ -257,6 +257,30 @@ func resetTestData(ctx context.Context, spannerClient *gcpspanner.Client, authCl return nil } + slog.InfoContext(ctx, "Resetting test user saved searches and bookmarks...") + if err := resetSavedSearches(ctx, spannerClient, userIDs); err != nil { + return err + } + slog.InfoContext(ctx, "Deleted saved searches for test users", "count", len(userIDs)) + + slog.InfoContext(ctx, "Resetting test user subscriptions...") + if err := resetSubscriptions(ctx, spannerClient, userIDs); err != nil { + return err + } + slog.InfoContext(ctx, "Test user subscriptions reset.") + + slog.InfoContext(ctx, "Resetting test user notification channels...") + if err := resetNotificationChannels(ctx, spannerClient, userIDs); err != nil { + return err + } + slog.InfoContext(ctx, "Test user notification channels reset.") + + slog.InfoContext(ctx, "Test user data reset complete.") + + return nil +} + +func resetSavedSearches(ctx context.Context, spannerClient *gcpspanner.Client, userIDs []string) error { for _, userID := range userIDs { page, err := spannerClient.ListUserSavedSearches(ctx, userID, 1000, nil) if err != nil { @@ -275,10 +299,11 @@ func resetTestData(ctx context.Context, spannerClient *gcpspanner.Client, authCl } } } - slog.InfoContext(ctx, "Deleted saved searches for test users", "count", len(userIDs)) - // Reset subscriptions for each test user. - slog.InfoContext(ctx, "Resetting test user subscriptions...") + return nil +} + +func resetSubscriptions(ctx context.Context, spannerClient *gcpspanner.Client, userIDs []string) error { for _, userID := range userIDs { // We don't need to handle pagination here, assuming a test user won't have more than 1000 subscriptions. req := gcpspanner.ListSavedSearchSubscriptionsRequest{ @@ -300,9 +325,39 @@ func resetTestData(ctx context.Context, spannerClient *gcpspanner.Client, authCl } } } - slog.InfoContext(ctx, "Test user subscriptions reset.") - slog.InfoContext(ctx, "Test user data reset complete.") + return nil +} + +func resetNotificationChannels(ctx context.Context, spannerClient *gcpspanner.Client, userIDs []string) error { + for _, userID := range userIDs { + if userID == "" { + continue + } + + // List all channels for the user + channels, _, err := spannerClient.ListNotificationChannels(ctx, gcpspanner.ListNotificationChannelsRequest{ + UserID: userID, + PageSize: 1000, + PageToken: nil, + }) + if err != nil { + return fmt.Errorf("failed to list notification channels for user %s: %w", userID, err) + } + + for _, channel := range channels { + // Only delete non-email channels. + if channel.Type == gcpspanner.NotificationChannelTypeEmail { + continue + } + + err := spannerClient.DeleteNotificationChannel(ctx, channel.ID, userID) + if err != nil { + slog.WarnContext(ctx, "failed to delete notification channel, continuing", + "channelID", channel.ID, "userID", userID, "error", err) + } + } + } return nil }