diff --git a/scripts/__tests__/settle-helper.test.js b/scripts/__tests__/settle-helper.test.js new file mode 100644 index 00000000..b7017e85 --- /dev/null +++ b/scripts/__tests__/settle-helper.test.js @@ -0,0 +1,99 @@ +/** + * `settleFrames` must have exactly one implementation, and must not regain a name that + * promises more than it does (#739). + * + * It was `waitForUIStability`, copy-pasted into five messaging specs, and the name caused a + * real hard failure: T009 used it to wait out a `behavior: 'smooth'` scroll and measured a + * scroll that had barely started, failing with 2393px remaining against a 100px threshold — + * on chromium in one run and firefox in the next. It observes nothing; the frames elapse + * whether the UI settled or not. + * + * Five copies is also five places to fix when the lesson is next learned, which is why this + * asserts there is exactly one. + */ + +'use strict'; + +const { describe, it } = require('node:test'); +const assert = require('node:assert'); +const fs = require('node:fs'); +const path = require('node:path'); + +const REPO_ROOT = path.resolve(__dirname, '..', '..'); +const E2E = path.join(REPO_ROOT, 'tests', 'e2e'); +const UTIL = path.join(E2E, 'utils', 'settle.ts'); + +function walk(dir, acc = []) { + for (const e of fs.readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, e.name); + if (e.isDirectory()) walk(full, acc); + else if (e.name.endsWith('.ts')) acc.push(full); + } + return acc; +} + +describe('the settle helper has one honest implementation (#739)', () => { + const files = walk(E2E); + + it('the scan found the spec tree', () => { + assert.ok( + files.length > 20, + `only ${files.length} .ts files under tests/e2e — stale path?` + ); + }); + + it('exactly one file defines it', () => { + const definers = files + .filter((f) => + /export async function settleFrames|async function settleFrames/.test( + fs.readFileSync(f, 'utf8') + ) + ) + .map((f) => f.replace(REPO_ROOT + '/', '')); + assert.deepStrictEqual( + definers, + ['tests/e2e/utils/settle.ts'], + 'settleFrames is defined somewhere other than the shared util, or in more than one ' + + 'place. Five copies of its predecessor is what made #739 five bugs instead of one.' + ); + }); + + it('nothing reintroduces the name that lied', () => { + const offenders = files + .filter((f) => f !== UTIL) + .filter((f) => + /function waitForUIStability/.test(fs.readFileSync(f, 'utf8')) + ) + .map((f) => f.replace(REPO_ROOT + '/', '')); + assert.deepStrictEqual( + offenders, + [], + 'a helper called waitForUIStability is back. It cannot wait for stability — it ' + + 'advances N animation frames and observes nothing. Use settleFrames, or better, ' + + 'assert the outcome with expect.poll.' + ); + }); + + it('the util documents the rule that caused #739', () => { + const src = fs.readFileSync(UTIL, 'utf8'); + assert.ok( + /does not retry|expect\.poll/.test(src), + 'the util no longer warns against putting it before a non-retrying measurement — ' + + 'which is the single mistake that produced #739' + ); + }); + + it('the control can fail — the definition detector works', () => { + const fake = 'export async function settleFrames(page) {}'; + assert.ok( + /export async function settleFrames|async function settleFrames/.test( + fake + ), + 'the detector cannot see a definition it should see' + ); + assert.ok( + !/function waitForUIStability/.test(fake), + 'the detector false-positives' + ); + }); +}); diff --git a/tests/e2e/messaging/gdpr-compliance.spec.ts b/tests/e2e/messaging/gdpr-compliance.spec.ts index 881af3c7..4caaa7b8 100644 --- a/tests/e2e/messaging/gdpr-compliance.spec.ts +++ b/tests/e2e/messaging/gdpr-compliance.spec.ts @@ -18,6 +18,7 @@ */ import { test, expect, type Page, type Browser } from '@playwright/test'; +import { settleFrames } from '../utils/settle'; import { seedIsolatedConversation, deleteIsolatedConversation, @@ -29,27 +30,6 @@ import { // and means a real account deletion only ever destroys this test's throwaway user. test.describe.configure({ mode: 'parallel' }); -/** - * Wait for UI to stabilize after navigation or interaction. - */ -async function waitForUIStability(page: Page) { - await page.waitForLoadState('domcontentloaded'); - await page.waitForFunction( - () => { - return new Promise((resolve) => { - let stableFrames = 0; - const checkStability = () => { - stableFrames++; - if (stableFrames >= 3) resolve(true); - else requestAnimationFrame(checkStability); - }; - requestAnimationFrame(checkStability); - }); - }, - { timeout: 15000 } - ); -} - /** A browser context opened on /account authenticated as the isolated viewer. */ interface OpenedAccount { page: Page; @@ -115,7 +95,7 @@ async function openAccountAsViewer( state: 'visible', timeout: 30000, }); - await waitForUIStability(page); + await settleFrames(page); await dismissCookieBanner(page); return { page, close: () => context.close() }; @@ -370,7 +350,7 @@ test.describe('GDPR Account Deletion', () => { const modal = page.getByRole('dialog'); await expect(modal).toBeVisible(); - await waitForUIStability(page); + await settleFrames(page); // Use ID selector for the confirmation input (more reliable than label) const confirmInput = page.locator('#confirmation-input'); @@ -439,7 +419,7 @@ test.describe('GDPR Account Deletion', () => { const modal = page.getByRole('dialog'); await expect(modal).toBeVisible(); - await waitForUIStability(page); + await settleFrames(page); const confirmInput = page.locator('#confirmation-input'); const confirmButton = modal.getByRole('button', { @@ -478,7 +458,7 @@ test.describe('GDPR Account Deletion', () => { const modal = page.getByRole('dialog'); await expect(modal).toBeVisible(); - await waitForUIStability(page); + await settleFrames(page); const confirmInput = page.locator('#confirmation-input'); const confirmButton = modal.getByRole('button', { @@ -527,7 +507,7 @@ test.describe('GDPR Account Deletion', () => { const modal = page.getByRole('dialog'); await expect(modal).toBeVisible(); - await waitForUIStability(page); + await settleFrames(page); // Modal should have an accessible name (via aria-labelledby OR aria-label OR title) const hasAriaLabelledBy = diff --git a/tests/e2e/messaging/message-editing.spec.ts b/tests/e2e/messaging/message-editing.spec.ts index 295a3401..a5974dc9 100644 --- a/tests/e2e/messaging/message-editing.spec.ts +++ b/tests/e2e/messaging/message-editing.spec.ts @@ -18,6 +18,7 @@ */ import { test, expect, type Page } from '@playwright/test'; +import { settleFrames } from '../utils/settle'; import { fillMessageInput, scrollThreadToBottom, @@ -49,27 +50,6 @@ function forwardConsole(page: Page, label = 'browser') { }); } -/** - * Wait for the UI to stabilize after navigation or interaction (3 stable frames). - */ -async function waitForUIStability(page: Page) { - await page.waitForLoadState('domcontentloaded'); - await page.waitForFunction( - () => { - return new Promise((resolve) => { - let stableFrames = 0; - const checkStability = () => { - stableFrames++; - if (stableFrames >= 3) resolve(true); - else requestAnimationFrame(checkStability); - }; - requestAnimationFrame(checkStability); - }); - }, - { timeout: 15000 } - ); -} - /** * Send a message in the current conversation via the real encrypted UI path. * @@ -123,7 +103,7 @@ async function sendMessage(page: Page, message: string) { await messageElement.scrollIntoViewIfNeeded(); // Wait for UI to stabilize after sending - await waitForUIStability(page); + await settleFrames(page); // Small additional wait for React to fully render Edit/Delete buttons // (buttons depend on isOwn and timestamp checks) @@ -213,7 +193,7 @@ test.describe('Message Editing', () => { await expect(editTextarea).not.toBeVisible({ timeout: 15000 }); // Wait for UI to stabilize after save (state update + re-render) - await waitForUIStability(viewer.page); + await settleFrames(viewer.page); // Find the message bubble with edited content (updates in place) const editedBubble = getMessageBubble(viewer.page, editedMessage); @@ -372,7 +352,7 @@ test.describe('Message Deletion', () => { await expect(modal).not.toBeVisible({ timeout: 10000 }); // Wait for UI to stabilize after deletion - await waitForUIStability(viewer.page); + await settleFrames(viewer.page); // Either the message is removed OR replaced with "[Message deleted]" const messageGone = viewer.page.getByText(messageToDelete); @@ -464,7 +444,7 @@ test.describe('Message Deletion', () => { await expect(modal).not.toBeVisible({ timeout: 10000 }); // Wait for UI to stabilize after deletion - await waitForUIStability(viewer.page); + await settleFrames(viewer.page); // Either the message is removed OR replaced with "[Message deleted]" const messageGone = viewer.page.getByText(messageToDelete); @@ -762,7 +742,7 @@ test.describe('Accessibility', () => { name: /Delete Message/i, }); await expect(modal).toBeVisible(); - await waitForUIStability(viewer.page); + await settleFrames(viewer.page); // Button accessible names are "Cancel deletion" and "Confirm deletion" const cancelButton = modal.getByRole('button', { diff --git a/tests/e2e/messaging/messaging-scroll.spec.ts b/tests/e2e/messaging/messaging-scroll.spec.ts index 6512c6e7..3e86cb15 100644 --- a/tests/e2e/messaging/messaging-scroll.spec.ts +++ b/tests/e2e/messaging/messaging-scroll.spec.ts @@ -1,4 +1,5 @@ import { test, expect, Page } from '@playwright/test'; +import { settleFrames } from '../utils/settle'; import { dismissCookieBanner, handleReAuthModal, @@ -20,27 +21,6 @@ const PRIMARY_EMAIL = process.env.TEST_USER_PRIMARY_EMAIL; const SCROLL_FIXTURE_MESSAGE_COUNT = 30; let scrollFixture: ScrollFixture | null = null; -/** - * Wait for UI to stabilize after navigation or interaction - */ -async function waitForUIStability(page: Page) { - await page.waitForLoadState('domcontentloaded'); - await page.waitForFunction( - () => { - return new Promise((resolve) => { - let stableFrames = 0; - const checkStability = () => { - stableFrames++; - if (stableFrames >= 3) resolve(true); - else requestAnimationFrame(checkStability); - }; - requestAnimationFrame(checkStability); - }); - }, - { timeout: 15000 } - ); -} - /** * Messaging Scroll E2E Tests * Feature: 005-fix-messaging-scroll @@ -117,7 +97,7 @@ async function clickFirstConversation(page: Page): Promise { // Wait for chat window to load after clicking await page.waitForSelector('[data-testid="chat-window"]', { timeout: 10000 }); - await waitForUIStability(page); + await settleFrames(page); } // Helper to check if element is in viewport @@ -269,7 +249,7 @@ test.describe('Messaging Scroll - User Story 2: Scroll Through Messages', () => }); // Wait for scroll to complete - await waitForUIStability(page); + await settleFrames(page); // Get input position after scroll const afterScrollInputBox = await messageInput.boundingBox(); @@ -312,7 +292,7 @@ test.describe('Messaging Scroll - User Story 3: Jump to Bottom Button', () => { const messageThread = page.locator('[data-testid="message-thread"]'); await expect(messageThread).toBeVisible({ timeout: 30000 }); - await waitForUIStability(page); + await settleFrames(page); // Scroll up more than 500px to trigger button await messageThread.evaluate((el) => { @@ -320,7 +300,7 @@ test.describe('Messaging Scroll - User Story 3: Jump to Bottom Button', () => { el.dispatchEvent(new Event('scroll', { bubbles: true })); }); - await waitForUIStability(page); + await settleFrames(page); const jumpButton = page.locator('[data-testid="jump-to-bottom"]'); @@ -388,15 +368,51 @@ test.describe('Messaging Scroll - User Story 3: Jump to Bottom Button', () => { el.dispatchEvent(new Event('scroll', { bubbles: true })); }); - await waitForUIStability(page); + await settleFrames(page); const jumpButton = page.locator('[data-testid="jump-to-bottom"]'); + // WAIT THE WAY T007/T008 DOES, WHICH IS WHY T007/T008 DOES NOT FLAKE. + // + // Removing the old `if (await jumpButton.isVisible())` wrapper — which made the whole + // test vacuous whenever the button was absent — exposed a SECOND failure hiding behind + // it: on firefox the button was simply not there yet, and `expect(...).toBeVisible()` + // reported `` after 5s. + // + // `settleFrames` advances three animation frames, roughly 50 ms. That is not long + // enough for the scroll to propagate through React state to a rendered button on every + // engine. T007/T008 solves this by waiting on the component's OWN signal instead of on + // time, and this now does the same. + // + // First: prove the thread really did scroll. A thread that is too short to pass the + // 500px threshold SHOULD have no button, and asserting the button in that case would be + // blaming the component for a fixture problem. + const scrollInfo = await messageThread.evaluate((el) => ({ + distanceFromBottom: el.scrollHeight - (el.scrollTop + el.clientHeight), + })); + expect( + scrollInfo.distanceFromBottom, + 'fixture thread is not tall enough to scroll 500px+ from the bottom, so the jump ' + + 'button is correctly absent — this is a fixture failure, not a UI regression' + ).toBeGreaterThan(500); + + // Then: the attribute MessageThread writes synchronously when it decides to show the + // button, which sidesteps the React-state-flush vs event-loop race entirely. + const wrapper = page.locator('[data-show-scroll-button]').first(); + await expect + .poll(async () => await wrapper.getAttribute('data-show-scroll-button'), { + message: + 'MessageThread never set data-show-scroll-button="true" after scrolling to the ' + + 'top — the component did not register the scroll', + timeout: 5000, + intervals: [50, 100, 200, 500], + }) + .toBe('true'); + // NOT `if (await jumpButton.isVisible())`. The whole body used to sit inside that // condition, so a thread where the button never appeared passed having asserted // nothing — and "the jump button stopped rendering" is precisely what this test is - // named for. T007/T008 above already establish it appears when scrolled up, so here - // it is a requirement, not a precondition. + // named for. It is a requirement here, not a precondition. await expect(jumpButton).toBeVisible(); await jumpButton.click(); @@ -405,7 +421,7 @@ test.describe('Messaging Scroll - User Story 3: Jump to Bottom Button', () => { // // This is what made T009 flaky, and it is not a browser quirk: the button calls // `scrollToBottom(true)`, i.e. `behavior: 'smooth'` (MessageThread.tsx:239-243), while - // `waitForUIStability` waits three animation frames — about 50 ms. A smooth scroll from + // `settleFrames` (then named `waitForUIStability`) waits three animation frames — about 50 ms. A smooth scroll from // the top of a 30-message thread takes several hundred. So the assertion measured a // scroll that had barely started, and failed with **2393px** remaining rather than // marginally over the 100px threshold. diff --git a/tests/e2e/messaging/offline-queue.spec.ts b/tests/e2e/messaging/offline-queue.spec.ts index c549b810..9f126a9e 100644 --- a/tests/e2e/messaging/offline-queue.spec.ts +++ b/tests/e2e/messaging/offline-queue.spec.ts @@ -20,6 +20,7 @@ */ import { test, expect, type Page } from '@playwright/test'; +import { settleFrames } from '../utils/settle'; import { seedIsolatedConversation, deleteIsolatedConversation, @@ -37,27 +38,6 @@ import { // Per-test isolation removes the shared-user data race that forced serial mode. test.describe.configure({ mode: 'parallel' }); -/** - * Wait for UI to stabilize after navigation or interaction. - */ -async function waitForUIStability(page: Page) { - await page.waitForLoadState('domcontentloaded'); - await page.waitForFunction( - () => { - return new Promise((resolve) => { - let stableFrames = 0; - const checkStability = () => { - stableFrames++; - if (stableFrames >= 3) resolve(true); - else requestAnimationFrame(checkStability); - }; - requestAnimationFrame(checkStability); - }); - }, - { timeout: 15000 } - ); -} - /** * Wait for conversation data to be cached (required for offline send). * @@ -215,7 +195,7 @@ test.describe('Offline Message Queue', () => { await messageInput.fill(msg); await sendButton.click(); // Wait for UI to stabilize between sends - await waitForUIStability(viewer.page); + await settleFrames(viewer.page); } // ===== STEP 4: Verify all 3 messages are queued ===== diff --git a/tests/e2e/messaging/performance.spec.ts b/tests/e2e/messaging/performance.spec.ts index 70e15dc2..afc54e99 100644 --- a/tests/e2e/messaging/performance.spec.ts +++ b/tests/e2e/messaging/performance.spec.ts @@ -22,6 +22,7 @@ */ import { test, expect } from '@playwright/test'; +import { settleFrames } from '../utils/settle'; import { seedIsolatedConversation, deleteIsolatedConversation, @@ -40,28 +41,6 @@ test.describe.configure({ mode: 'parallel' }); */ const SEEDED_MESSAGE_COUNT = 150; -/** - * Wait for UI to stabilize after navigation or interaction. - * Mirrors the original spec's frame-stability gate. - */ -async function waitForUIStability(page: import('@playwright/test').Page) { - await page.waitForLoadState('domcontentloaded'); - await page.waitForFunction( - () => { - return new Promise((resolve) => { - let stableFrames = 0; - const checkStability = () => { - stableFrames++; - if (stableFrames >= 3) resolve(true); - else requestAnimationFrame(checkStability); - }; - requestAnimationFrame(checkStability); - }); - }, - { timeout: 15000 } - ); -} - /** * Assert the isolated conversation is fully loaded: message-thread mounted and * the message input visible. openAsViewer already waits for the thread to be @@ -112,7 +91,7 @@ test.describe('Virtual Scrolling Performance', () => { await expectConversationLoaded(viewer); // Basic performance check — large-history page should load + stabilize // without errors. - await waitForUIStability(viewer.page); + await settleFrames(viewer.page); } finally { await viewer.close(); } @@ -123,7 +102,7 @@ test.describe('Virtual Scrolling Performance', () => { try { await expectConversationLoaded(viewer); // Wait for initial messages to load and stabilize. - await waitForUIStability(viewer.page); + await settleFrames(viewer.page); } finally { await viewer.close(); } @@ -142,7 +121,7 @@ test.describe('Virtual Scrolling Performance', () => { if (jumpVisible) { await jumpButton.click(); - await waitForUIStability(viewer.page); + await settleFrames(viewer.page); } } finally { await viewer.close(); @@ -156,7 +135,7 @@ test.describe('Virtual Scrolling Performance', () => { try { await expectConversationLoaded(viewer); // Basic scroll test. - await waitForUIStability(viewer.page); + await settleFrames(viewer.page); } finally { await viewer.close(); } @@ -169,7 +148,7 @@ test.describe('Virtual Scrolling Performance', () => { try { await expectConversationLoaded(viewer); // Basic performance check. - await waitForUIStability(viewer.page); + await settleFrames(viewer.page); } finally { await viewer.close(); } @@ -204,7 +183,7 @@ test.describe('Keyboard Navigation', () => { await viewer.page.keyboard.press('ArrowDown'); await viewer.page.keyboard.press('ArrowUp'); - await waitForUIStability(viewer.page); + await settleFrames(viewer.page); } finally { await viewer.close(); } @@ -217,7 +196,7 @@ test.describe('Keyboard Navigation', () => { // Test tab navigation. await viewer.page.keyboard.press('Tab'); - await waitForUIStability(viewer.page); + await settleFrames(viewer.page); } finally { await viewer.close(); } @@ -241,7 +220,7 @@ test.describe('Scroll Restoration', () => { const viewer = await openAsViewer(browser, fixture!); try { await expectConversationLoaded(viewer); - await waitForUIStability(viewer.page); + await settleFrames(viewer.page); } finally { await viewer.close(); } @@ -251,7 +230,7 @@ test.describe('Scroll Restoration', () => { const viewer = await openAsViewer(browser, fixture!); try { await expectConversationLoaded(viewer); - await waitForUIStability(viewer.page); + await settleFrames(viewer.page); } finally { await viewer.close(); } diff --git a/tests/e2e/utils/settle.ts b/tests/e2e/utils/settle.ts new file mode 100644 index 00000000..4640c0cd --- /dev/null +++ b/tests/e2e/utils/settle.ts @@ -0,0 +1,43 @@ +import type { Page } from '@playwright/test'; + +/** + * Advance a few animation frames. **This is not a stability wait** (#739). + * + * It was called `waitForUIStability` and duplicated verbatim in five messaging specs, and + * the name did real damage: it observes nothing, so the frames elapse whether the UI settled + * or not. T009 used it to wait out a `behavior: 'smooth'` scroll that takes several hundred + * milliseconds, measured a scroll that had barely started, and hard-failed with + * `distanceFromBottom` 2393 against a threshold of 100 — on chromium in one run and firefox + * in the next. + * + * WHAT IT IS FOR. Yielding to the browser so a synchronous DOM mutation has been rendered + * before you do something else. That is all it can honestly promise. + * + * WHAT IT IS NOT FOR — and the rule that matters: + * + * **Never put this before a measurement that does not retry.** If the next line reads + * `boundingBox()`, `evaluate(() => el.scrollTop)`, or any one-shot value, you are racing + * whatever produced that value. Use `expect.poll(...)` on the value instead, or an + * auto-retrying `expect(locator)`, and assert the OUTCOME rather than a duration. + * + * Playwright's `expect(locator)` already retries, so a call immediately before one is + * redundant rather than harmful. Those are left in place deliberately: removing twenty of + * them would be churn with a real chance of disturbing timing nobody has measured. + */ +export async function settleFrames(page: Page, frames = 3): Promise { + await page.waitForLoadState('domcontentloaded'); + await page.waitForFunction( + (n) => + new Promise((resolve) => { + let seen = 0; + const tick = () => { + seen++; + if (seen >= n) resolve(true); + else requestAnimationFrame(tick); + }; + requestAnimationFrame(tick); + }), + frames, + { timeout: 15000 } + ); +}