diff --git a/e2e/a11y/config.spec.ts b/e2e/a11y/config.spec.ts index 710e184f..94ed5aed 100644 --- a/e2e/a11y/config.spec.ts +++ b/e2e/a11y/config.spec.ts @@ -5,6 +5,7 @@ import { LIVE, THEMES, WCAG_TAGS, + lockBase, CONFIG_TABS, openConfigTab, lockUserProfile, @@ -16,8 +17,10 @@ import { /** * Accessibility and visual-system guardrails for the configuration page (VS-12): * axe on every tab at 1440 and 390 px in both themes, layout invariants of the - * settings grid, and screenshot baselines. Needs a live backend (see ../live.ts); - * refresh baselines with `--update-snapshots` after intended visual changes. + * settings grid, and screenshot baselines. Needs a live backend (see ../live.ts). + * The baselines show librechat.yaml values only: refresh them with + * `--update-snapshots` after intended visual changes, on a Base that stores no + * admin overrides. */ test.skip(!LIVE, LIVE_SKIP_REASON); @@ -301,16 +304,45 @@ const VISUAL_VARIANTS = [ { label: 'light-390', colorScheme: 'light', width: 390, height: 1100 }, ] as const; +/** + * Opens a tab and checks that Base stores no admin overrides. An override draws + * its tab's override count into every baseline, the header's revert action into + * the 1440 px ones, and its chip and section count into its own tab's. Each + * journey that saves to Base holds the Base lock until it has reverted, and the + * baseline holds it until its screenshot is taken, so an override found here was + * left by a run that failed before reverting, or saved by a test that does not + * take the lock. It fails naming its tabs ("System: 1 admin override") before a + * screenshot is compared or written by `--update-snapshots`. + */ +async function openWithoutBaseOverrides(page: Page, tab: string) { + await openConfigTab(page, tab); + await expect( + page.getByRole('tab', { selected: true }).locator('.config-tab-count').first(), + ).toBeVisible(); + const overridden = await page + .getByRole('tab') + .filter({ has: page.locator('.config-tab-count-override') }) + .evaluateAll((tabs: HTMLElement[]) => + tabs.map((el) => { + const count = el.querySelector('.config-tab-count-override')?.getAttribute('title'); + return `${el.innerText.split('\n')[0]}: ${count}`; + }), + ); + expect(overridden, 'Tabs where Base stores admin overrides, which no baseline shows').toEqual([]); +} + for (const variant of VISUAL_VARIANTS) { test.describe(`visual baseline: ${variant.label}`, () => { test.use({ colorScheme: variant.colorScheme, viewport: { width: variant.width, height: variant.height }, }); + lockBase(); + lockUserProfile(); for (const tab of VISUAL_TABS) { test(`${tab}`, async ({ page }) => { - await openConfigTab(page, tab); + await openWithoutBaseOverrides(page, tab); await hideDevtools(page); await expect(page).toHaveScreenshot(`${tab}-${variant.label}.png`, { animations: 'disabled', @@ -509,7 +541,7 @@ test.describe('a role profile with an override of its own', () => { */ let saved: 'profile' | 'override' | null = null; - lockUserProfile(async (page) => { + lockUserProfile(async ({ page }) => { const undo = saved; saved = null; if (undo == null) return; diff --git a/e2e/a11y/config.spec.ts-snapshots/features-light-390-chromium-linux.png b/e2e/a11y/config.spec.ts-snapshots/features-light-390-chromium-linux.png index 8960c1e6..1717211f 100644 Binary files a/e2e/a11y/config.spec.ts-snapshots/features-light-390-chromium-linux.png and b/e2e/a11y/config.spec.ts-snapshots/features-light-390-chromium-linux.png differ diff --git a/e2e/a11y/config.spec.ts-snapshots/system-light-390-chromium-linux.png b/e2e/a11y/config.spec.ts-snapshots/system-light-390-chromium-linux.png index 86fc9a61..e9b67897 100644 Binary files a/e2e/a11y/config.spec.ts-snapshots/system-light-390-chromium-linux.png and b/e2e/a11y/config.spec.ts-snapshots/system-light-390-chromium-linux.png differ diff --git a/e2e/config-shell.spec.ts b/e2e/config-shell.spec.ts index 060db39b..89e88d4f 100644 --- a/e2e/config-shell.spec.ts +++ b/e2e/config-shell.spec.ts @@ -259,7 +259,7 @@ test.describe('Configuration shell: Back from the USER role profile', () => { /** Set once a test creates the USER role's profile, so cleanup never deletes one it found. */ let createdUserProfile = false; - lockUserProfile(async (page) => { + lockUserProfile(async ({ page }) => { if (!createdUserProfile) return; createdUserProfile = false; await deleteUserRoleProfile(page); diff --git a/e2e/features.spec.ts b/e2e/features.spec.ts index 09d170c5..f9c75eb9 100644 --- a/e2e/features.spec.ts +++ b/e2e/features.spec.ts @@ -1,6 +1,6 @@ import { test, expect } from '@playwright/test'; import type { Locator, Page } from '@playwright/test'; -import { LIVE, LIVE_SKIP_REASON, openConfigTab } from './live'; +import { LIVE, LIVE_SKIP_REASON, lockBase, openConfigTab } from './live'; /** * Features tab journeys (WS13): the memory agent switch, allowlists, web @@ -35,6 +35,7 @@ async function setText(field: Locator, value: string) { test.describe('Features editors', () => { test.skip(!LIVE, LIVE_SKIP_REASON); test.describe.configure({ mode: 'serial', timeout: 120_000 }); + lockBase(); test('the memory agent switches from a librechat.yaml ID to an inline model and back', async ({ page, diff --git a/e2e/import.spec.ts b/e2e/import.spec.ts index 7d03c522..b5fc318f 100644 --- a/e2e/import.spec.ts +++ b/e2e/import.spec.ts @@ -1,6 +1,6 @@ import { test, expect } from '@playwright/test'; import type { Page } from '@playwright/test'; -import { LIVE, LIVE_SKIP_REASON, lockUserProfile } from './live'; +import { LIVE, LIVE_SKIP_REASON, lockBase, lockUserProfile } from './live'; /** * The YAML import wizard needs LibreChat's admin config API, which the mock @@ -45,22 +45,45 @@ async function startBalanceIn(page: Page, scope?: string) { return page.locator('#balance-startBalance'); } -test('imports a snippet into Base, previewing exactly its keys', async ({ page }) => { - const value = 100000 + (Date.now() % 100000); - await openWizard( - page, - `version: 1.3.3\nbalance:\n startBalance: ${value}\ninterface:\n prompts: false\n`, - ); - await page.getByRole('radio', { name: /^Base configuration/ }).click(); - const { dialog, apply } = await previewAndApply(page, /into Base configuration$/); - await expect(dialog.getByRole('checkbox')).toHaveCount(1); - await expect(dialog.getByText('Starting credits')).toBeVisible(); - await expect(dialog.getByText(/Prompts: managed with role permissions/)).toBeVisible(); - await apply.click(); - await expect( - page.getByText(/Imported 1 value into the base configuration/).first(), - ).toBeVisible(); - await expect(await startBalanceIn(page)).toHaveValue(value.toLocaleString('en-US')); +/** + * Puts Base's Starting credits back to librechat.yaml when an import left an + * override there. Base otherwise keeps it after the run, and every Base + * override shows in the configuration page's visual baselines. + */ +async function revertBaseStartBalance(page: Page) { + await startBalanceIn(page); + const revert = page.getByRole('button', { + name: /^(Revert to .+|Remove override): Starting credits$/, + }); + if ((await revert.count()) === 0) return; + await revert.click(); + await page.getByRole('button', { name: /^Review & save$/ }).click(); + const dialog = page.getByRole('dialog'); + await dialog.getByRole('button', { name: /^Save$/ }).click(); + await expect(dialog).toHaveCount(0); + await expect(revert).toHaveCount(0); +} + +test.describe('the base configuration', () => { + lockBase(({ page }) => revertBaseStartBalance(page)); + + test('imports a snippet into Base, previewing exactly its keys', async ({ page }) => { + const value = 100000 + (Date.now() % 100000); + await openWizard( + page, + `version: 1.3.3\nbalance:\n startBalance: ${value}\ninterface:\n prompts: false\n`, + ); + await page.getByRole('radio', { name: /^Base configuration/ }).click(); + const { dialog, apply } = await previewAndApply(page, /into Base configuration$/); + await expect(dialog.getByRole('checkbox')).toHaveCount(1); + await expect(dialog.getByText('Starting credits')).toBeVisible(); + await expect(dialog.getByText(/Prompts: managed with role permissions/)).toBeVisible(); + await apply.click(); + await expect( + page.getByText(/Imported 1 value into the base configuration/).first(), + ).toBeVisible(); + await expect(await startBalanceIn(page)).toHaveValue(value.toLocaleString('en-US')); + }); }); test.describe('the USER role profile', () => { diff --git a/e2e/live.ts b/e2e/live.ts index da3a369b..e037c373 100644 --- a/e2e/live.ts +++ b/e2e/live.ts @@ -2,7 +2,7 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { test, expect } from '@playwright/test'; import { mkdirSync, readFileSync, renameSync, rmSync, statSync, writeFileSync } from 'node:fs'; -import type { Page, TestInfo } from '@playwright/test'; +import type { Browser, Page, TestInfo } from '@playwright/test'; /** * Configuration specs need a LibreChat backend that serves the admin config @@ -78,15 +78,12 @@ function lockKey(baseUrl: string): string { } const LOCK_KEY = lockKey(process.env.E2E_BASE_URL ?? 'http://localhost:3000'); -const LOCK_DIR = join(tmpdir(), `librechat-admin-e2e-user-profile-${LOCK_KEY}`); const LOCK_POLL_MS = 250; /** Longer than a locked test holds the lock, unless its body and its cleanup both hang. */ const LOCK_STALE_MS = 5 * 60_000; /** Past the stale age, so a waiter takes over a hung holder's lock before it gives up. */ const LOCK_WAIT_MS = LOCK_STALE_MS + 60_000; -let lockHeld = false; - function lockOwner(dir: string): number | undefined { try { const pid = Number(readFileSync(join(dir, 'owner'), 'utf8')); @@ -106,9 +103,9 @@ function isAlive(pid: number): boolean { } /** - * A lock is stale when its owner has exited, is this worker (one test runs at a time per - * worker, so it leaked), or has held it past the stale age. A lock whose owner file is not - * written yet is judged by age alone. + * A lock is stale when its owner has exited, is this worker (a worker only waits for a lock it + * does not hold, so it leaked), or has held it past the stale age. A lock whose owner file is + * not written yet is judged by age alone. */ function isStale(dir: string): boolean { const pid = lockOwner(dir); @@ -124,11 +121,11 @@ function isStale(dir: string): boolean { * Moves a stale lock aside and deletes it. When the move fails, another waiter got there * first; when the lock moved aside is not stale, a waiter re-took it in between, so it goes back. */ -function takeOver() { - const aside = `${LOCK_DIR}.stale-${process.pid}-${Date.now()}`; +function takeOver(dir: string) { + const aside = `${dir}.stale-${process.pid}-${Date.now()}`; try { - renameSync(LOCK_DIR, aside); - if (!isStale(aside)) renameSync(aside, LOCK_DIR); + renameSync(dir, aside); + if (!isStale(aside)) renameSync(aside, dir); } catch { /* Another waiter moved it first, or took the lock while it was aside. */ } finally { @@ -137,62 +134,104 @@ function takeOver() { } /** One attempt: `mkdir` is atomic and fails while another worker holds the lock. */ -function tryLock(): boolean { +function tryLock(dir: string): boolean { try { - mkdirSync(LOCK_DIR); + mkdirSync(dir); } catch (error) { if ((error as NodeJS.ErrnoException).code !== 'EEXIST') throw error; - if (isStale(LOCK_DIR)) takeOver(); + if (isStale(dir)) takeOver(dir); return false; } - writeFileSync(join(LOCK_DIR, 'owner'), String(process.pid)); + writeFileSync(join(dir, 'owner'), String(process.pid)); return true; } -function releaseLock() { - if (!lockHeld) return; - lockHeld = false; - process.off('exit', releaseLock); - if (lockOwner(LOCK_DIR) === process.pid) rmSync(LOCK_DIR, { recursive: true, force: true }); -} +/** A lock on one piece of E2E_BASE_URL's backend, shared by every worker and spec file. */ +function backendLock(name: string, label: string) { + const dir = join(tmpdir(), `librechat-admin-e2e-${name}-${LOCK_KEY}`); + let held = false; + + function release() { + if (!held) return; + held = false; + process.off('exit', release); + if (lockOwner(dir) === process.pid) rmSync(dir, { recursive: true, force: true }); + } -/** Waits for the lock without spending the test's own timeout on the wait. */ -async function acquireLock(testInfo: TestInfo) { - const started = Date.now(); - const timeout = testInfo.timeout; - if (timeout) testInfo.setTimeout(timeout + LOCK_WAIT_MS); - while (!tryLock()) { - if (Date.now() - started > LOCK_WAIT_MS) { - const holder = lockOwner(LOCK_DIR) ?? 'unknown'; - throw new Error( - `Gave up after ${LOCK_WAIT_MS / 1000} s waiting for the USER role profile lock ` + - `${LOCK_DIR}, held by pid ${holder}`, - ); + /** Waits for the lock without spending the test's own timeout on the wait. */ + async function acquire(testInfo: TestInfo) { + const started = Date.now(); + const timeout = testInfo.timeout; + if (timeout) testInfo.setTimeout(timeout + LOCK_WAIT_MS); + while (!tryLock(dir)) { + if (Date.now() - started > LOCK_WAIT_MS) { + const holder = lockOwner(dir) ?? 'unknown'; + throw new Error( + `Gave up after ${LOCK_WAIT_MS / 1000} s waiting for the ${label} lock ` + + `${dir}, held by pid ${holder}`, + ); + } + await new Promise((resolve) => setTimeout(resolve, LOCK_POLL_MS)); } - await new Promise((resolve) => setTimeout(resolve, LOCK_POLL_MS)); + held = true; + process.on('exit', release); + if (timeout) testInfo.setTimeout(timeout + Date.now() - started); } - lockHeld = true; - process.on('exit', releaseLock); - if (timeout) testInfo.setTimeout(timeout + Date.now() - started); + + return { acquire, release }; } -/** - * Runs each test of the calling describe alone against E2E_BASE_URL's backend, across workers - * and spec files. Every test that creates, uses or deletes the USER role's profile calls it: - * there are only two built-in roles, so no test gets a profile of its own. `cleanup` runs - * before the lock is released, also after a failure. - */ -export function lockUserProfile(cleanup?: (page: Page) => Promise) { - test.beforeEach(() => acquireLock(test.info())); - test.afterEach(async ({ page }) => { +const userProfileLock = backendLock('user-profile', 'USER role profile'); +const baseLock = backendLock('base', 'Base configuration'); + +/** Puts back what a locked test changed, before the lock is released, also after a failure. */ +type LockCleanup = ( + fixtures: { page: Page; browser: Browser }, + testInfo: TestInfo, +) => Promise; + +/** Holds `lock` through each test of the calling describe and its `cleanup`. */ +function lockEach(lock: ReturnType, cleanup?: LockCleanup) { + test.beforeEach(() => lock.acquire(test.info())); + test.afterEach(async ({ page, browser }, testInfo) => { try { - await cleanup?.(page); + await cleanup?.({ page, browser }, testInfo); } finally { - releaseLock(); + lock.release(); } }); } +/** + * Runs each test of the calling describe alone against E2E_BASE_URL's backend, across workers + * and spec files. Every test that creates, uses or deletes the USER role's profile calls it: + * there are only two built-in roles, so no test gets a profile of its own. + */ +export function lockUserProfile(cleanup?: LockCleanup) { + lockEach(userProfileLock, cleanup); +} + +/** + * Keeps Base's admin overrides from changing under a configuration visual baseline. Every + * describe whose journeys save to Base calls it, so each journey holds the lock from before its + * first save until it, or its `cleanup`, has put Base back; each baseline holds it from its check + * that Base stores no override until its screenshot is taken. A describe that also locks the USER + * role's profile nests inside this one, so every worker takes the two locks in the same order. + */ +export function lockBase(cleanup?: LockCleanup) { + lockEach(baseLock, cleanup); +} + +/** Holds the Base lock (see lockBase) around a hook that saves to Base outside a test. */ +export async function withBaseLock(testInfo: TestInfo, run: () => Promise) { + await baseLock.acquire(testInfo); + try { + await run(); + } finally { + baseLock.release(); + } +} + /** Opens the scope selector once its profile list has loaded. */ export async function openScopeSelector(page: Page) { await page.getByRole('button', { name: /^Select scope/ }).click(); diff --git a/e2e/mcp.spec.ts b/e2e/mcp.spec.ts index 713237f3..705747fe 100644 --- a/e2e/mcp.spec.ts +++ b/e2e/mcp.spec.ts @@ -1,6 +1,6 @@ import { test, expect } from '@playwright/test'; import type { Locator, Page } from '@playwright/test'; -import { LIVE, LIVE_SKIP_REASON } from './live'; +import { LIVE, LIVE_SKIP_REASON, lockBase } from './live'; /** * MCP server journeys (WS12) from the MCP audit. They need a real LibreChat @@ -66,6 +66,7 @@ async function setText(field: Locator, value: string) { test.describe('Configuration: MCP servers', () => { test.skip(!LIVE, LIVE_SKIP_REASON); test.describe.configure({ mode: 'serial', timeout: 120_000 }); + lockBase(); test('turning Show in chat menu on and back to Default leaves nothing to save (MCP-6)', async ({ page, diff --git a/e2e/providers.spec.ts b/e2e/providers.spec.ts index 91eec079..b1ce75fc 100644 --- a/e2e/providers.spec.ts +++ b/e2e/providers.spec.ts @@ -1,6 +1,6 @@ import { test, expect } from '@playwright/test'; import type { Locator, Page } from '@playwright/test'; -import { LIVE, LIVE_SKIP_REASON } from './live'; +import { LIVE, LIVE_SKIP_REASON, lockBase } from './live'; /** * AI providers editor (WS9) journeys. They need a live LibreChat backend @@ -11,6 +11,7 @@ import { LIVE, LIVE_SKIP_REASON } from './live'; */ test.skip(!LIVE, LIVE_SKIP_REASON); test.describe.configure({ mode: 'serial', timeout: 120_000 }); +lockBase(); const DISABLED_PROVIDER = process.env.E2E_DISABLED_PROVIDER ?? 'assistants'; diff --git a/e2e/saves.spec.ts b/e2e/saves.spec.ts index 210cfeb6..b18cacd3 100644 --- a/e2e/saves.spec.ts +++ b/e2e/saves.spec.ts @@ -1,6 +1,6 @@ import { test, expect } from '@playwright/test'; import type { Locator, Page } from '@playwright/test'; -import { LIVE, LIVE_SKIP_REASON } from './live'; +import { LIVE, LIVE_SKIP_REASON, lockBase } from './live'; /** * Save-pipeline journeys for custom endpoints (WS1). They need a real @@ -53,6 +53,7 @@ async function setText(field: Locator, value: string) { test.describe('Configuration saves: custom endpoints by identity', () => { test.skip(!LIVE, LIVE_SKIP_REASON); test.describe.configure({ mode: 'serial', timeout: 120_000 }); + lockBase(); test('editing one field of a librechat.yaml endpoint saves only that change', async ({ page, diff --git a/e2e/specs.spec.ts b/e2e/specs.spec.ts index 901253dc..64410a5a 100644 --- a/e2e/specs.spec.ts +++ b/e2e/specs.spec.ts @@ -1,13 +1,13 @@ import { test, expect } from '@playwright/test'; -import type { Locator, Page } from '@playwright/test'; -import { LIVE, LIVE_SKIP_REASON, lockUserProfile } from './live'; +import type { Browser, BrowserContextOptions, Locator, Page } from '@playwright/test'; +import { LIVE, LIVE_SKIP_REASON, lockBase, lockUserProfile, withBaseLock } from './live'; /** * Model specs journeys (WS11, the audit's specs journeys). They need a live - * LibreChat backend whose librechat.yaml defines at least one model spec and - * whose Base stores no spec list of its own (reset the admin overrides first): - * run with E2E_LIVE=1. Each journey that saves puts the list back to - * librechat.yaml before it ends. + * LibreChat backend whose librechat.yaml defines at least one model spec: + * run with E2E_LIVE=1. They also need a Base that stores no spec list of its + * own, so the list goes back to librechat.yaml before the first journey (a + * failed run may have left one) and after every journey, passed or failed. */ const unsavedBar = (page: Page) => page.getByRole('region', { name: 'Unsaved changes' }); const cards = (page: Page) => page.locator('#section-modelSpecs\\.list [data-entry-card]'); @@ -36,14 +36,26 @@ async function saveAndWait(page: Page) { await expect(dialog).toHaveCount(0); } -/** Puts the spec list back to librechat.yaml when the journey left an override. */ -async function revertList(page: Page) { - await openSpecs(page); - const revert = page.getByRole('button', { name: 'Revert list to librechat.yaml' }); - if ((await revert.count()) === 0) return; - await revert.click(); - await review(page); - await saveAndWait(page); +/** + * Puts the spec list back to librechat.yaml when Base stores a list of its own. + * It opens a page in a context of its own, so it runs before the first journey, + * which has no page yet, and after each journey whatever that journey left on + * its page: a review dialog, unsaved edits or a phone-width viewport. + */ +async function revertList(browser: Browser, { baseURL, storageState }: BrowserContextOptions) { + const context = await browser.newContext({ baseURL, storageState }); + try { + const page = await context.newPage(); + await openSpecs(page); + const revert = page.getByRole('button', { name: 'Revert list to librechat.yaml' }); + if ((await revert.count()) === 0) return; + await revert.click(); + await review(page); + await saveAndWait(page); + await expect(revert).toHaveCount(0); + } finally { + await context.close(); + } } /** @@ -66,6 +78,10 @@ async function addSpec(page: Page, label: string, endpoint: string, model: strin test.describe('Model specs', () => { test.skip(!LIVE, LIVE_SKIP_REASON); test.describe.configure({ mode: 'serial', timeout: 120_000 }); + test.beforeAll(({ browser }, testInfo) => + withBaseLock(testInfo, () => revertList(browser, testInfo.project.use)), + ); + lockBase(({ browser }, testInfo) => revertList(browser, testInfo.project.use)); test('the tab says how users pick models and what they will see', async ({ page }) => { await openSpecs(page); @@ -90,7 +106,6 @@ test.describe('Model specs', () => { await expect(cards(page)).toHaveCount(before + 1); await expect(cards(page).last()).toContainText('E2E Spec'); await expect(cards(page).last()).toContainText('OpenAI ยท gpt-4o-mini'); - await revertList(page); }); test('duplicate names and incomplete specs block Save', async ({ page }) => { @@ -141,7 +156,6 @@ test.describe('Model specs', () => { await page.waitForLoadState('networkidle'); const count = await cards(page).count(); await expect(cards(page).nth(count - 2)).toContainText('E2E Last'); - await revertList(page); }); test('Enter in a spec field never adds or removes an endpoint next to the specs', async ({