From da200a3a7f63d5e2afc680426d47985ab551242b Mon Sep 17 00:00:00 2001 From: Valentin Millet Date: Fri, 18 Sep 2026 15:15:51 +0200 Subject: [PATCH] Wait on a condition in every scenario that reads after one #190 swept six sleeping assertions and its grep was wrong: a one-line lookahead demanding a literal 'expect(' on the very next line, which misses a value read now and asserted two lines down, and anything behind a blank line. Counted properly there were 42. They are replaced with 'eventually(read, matches, what)', file by file, each with the condition that actually applies. Three are kept and say why: an assertion that *nothing* happened is not a condition anything can wait on. The four in '15-preferences-on-disk' now poll the file from Node instead of outwaiting the store plugin's debounce. 'scripts/e2e-waits.test.mjs' holds the rule with a lookahead that works, so the sweep cannot quietly come undone. Two of the rewrites taught something the sweep alone would not have: submitting a dialog and waiting only on the stored value returns sooner than the sleep did, before the dialog has closed and the next scenario opens a panel over it; and a wait on an end state is worth splitting into the steps that lead there, so a timeout names which one failed. Closes #197 --- CLAUDE.md | 4 +- e2e/specs/03-editing-a-note.e2e.ts | 12 +++-- e2e/specs/04-trash-and-undo.e2e.ts | 7 ++- e2e/specs/05-spaces.e2e.ts | 55 ++++++++++++++----- e2e/specs/07-checklists.e2e.ts | 17 ++++-- e2e/specs/08-placeholder-fields.e2e.ts | 32 ++++++++--- e2e/specs/12-preferences.e2e.ts | 8 ++- e2e/specs/13-multiple-selection.e2e.ts | 21 ++++++-- e2e/specs/14-tag-manager.e2e.ts | 38 ++++++++++---- e2e/specs/15-preferences-on-disk.e2e.ts | 31 +++++++---- e2e/specs/17-folders.e2e.ts | 56 ++++++++++++++------ e2e/specs/18-board.e2e.ts | 13 +++-- e2e/specs/19-board-gesture.e2e.ts | 33 ++++++++---- e2e/specs/20-folder-descent.e2e.ts | 34 +++++++++--- package.json | 2 +- scripts/e2e-waits.test.mjs | 70 +++++++++++++++++++++++++ 16 files changed, 335 insertions(+), 98 deletions(-) create mode 100644 scripts/e2e-waits.test.mjs diff --git a/CLAUDE.md b/CLAUDE.md index e1d12e3c..8e7e4997 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -59,7 +59,7 @@ Run all commands from the repo root (`package.json` there wraps both Angular and - `cargo test` from `src-tauri/` — unit tests are inline `#[cfg(test)] mod tests` blocks at the bottom of the file they cover; `src-tauri/tests/` holds the integration binaries (`notes`, `spaces`, `folders`, `transfer`, `ipc_contract`), which see only the crate's public API. No extra setup: they run against an in-memory SQLite database. -- `npm run test:scripts` — `node --test` on the release-notes generator and on the palette's contrast ratios. Node's own runner: no dependency, no config, and it cannot be swept up by the Angular builder, which only sees `src/**/*.spec.ts` — which is also the point, since both of these read a **shipped file off disk** and the builder compiles for a browser. ⚠️ The test **files are named one by one**, not discovered from `scripts/`: Node 24 (what `.nvmrc` pins, and what CI installs) does not expand a bare directory positional the way newer versions do — it tries to load it as the entry module and dies on `MODULE_NOT_FOUND`. A new test file has to be added to the script. +- `npm run test:scripts` — `node --test` on the release-notes generator, on the palette's contrast ratios and on the e2e suite's waits. Node's own runner: no dependency, no config, and it cannot be swept up by the Angular builder, which only sees `src/**/*.spec.ts` — which is also the point, since both of these read a **shipped file off disk** and the builder compiles for a browser. ⚠️ The test **files are named one by one**, not discovered from `scripts/`: Node 24 (what `.nvmrc` pins, and what CI installs) does not expand a bare directory positional the way newer versions do — it tries to load it as the entry module and dies on `MODULE_NOT_FOUND`. A new test file has to be added to the script. - **Releasing is a `workflow_dispatch`.** Bump the version in `src-tauri/Cargo.toml`, `package.json`, both lockfiles, merge to `main`, then Actions → Release, `dry_run` first. `release.yml` writes the changelog section, commits, tags, builds and publishes in one run. See `docs/architecture.md` → "Releasing". @@ -76,7 +76,7 @@ Run all commands from the repo root (`package.json` there wraps both Angular and These are the non-obvious constraints; the rest of the architecture is in `docs/architecture.md`. - **The end-to-end harness is a build flavour, not a flag.** `npm run e2e:build` merges `src-tauri/tauri.e2e.conf.json` and links the `e2e` Cargo feature: `withGlobalTauri`, the `wdio` capability declared **inline** (a file under `capabilities/` would ship), the `e2e` Angular configuration whose only difference is `"polyfills": ["@wdio/tauri-plugin"]`, and an identifier of its own — `app_data_dir()` being `data_dir()/identifier`, that is what keeps the suite out of the library you dogfood. ⚠️ The Rust plugins and the npm polyfill go **together**: with the crates alone the runner waits on `window.wdioTauri` and hangs before opening a session; with the polyfill alone the front end invokes `plugin:wdio|…` commands nothing answers. Details in `docs/architecture.md`. -- **⚠️ The e2e runs are not isolated from each other, and that shapes every scenario.** `driverProvider` is `embedded`, so the WebDriver server lives inside the application and the service spawns it **once** for the whole run: the fifteen spec files share one process, one SQLite file and one `preferences.json`. Raising `maxInstances` changes nothing — the service skips its per-worker spawn for this provider. So the profile is wiped once, by `tsx e2e/reset-profile.ts` **before** wdio starts (a wdio hook has no ordering against the service's own `onPrepare`, and would meet a locked database); `before()` only reloads the page, which resets the front end and no data; and a spec file seeds its own preconditions instead of assuming a clean corpus. The numeric prefix on each file is the run order, and `01-first-launch` is the only one that meets a virgin profile — hence the only one that can resolve the seeded space, which it writes to a marker file for the rest. ⚠️ `reopenSession()` is **not** a restart and nothing can be: the process hosts the server, so it must stay up. It proves the interface was rebuilt from what the commands answer, never that anything reached the disk — `15-preferences-on-disk` reads the file from Node for that. ⚠️ **A scenario waits on a condition, never on a duration**: `eventually(read, matches, what)` in `support/app.ts`, which hands the value back so the assertion reads what it waited for. A `browser.pause` before an `expect` is a guess at a round trip on a runner sharing a CPU with a WebView, and it is how one scenario went red on Windows and green on a re-run of the very same commit. The **one** exception is an assertion that nothing happened — that is not a condition anything can wait on, and the two that remain say so in a comment. Address a control by its id rather than by position wherever a page can hold two of them. +- **⚠️ The e2e runs are not isolated from each other, and that shapes every scenario.** `driverProvider` is `embedded`, so the WebDriver server lives inside the application and the service spawns it **once** for the whole run: the fifteen spec files share one process, one SQLite file and one `preferences.json`. Raising `maxInstances` changes nothing — the service skips its per-worker spawn for this provider. So the profile is wiped once, by `tsx e2e/reset-profile.ts` **before** wdio starts (a wdio hook has no ordering against the service's own `onPrepare`, and would meet a locked database); `before()` only reloads the page, which resets the front end and no data; and a spec file seeds its own preconditions instead of assuming a clean corpus. The numeric prefix on each file is the run order, and `01-first-launch` is the only one that meets a virgin profile — hence the only one that can resolve the seeded space, which it writes to a marker file for the rest. ⚠️ `reopenSession()` is **not** a restart and nothing can be: the process hosts the server, so it must stay up. It proves the interface was rebuilt from what the commands answer, never that anything reached the disk — `15-preferences-on-disk` reads the file from Node for that. ⚠️ **A scenario waits on a condition, never on a duration**: `eventually(read, matches, what)` in `support/app.ts`, which hands the value back so the assertion reads what it waited for. A `browser.pause` before an `expect` is a guess at a round trip on a runner sharing a CPU with a WebView, and it is how one scenario went red on Windows and green on a re-run of the very same commit. The **one** exception is an assertion that nothing happened — that is not a condition anything can wait on, and the three that remain say so in a comment. ⚠️ `scripts/e2e-waits.test.mjs` holds that: it fails on a `browser.pause` followed within four lines by a read or an assertion, unless a comment above says the wait is `deliberately` one. The sweep is only as good as its lookahead — #190 demanded a literal `expect(` on the very next line, missed a value read now and asserted two lines down, and covered a sixth of the job (#197). Address a control by its id rather than by position wherever a page can hold two of them. - **The IPC surface is generated.** `src/app/core/ipc/bindings.ts` comes from tauri-specta: one typed function per command plus a TS type per struct crossing the bridge. It is committed and regenerated by `npm run tauri dev` or `npm run bindings` (the `export-bindings` binary). Adding a command means annotating it `#[tauri::command]` **and** `#[specta::specta]`, adding it to `collect_commands![...]` in `src-tauri/src/lib.rs` — the single list, it both registers with Tauri and drives the generation — then regenerating. Every type crossing the bridge derives `specta::Type`. Specta refuses `usize`/`i64`/… (JSON precision), hence `NotesView.matched: u32`. The generator is _not_ wired as a `#[test]`: on Windows the test exe lives in `target/debug/deps/`, without the `WebView2Loader.dll` that linking `Builder::export` then needs, and the whole test binary fails to start. - **Calls return a Result, not a rejection.** `commands.queryNotes(q)` gives `{ status: 'ok' | 'error' }`. Repositories run it through `unwrap()` (`core/ipc/ipc.error.ts`), which returns the data or throws an `IpcError` — stores and components keep their `try`/`catch`. Only `core/data/` and `core/ipc/` call a generated command; everything else speaks the model, which `note.mapper.ts` converts to and from. `core/services/app-info/` also imports `bindings.ts`, for the `APP_METADATA` **constant** — no bridge involved — and re-exports it once as `APP_INFO`. - **Serialisation contract.** The camelCase and `tag = "kind"` serde attributes are still load-bearing, but specta reads them, so the TS side follows automatically. What generation does _not_ cover, and what `core/data/note.mapper.ts` still exists for: JSON has no date type (the Rust model holds `DateTime`, which crosses as an ISO string and the front turns back into a `Date`), and a patch omits the keys it does not touch (hence `#[specta(optional)]` on every `NotePatch` field — without it the generated type would demand explicit `null`s, which overwrite). `language` no longer needs anything: it is a Rust enum, so the bindings hand the front a real union and `LanguageTag` is a plain alias of it. diff --git a/e2e/specs/03-editing-a-note.e2e.ts b/e2e/specs/03-editing-a-note.e2e.ts index 8ef7cebf..bb2cd74c 100644 --- a/e2e/specs/03-editing-a-note.e2e.ts +++ b/e2e/specs/03-editing-a-note.e2e.ts @@ -1,9 +1,9 @@ -import { browser, expect } from '@wdio/globals'; +import { expect } from '@wdio/globals'; import { canvas } from '../pageobjects/canvas.page.js'; import { editor } from '../pageobjects/editor.page.js'; import { spaces } from '../pageobjects/overlays.page.js'; -import { reloadCanvas, viewportSize } from '../support/app.js'; +import { eventually, reloadCanvas, viewportSize } from '../support/app.js'; import { bridge, draft, homeSpaceId, query } from '../support/bridge.js'; /** @@ -133,9 +133,13 @@ describe('Editing a note', () => { it('moves the note to another space, through the renamed argument', async () => { await canvas.moveNote(title, refugeId); - await browser.pause(500); - expect((await reread())?.spaceId).toBe(refugeId); + const filed = await eventually( + async () => (await reread())?.spaceId, + (spaceId) => spaceId === refugeId, + 'the note to be filed in the refuge', + ); + expect(filed).toBe(refugeId); }); it('leaves the note reachable from the space it moved to', async () => { diff --git a/e2e/specs/04-trash-and-undo.e2e.ts b/e2e/specs/04-trash-and-undo.e2e.ts index 0cc24fd7..0944db5f 100644 --- a/e2e/specs/04-trash-and-undo.e2e.ts +++ b/e2e/specs/04-trash-and-undo.e2e.ts @@ -113,10 +113,13 @@ describe('Deleting a note, and taking it back', () => { expect((await trash.titles()).length).toBeGreaterThan(1); await trash.empty(); - await browser.pause(800); // The empty state replaces the list rather than leaving a header over nothing. - expect(await trash.emptyState().isExisting()).toBe(true); + await eventually( + () => trash.emptyState().isExisting(), + (showing) => showing, + 'the trash to say it is empty', + ); expect(await trash.rows().length).toBe(0); expect(await bridge.listTrash()).toHaveLength(0); await trash.close(); diff --git a/e2e/specs/05-spaces.e2e.ts b/e2e/specs/05-spaces.e2e.ts index a6a34733..7328f179 100644 --- a/e2e/specs/05-spaces.e2e.ts +++ b/e2e/specs/05-spaces.e2e.ts @@ -45,9 +45,12 @@ describe('Spaces', () => { it('creates a space from the switcher', async () => { await spaces.open(); await spaces.create('Veille'); - await browser.pause(500); - const all = await bridge.listSpaces(); + const all = await eventually( + () => bridge.listSpaces(), + (listed) => listed.some((space) => space.name === 'Veille'), + 'the new space to be listed', + ); expect(all.map((space) => space.name)).toContain('Veille'); }); @@ -61,9 +64,12 @@ describe('Spaces', () => { const before = (await bridge.listSpaces()).find((space) => space.name === 'Veille'); await spaces.open(); await spaces.rename(before!.id, 'Lectures'); - await browser.pause(500); - const after = (await bridge.listSpaces()).find((space) => space.id === before!.id); + const after = await eventually( + async () => (await bridge.listSpaces()).find((space) => space.id === before!.id), + (space) => space?.name === 'Lectures', + 'the renamed space to come back under its new name', + ); expect(after?.name).toBe('Lectures'); }); @@ -92,17 +98,23 @@ describe('Spaces', () => { await spaces.open(); await spaces.togglePin(id); - await browser.pause(500); - const pinned = await bridge.listSpaces(); + const pinned = await eventually( + () => bridge.listSpaces(), + (listed) => listed[0]?.pinned === true, + 'the pinned space to reach the head of the list', + ); expect(pinned[0]?.id).toBe(id); expect(pinned[0]?.pinned).toBe(true); await spaces.open(); await spaces.togglePin(id); - await browser.pause(500); - const loose = await bridge.listSpaces(); + const loose = await eventually( + () => bridge.listSpaces(), + (listed) => listed.at(-1)?.pinned === false, + 'the unpinned space to fall back to its name order', + ); expect(loose.at(-1)?.id).toBe(id); expect(loose.at(-1)?.pinned).toBe(false); }); @@ -111,13 +123,20 @@ describe('Spaces', () => { it('survives a rename', async () => { await spaces.open(); await spaces.togglePin(id); - await browser.pause(500); + await eventually( + () => bridge.listSpaces(), + (listed) => listed.find((space) => space.id === id)?.pinned === true, + 'the space to be pinned before it is renamed', + ); await spaces.open(); await spaces.rename(id, 'Zzz renamed'); - await browser.pause(500); - const after = (await bridge.listSpaces()).find((space) => space.id === id); + const after = await eventually( + async () => (await bridge.listSpaces()).find((space) => space.id === id), + (space) => space?.name === 'Zzz renamed', + 'the pinned space to come back renamed', + ); expect(after?.name).toBe('Zzz renamed'); expect(after?.pinned).toBe(true); }); @@ -141,9 +160,12 @@ describe('Spaces', () => { const target = (await bridge.listSpaces()).find((space) => space.name === 'Lectures')!; await spaces.open(); await spaces.remove(target.id, homeId); - await browser.pause(800); - expect((await bridge.listSpaces()).map((space) => space.name)).not.toContain('Lectures'); + await eventually( + () => bridge.listSpaces(), + (listed) => !listed.some((space) => space.name === 'Lectures'), + 'the absorbed space to be gone', + ); // The note survived, in the refuge — a cascade would have taken it. const view = await bridge.queryNotes(query({ search: 'Only in Lectures' })); @@ -189,8 +211,13 @@ describe('Spaces', () => { it('gives the switchers back when it is put away, and is remembered', async () => { await rail.hide(); expect(await browser.$(testid('space-switcher')).isExisting()).toBe(true); - await browser.pause(500); + // ⚠️ The preference has to reach the file before the reload, or the rail comes back. + await eventually( + () => rail.isShowing(), + (showing) => !showing, + 'the rail to be put away', + ); await reloadCanvas(); expect(await rail.isShowing()).toBe(false); diff --git a/e2e/specs/07-checklists.e2e.ts b/e2e/specs/07-checklists.e2e.ts index 64e619f6..f37b3aad 100644 --- a/e2e/specs/07-checklists.e2e.ts +++ b/e2e/specs/07-checklists.e2e.ts @@ -105,21 +105,28 @@ describe('Todo lists', () => { await canvas.openNote(title); await editor.toggleItem(1); await editor.close(); - await browser.pause(400); // `- [x] ` is `notes::checklist::to_markdown`'s syntax, reaching the card as // `DisplayNote.copyText`; the front end holds no second copy of it. - const copyText = (await reread())?.copyText; + const copyText = await eventually( + async () => (await reread())?.copyText, + (text) => text?.includes('- [ ] Tag the release') === true, + 'the untick to come back in the rendered Markdown', + ); expect(copyText).toContain('- [x] Write the changelog'); expect(copyText).toContain('- [ ] Tag the release'); }); it('puts that same Markdown on the clipboard', async function () { const card = await canvas.cardWithTitle(title); - await card.$('[data-testid="copy-button"]').click(); - await browser.pause(600); + await card.$(testid('copy-button')).click(); - const copied = await clipboardText(); + // ⚠️ An unreadable clipboard answers null at once, so only the readable case waits. + const copied = await eventually( + () => clipboardText(), + (text) => text === null || text.includes('- [x] Write the changelog'), + 'the copy to reach the clipboard', + ); if (copied === null) { // No readable clipboard on this runner. Skipped rather than returned: a bare // `return` is a green test that asserted nothing. See `clipboardText`. diff --git a/e2e/specs/08-placeholder-fields.e2e.ts b/e2e/specs/08-placeholder-fields.e2e.ts index 40a4ad32..32e15f1d 100644 --- a/e2e/specs/08-placeholder-fields.e2e.ts +++ b/e2e/specs/08-placeholder-fields.e2e.ts @@ -1,10 +1,10 @@ -import { browser, expect } from '@wdio/globals'; +import { expect } from '@wdio/globals'; import { canvas } from '../pageobjects/canvas.page.js'; import { editor } from '../pageobjects/editor.page.js'; import { fieldsForm } from '../pageobjects/overlays.page.js'; import { settings, variables } from '../pageobjects/titlebar.page.js'; -import { clipboardText, reloadCanvas } from '../support/app.js'; +import { clipboardText, eventually, reloadCanvas } from '../support/app.js'; import { bridge, draft, homeSpaceId, query } from '../support/bridge.js'; /** @@ -77,8 +77,12 @@ describe('{{fields}} in a snippet', () => { expect(await editor.hasCopyFilled()).toBe(true); await editor.copyFilled(); - await browser.pause(800); - const filled = await clipboardText(); + // ⚠️ An unreadable clipboard answers null at once, so only the readable case waits. + const filled = await eventually( + () => clipboardText(), + (text) => text === null || text.includes('-p 5432'), + 'the filled copy to reach the clipboard', + ); await editor.close(); if (filled === null) { @@ -118,9 +122,13 @@ describe('{{fields}} in a snippet', () => { await fieldsForm.field('host').setValue('db.internal'); await fieldsForm.field('user').setValue('reader'); await fieldsForm.submit(); - await browser.pause(800); + await fieldsForm.form().waitForExist({ reverse: true, timeout: 10_000 }); - const fields = (await reread())?.placeholders ?? []; + const fields = await eventually( + async () => (await reread())?.placeholders ?? [], + (stored) => stored.find((field) => field.name === 'host')?.value === 'db.internal', + 'the typed values to be stored', + ); expect(fields.find((field) => field.name === 'host')?.value).toBe('db.internal'); expect(fields.find((field) => field.name === 'user')?.value).toBe('reader'); }); @@ -155,9 +163,17 @@ describe('{{fields}} in a snippet', () => { await fieldsForm.form().waitForExist({ timeout: 10_000 }); await fieldsForm.field('host').setValue('db.other'); await fieldsForm.submit(); - await browser.pause(800); + // ⚠️ The form closing is its own condition, and the next scenario opens a panel over + // this one: waiting on the stored value alone let the two dialogs overlap. + await fieldsForm.form().waitForExist({ reverse: true, timeout: 10_000 }); - expect((await reread())?.updatedAt).toBe(before); + // The write is waited on through the value, then the column it must *not* have moved. + const after = await eventually( + () => reread(), + (note) => note?.placeholders.find((field) => field.name === 'host')?.value === 'db.other', + 'the new value to be stored', + ); + expect(after?.updatedAt).toBe(before); }); it('lets a global variable propose a value without freezing it', async () => { diff --git a/e2e/specs/12-preferences.e2e.ts b/e2e/specs/12-preferences.e2e.ts index 69468f44..653b1b15 100644 --- a/e2e/specs/12-preferences.e2e.ts +++ b/e2e/specs/12-preferences.e2e.ts @@ -2,7 +2,7 @@ import { browser, expect } from '@wdio/globals'; import { canvas } from '../pageobjects/canvas.page.js'; import { fileMenu, settings, titlebar } from '../pageobjects/titlebar.page.js'; -import { cursorOf, press, reopenSession } from '../support/app.js'; +import { cursorOf, eventually, press, reopenSession } from '../support/app.js'; /** * A preference applies as it is typed, one key at a time. @@ -76,7 +76,11 @@ describe('Preferences', () => { it('keeps the titlebar switch and the panel in agreement', async () => { await titlebar.setLocale('fr'); - await browser.pause(300); + await eventually( + () => titlebar.activeLocale(), + (locale) => locale === 'fr', + 'the titlebar to settle on the locale it was given', + ); await fileMenu.openPreferences(); expect(await settings.locale().getValue()).toBe('fr'); diff --git a/e2e/specs/13-multiple-selection.e2e.ts b/e2e/specs/13-multiple-selection.e2e.ts index a7aae489..87564730 100644 --- a/e2e/specs/13-multiple-selection.e2e.ts +++ b/e2e/specs/13-multiple-selection.e2e.ts @@ -1,4 +1,4 @@ -import { browser, expect } from '@wdio/globals'; +import { expect } from '@wdio/globals'; import { canvas } from '../pageobjects/canvas.page.js'; import { selectionBar, undoBar } from '../pageobjects/overlays.page.js'; @@ -51,8 +51,12 @@ describe('Selecting several notes at once', () => { it('tags every ticked note in one call, and only those', async () => { await selectionBar.tag('batch'); - await browser.pause(800); + await eventually( + () => reread(second), + (note) => note?.tags.includes('batch') === true, + 'the batch to reach the last note it was given', + ); expect((await reread(first))?.tags).toEqual(['batch']); expect((await reread(second))?.tags).toEqual(['batch']); expect((await reread(untouched))?.tags).toEqual([]); @@ -84,11 +88,14 @@ describe('Selecting several notes at once', () => { it('copies the selection as Markdown', async function () { await selectionBar.copy(); - await browser.pause(800); // `copyConfirmation` is on by default, and the toast is the only thing saying the // copy happened. - expect(await banners.status().isExisting()).toBe(true); + await eventually( + () => banners.status().isExisting(), + (showing) => showing, + 'the copy confirmation to appear', + ); const copied = await clipboardText(); if (copied === null) { @@ -103,8 +110,12 @@ describe('Selecting several notes at once', () => { it('moves them to another space through the renamed argument', async () => { await selectionBar.moveTo(refugeId); - await browser.pause(800); + await eventually( + () => reread(second), + (note) => note?.spaceId === refugeId, + 'the batch to reach the last note it was given', + ); expect((await reread(first))?.spaceId).toBe(refugeId); expect((await reread(second))?.spaceId).toBe(refugeId); expect((await reread(untouched))?.spaceId).toBe(spaceId); diff --git a/e2e/specs/14-tag-manager.e2e.ts b/e2e/specs/14-tag-manager.e2e.ts index d58cf1e4..540134fc 100644 --- a/e2e/specs/14-tag-manager.e2e.ts +++ b/e2e/specs/14-tag-manager.e2e.ts @@ -2,7 +2,7 @@ import { browser, expect } from '@wdio/globals'; import { canvas } from '../pageobjects/canvas.page.js'; import { tagManager } from '../pageobjects/overlays.page.js'; -import { reloadCanvas } from '../support/app.js'; +import { eventually, reloadCanvas } from '../support/app.js'; import { bridge, draft, homeSpaceId, query } from '../support/bridge.js'; /** @@ -57,9 +57,10 @@ describe('Managing the tags of the whole corpus', () => { expect((await bridge.listTags()).map((usage) => usage.tag)).toContain('staging'); await tagManager.cancel(); - await browser.pause(400); - // Cancelling is the whole point of asking: nothing moved. + // ⚠️ A duration, deliberately: cancelling is the whole point of asking, so this + // asserts nothing moved — which is not a condition anything can wait on. + await browser.pause(400); expect(await tagsOf('Tagged alpha')).toEqual(['staging']); }); @@ -68,8 +69,12 @@ describe('Managing the tags of the whole corpus', () => { await tagManager.setTarget('recette'); await tagManager.apply(); - await browser.pause(800); + await eventually( + () => tagsOf('Tagged alpha'), + (tags) => tags.includes('recette'), + 'the rename to reach the notes it was aimed at', + ); expect(await tagsOf('Tagged alpha')).toEqual(['recette']); expect((await tagsOf('Tagged beta')).sort()).toEqual(['recette', 'urgent']); // Untouched: a rename acts on what was selected, not on every tag. @@ -84,9 +89,13 @@ describe('Managing the tags of the whole corpus', () => { await tagManager.select('preprod'); await tagManager.setTarget('recette'); await tagManager.apply(); - await browser.pause(800); // `(note_id, tag)` is NOCASE, so the target is swept along with the sources. + await eventually( + () => tagsOf('Tagged gamma'), + (tags) => tags.includes('recette'), + 'the merge to reach the note that carried the source', + ); expect(await tagsOf('Tagged gamma')).toEqual(['recette']); expect(await tagsOf('Tagged alpha')).toEqual(['recette']); @@ -102,10 +111,14 @@ describe('Managing the tags of the whole corpus', () => { await tagManager.select('recette'); await tagManager.setTarget('Recette'); await tagManager.apply(); - await browser.pause(800); // Same rows, different spelling: the value has to be rewritten rather than ignored // as a duplicate. + await eventually( + () => tagsOf('Tagged alpha'), + (tags) => tags.includes('Recette'), + 'the case correction to be written rather than dropped', + ); expect(await tagsOf('Tagged alpha')).toEqual(['Recette']); }); @@ -115,8 +128,12 @@ describe('Managing the tags of the whole corpus', () => { await tagManager.select('urgent'); await tagManager.delete(); - await browser.pause(800); + await eventually( + () => bridge.listTags(), + (tags) => !tags.some((usage) => usage.tag === 'urgent'), + 'the deleted tag to leave the corpus', + ); expect((await bridge.listTags()).map((usage) => usage.tag)).not.toContain('urgent'); // The note survived its tag. expect(await tagsOf('Tagged beta')).toEqual(['Recette']); @@ -129,9 +146,12 @@ describe('Managing the tags of the whole corpus', () => { ?.updatedAt; await bridge.renameTag('Recette', 'production'); - await browser.pause(400); - const after = (await bridge.queryNotes(query({ search: 'Tagged alpha' }))).sections[0]?.notes[0]; + const after = await eventually( + async () => (await bridge.queryNotes(query({ search: 'Tagged alpha' }))).sections[0]?.notes[0], + (note) => note?.tags.includes('production') === true, + 'the global retag to reach the note', + ); expect(after?.tags).toEqual(['production']); // The canvas sorts on that column and would float notes nobody reopened to the top. expect(after?.updatedAt).toBe(before); diff --git a/e2e/specs/15-preferences-on-disk.e2e.ts b/e2e/specs/15-preferences-on-disk.e2e.ts index 8e76ac31..a959b2d8 100644 --- a/e2e/specs/15-preferences-on-disk.e2e.ts +++ b/e2e/specs/15-preferences-on-disk.e2e.ts @@ -1,8 +1,9 @@ -import { browser, expect } from '@wdio/globals'; +import { expect } from '@wdio/globals'; import { existsSync, readFileSync } from 'node:fs'; import { canvas } from '../pageobjects/canvas.page.js'; import { fileMenu, settings, titlebar } from '../pageobjects/titlebar.page.js'; +import { eventually } from '../support/app.js'; import { preferencesPath } from '../support/profile.js'; /** @@ -15,9 +16,6 @@ import { preferencesPath } from '../support/profile.js'; * so this file only says anything about the location on Linux. See `support/profile.ts`. */ describe('Preferences reach the disk', () => { - /** Longer than the plugin's `autoSave` debounce, with room for a slow runner. */ - const FLUSH_MS = 1_500; - function stored(): Record { const path = preferencesPath(); if (!existsSync(path)) { @@ -26,6 +24,20 @@ describe('Preferences reach the disk', () => { return JSON.parse(readFileSync(path, 'utf8')) as Record; } + /** + * ⚠️ The file *is* the condition, and it is polled from Node: the page can only say the + * in-memory map moved, which is the one thing this file exists not to trust. The fixed + * wait it replaces was longer than the plugin's `autoSave` debounce, and paid that worst + * case on every assertion. + */ + function settled(key: string, want: unknown): Promise> { + return eventually( + async () => (existsSync(preferencesPath()) ? stored() : {}), + (file) => file[key] === want, + `${key} to reach the preferences file as ${String(want)}`, + ); + } + before(canvas.open); it('writes the file at all, next to the database', async () => { @@ -33,7 +45,7 @@ describe('Preferences reach the disk', () => { await fileMenu.openPreferences(); await settings.select(settings.control.theme, 'dark'); await settings.close(); - await browser.pause(FLUSH_MS); + await settled('devbox.theme', 'dark'); expect(existsSync(preferencesPath())).toBe(true); }); @@ -42,9 +54,8 @@ describe('Preferences reach the disk', () => { await fileMenu.openPreferences(); await settings.select(settings.control.density, 'comfortable'); await settings.close(); - await browser.pause(FLUSH_MS); - const file = stored(); + const file = await settled('devbox.density', 'comfortable'); // ⚠️ One key per setting: a blob under one key would make a half-written file lose // every setting at once. expect(file['devbox.theme']).toBe('dark'); @@ -55,17 +66,15 @@ describe('Preferences reach the disk', () => { await fileMenu.openPreferences(); await settings.select(settings.control.theme, 'light'); await settings.close(); - await browser.pause(FLUSH_MS); // There is no OK anywhere in the panel: closing it is not what saves. - expect(stored()['devbox.theme']).toBe('light'); + expect((await settled('devbox.theme', 'light'))['devbox.theme']).toBe('light'); }); it('stores the locale the titlebar switch chose, like the panel does', async () => { await titlebar.setLocale('en'); - await browser.pause(FLUSH_MS); - expect(stored()['devbox.locale']).toBe('en'); + expect((await settled('devbox.locale', 'en'))['devbox.locale']).toBe('en'); expect(await titlebar.activeLocale()).toBe('en'); }); diff --git a/e2e/specs/17-folders.e2e.ts b/e2e/specs/17-folders.e2e.ts index e35ffdc9..2ffa34a6 100644 --- a/e2e/specs/17-folders.e2e.ts +++ b/e2e/specs/17-folders.e2e.ts @@ -1,8 +1,8 @@ -import { browser, expect } from '@wdio/globals'; +import { expect } from '@wdio/globals'; import { canvas } from '../pageobjects/canvas.page.js'; import { crumb, folders, selectionBar, spaces } from '../pageobjects/overlays.page.js'; -import { reloadCanvas, testid, waitForCanvas } from '../support/app.js'; +import { eventually, reloadCanvas, testid, waitForCanvas } from '../support/app.js'; import { bridge, draft, homeSpaceId, query } from '../support/bridge.js'; /** @@ -47,18 +47,25 @@ describe('Folders', () => { it('creates a folder from the switcher', async () => { await folders.open(); await folders.create('Migrations'); - await browser.pause(500); - expect((await bridge.listFolders(homeId)).map((folder) => folder.name)).toEqual(['Migrations']); + const made = await eventually( + () => bridge.listFolders(homeId), + (listed) => listed.some((folder) => folder.name === 'Migrations'), + 'the new folder to be listed', + ); + expect(made.map((folder) => folder.name)).toEqual(['Migrations']); }); /** Assigned rather than chosen, so two made back to back never come out the same. */ it('gives the next one a different colour without being asked', async () => { await folders.open(); await folders.create('Perf'); - await browser.pause(500); - const made = await bridge.listFolders(homeId); + const made = await eventually( + () => bridge.listFolders(homeId), + (listed) => listed.some((folder) => folder.name === 'Perf'), + 'the second folder to be listed', + ); expect(made.map((folder) => folder.name)).toEqual(['Migrations', 'Perf']); expect(made[0]?.colour).not.toBe(made[1]?.colour); }); @@ -96,9 +103,12 @@ describe('Folders', () => { await canvas.check('Locks sur jobs'); await canvas.check('Cache hit ratio'); await selectionBar.fileInto(perfId); - await browser.pause(800); - const view = await bridge.queryNotes(query({ spaceId: homeId, folderId: perfId })); + const view = await eventually( + () => bridge.queryNotes(query({ spaceId: homeId, folderId: perfId })), + (filed) => filed.matched === 2, + 'both ticked notes to be filed', + ); expect(view.matched).toBe(2); }); @@ -141,10 +151,13 @@ describe('Folders', () => { it('takes a selection back out of its folder', async () => { await canvas.check('Cache hit ratio'); await selectionBar.fileInto(null); - await browser.pause(800); - await selectionBar.clear(); - const view = await bridge.queryNotes(query({ spaceId: homeId, folderId: perfId })); + const view = await eventually( + () => bridge.queryNotes(query({ spaceId: homeId, folderId: perfId })), + (left) => left.matched === 1, + 'the unfiled note to leave the folder', + ); + await selectionBar.clear(); expect(view.matched).toBe(1); }); @@ -168,12 +181,20 @@ describe('Folders', () => { await folders.open(); await folders.rename(before.id, 'Schéma'); - await browser.pause(500); + await eventually( + async () => (await bridge.listFolders(homeId)).find((folder) => folder.id === before.id), + (folder) => folder?.name === 'Schéma', + 'the rename to land before the colour is changed', + ); + await folders.open(); await folders.recolour(before.id, 'red'); - await browser.pause(500); - const after = (await bridge.listFolders(homeId)).find((folder) => folder.id === before.id); + const after = await eventually( + async () => (await bridge.listFolders(homeId)).find((folder) => folder.id === before.id), + (folder) => folder?.colour === 'red', + 'the new colour to be stored', + ); expect(after?.name).toBe('Schéma'); expect(after?.colour).toBe('red'); }); @@ -196,9 +217,12 @@ describe('Folders', () => { await folders.open(); await folders.remove(doomed.id); - await browser.pause(800); - expect((await bridge.listFolders(homeId)).map((folder) => folder.name)).not.toContain('Jetable'); + await eventually( + () => bridge.listFolders(homeId), + (listed) => !listed.some((folder) => folder.name === 'Jetable'), + 'the deleted folder to be gone', + ); expect((await bridge.queryNotes(query({ spaceId: homeId }))).matched).toBe(corpus); // Standing, and now loose — the chip is what a `SET NULL` takes away. diff --git a/e2e/specs/18-board.e2e.ts b/e2e/specs/18-board.e2e.ts index 99e6a0d1..1898d4e3 100644 --- a/e2e/specs/18-board.e2e.ts +++ b/e2e/specs/18-board.e2e.ts @@ -89,8 +89,12 @@ describe('The board', () => { /** ⚠️ Dimmed in place: reflowing throws away the only thing the board has. */ it('dims what a search does not match rather than removing it', async () => { await canvas.search('EXPLAIN'); - await browser.pause(600); + await eventually( + () => board.isDimmed('Dump nocturne'), + (dimmed) => dimmed, + 'the board to dim what the search does not match', + ); expect(await board.zoneTitles('Perf')).toEqual(['EXPLAIN lent sur join']); expect(await board.looseTitles()).toEqual(['Dump nocturne']); expect(await board.isDimmed('Dump nocturne')).toBe(true); @@ -174,9 +178,12 @@ describe('The board', () => { const card = browser.$(`${testid('note-card')}[data-note-id="${list.id}"]`); await card.$(testid('note-card-item')).click(); - await browser.pause(900); - const view = await bridge.queryNotes(query({ spaceId, search: 'Avant la release' })); + const view = await eventually( + () => bridge.queryNotes(query({ spaceId, search: 'Avant la release' })), + (read) => read.sections[0]?.notes[0]?.items?.[0]?.done === true, + 'the tick to reach the note behind the board', + ); expect(view.sections[0]?.notes[0]?.items?.[0]?.done).toBe(true); // Drawn from what the board re-read, not from the date view behind it. expect(await card.$(testid('note-card-item')).getAttribute('aria-checked')).toBe('true'); diff --git a/e2e/specs/19-board-gesture.e2e.ts b/e2e/specs/19-board-gesture.e2e.ts index db5ea9e1..28c33e53 100644 --- a/e2e/specs/19-board-gesture.e2e.ts +++ b/e2e/specs/19-board-gesture.e2e.ts @@ -2,7 +2,7 @@ import { browser, expect } from '@wdio/globals'; import { canvas } from '../pageobjects/canvas.page.js'; import { board, selectionBar, spaces } from '../pageobjects/overlays.page.js'; -import { reloadCanvas, waitForCanvas } from '../support/app.js'; +import { eventually, reloadCanvas, waitForCanvas } from '../support/app.js'; import { bridge, draft, homeSpaceId, query } from '../support/bridge.js'; /** @@ -134,9 +134,12 @@ describe('Arranging the board', () => { it('creates a folder from a band drawn on the background', async () => { await board.drawZone({ x: 60, y: 900, width: 420, height: 320 }); await board.nameZone('Reporting'); - await browser.pause(600); - const made = (await bridge.listFolders(spaceId)).find((folder) => folder.name === 'Reporting'); + const made = await eventually( + async () => (await bridge.listFolders(spaceId)).find((folder) => folder.name === 'Reporting'), + (folder) => folder !== undefined, + 'the drawn band to become a folder', + ); expect(made).toBeDefined(); await openBoard(); @@ -153,10 +156,16 @@ describe('Arranging the board', () => { it('files a note with no pointer at all, from the selection bar', async () => { await canvas.check('EXPLAIN lent sur join'); await selectionBar.fileInto(migrationsId); - await browser.pause(900); - await selectionBar.clear(); - const view = await bridge.queryNotes(query({ spaceId, folderId: migrationsId })); + const view = await eventually( + () => bridge.queryNotes(query({ spaceId, folderId: migrationsId })), + (filed) => + filed.sections + .flatMap((section) => section.notes) + .some((note) => note.title === 'EXPLAIN lent sur join'), + 'the keyboard filing to land', + ); + await selectionBar.clear(); expect(view.sections.flatMap((section) => section.notes).map((note) => note.title)).toContain( 'EXPLAIN lent sur join', ); @@ -165,10 +174,16 @@ describe('Arranging the board', () => { it('takes it back out again with no pointer either', async () => { await canvas.check('EXPLAIN lent sur join'); await selectionBar.fileInto(null); - await browser.pause(900); - await selectionBar.clear(); - const view = await bridge.queryNotes(query({ spaceId, folderId: migrationsId })); + const view = await eventually( + () => bridge.queryNotes(query({ spaceId, folderId: migrationsId })), + (left) => + !left.sections + .flatMap((section) => section.notes) + .some((note) => note.title === 'EXPLAIN lent sur join'), + 'the keyboard unfiling to land', + ); + await selectionBar.clear(); expect(view.sections.flatMap((section) => section.notes).map((note) => note.title)).not.toContain( 'EXPLAIN lent sur join', ); diff --git a/e2e/specs/20-folder-descent.e2e.ts b/e2e/specs/20-folder-descent.e2e.ts index 62c3b8f4..76ff576f 100644 --- a/e2e/specs/20-folder-descent.e2e.ts +++ b/e2e/specs/20-folder-descent.e2e.ts @@ -3,7 +3,7 @@ import { browser, expect } from '@wdio/globals'; import { canvas } from '../pageobjects/canvas.page.js'; import { editor } from '../pageobjects/editor.page.js'; import { board, crumb, spaces } from '../pageobjects/overlays.page.js'; -import { blurField, press, reloadCanvas, testid, waitForCanvas } from '../support/app.js'; +import { blurField, eventually, press, reloadCanvas, testid, waitForCanvas } from '../support/app.js'; import { bridge, draft, homeSpaceId, query } from '../support/bridge.js'; /** @@ -88,9 +88,21 @@ describe('Opening a folder', () => { await canvas.createSnippet(); await editor.setTitle('Cache hit ratio'); await editor.close(); - await browser.pause(900); - const view = await bridge.queryNotes(query({ spaceId, folderId: perfId })); + // ⚠️ Two conditions and not one: the note has to exist before "filed" means anything, + // and keeping them apart is what tells a note never written from one written loose. + await eventually( + () => bridge.queryNotes(query({ spaceId, search: 'Cache hit ratio' })), + (found) => found.matched > 0, + 'the note made here to be written at all', + ); + + const view = await eventually( + () => bridge.queryNotes(query({ spaceId, folderId: perfId })), + (filed) => + filed.sections.flatMap((section) => section.notes).some((note) => note.title === 'Cache hit ratio'), + 'the note made here to arrive filed', + ); expect(view.sections.flatMap((section) => section.notes).map((note) => note.title)).toContain( 'Cache hit ratio', ); @@ -98,9 +110,14 @@ describe('Opening a folder', () => { it('renames the folder from beside the breadcrumb', async () => { await crumb.rename('Performance'); - await browser.pause(700); - expect(await crumb.name()).toBe('Performance'); + expect( + await eventually( + () => crumb.name(), + (name) => name === 'Performance', + 'the breadcrumb to carry the new name', + ), + ).toBe('Performance'); expect((await bridge.listFolders(spaceId)).map((folder) => folder.name)).toContain('Performance'); }); @@ -146,9 +163,12 @@ describe('Opening a folder', () => { const before = (await bridge.queryNotes(query({ spaceId }))).matched; await crumb.remove(); - await browser.pause(900); - expect(await crumb.isShowing()).toBe(false); + await eventually( + () => crumb.isShowing(), + (showing) => !showing, + 'the breadcrumb to go with the folder it named', + ); expect((await bridge.listFolders(spaceId)).map((folder) => folder.name)).not.toContain('Performance'); expect((await bridge.queryNotes(query({ spaceId }))).matched).toBe(before); }); diff --git a/package.json b/package.json index dc4b47de..ac097980 100644 --- a/package.json +++ b/package.json @@ -15,7 +15,7 @@ "test": "ng test", "test:watch": "ng test --watch", "test:coverage": "ng test --coverage", - "test:scripts": "node --test scripts/release-notes.test.mjs scripts/palette.test.mjs", + "test:scripts": "node --test scripts/release-notes.test.mjs scripts/palette.test.mjs scripts/e2e-waits.test.mjs", "build:e2e": "ng build --configuration e2e", "e2e:build": "tauri build --debug --no-bundle --features e2e --config src-tauri/tauri.e2e.conf.json", "test:e2e": "tsx e2e/reset-profile.ts && wdio run e2e/wdio.conf.ts", diff --git a/scripts/e2e-waits.test.mjs b/scripts/e2e-waits.test.mjs new file mode 100644 index 00000000..e3c38d01 --- /dev/null +++ b/scripts/e2e-waits.test.mjs @@ -0,0 +1,70 @@ +import assert from 'node:assert/strict'; +import { readdirSync, readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { describe, it } from 'node:test'; + +/** + * ⚠️ `node --test` and not a `*.spec.ts`, like the palette file beside it: this reads the + * shipped spec files off disk, and the Angular builder compiles its own specs for a browser. + */ +const SPECS = 'e2e/specs'; + +/** How far after a `browser.pause` something still counts as waiting on it. */ +const REACH = 4; + +/** The reach of the comment that excuses one, which sits directly above. */ +const EXCUSE_REACH = 3; + +/** + * ⚠️ This is the lookahead #190 got wrong. It demanded a literal `expect(` on the **very + * next line**, which misses the two shapes that are everywhere: a value read now and + * asserted two lines down, and a blank line between the pause and the assertion. Counted + * properly, the sweep had covered a sixth of the job. + */ +const READS = /expect\(|const .* = await|await (bridge|canvas|editor|board|crumb|trash|settings)\./; + +/** + * The one exception, and it has to say so: an assertion that **nothing** happened cannot + * wait on a condition, because nothing arriving is not one. Everything else waits through + * `eventually` in `e2e/support/app.ts`. + */ +const DELIBERATE = /deliberately/; + +function sleepingAssertions() { + const found = []; + + for (const name of readdirSync(SPECS).filter((file) => file.endsWith('.e2e.ts'))) { + const lines = readFileSync(join(SPECS, name), 'utf8').split('\n'); + + lines.forEach((line, index) => { + if (!line.includes('browser.pause')) return; + + const after = lines.slice(index + 1, index + 1 + REACH).join('\n'); + const above = lines.slice(Math.max(0, index - EXCUSE_REACH), index).join('\n'); + if (READS.test(after) && !DELIBERATE.test(above)) { + found.push(`${name}:${index + 1}`); + } + }); + } + + return found; +} + +describe('a scenario waits on a condition, not on a duration', () => { + it('has no assertion sleeping behind a browser.pause', () => { + const sleeping = sleepingAssertions(); + + assert.deepEqual( + sleeping, + [], + `these wait on a duration before reading or asserting:\n ${sleeping.join('\n ')}\n` + + 'Use `eventually(read, matches, what)`, or say in a comment above why the wait is deliberate.', + ); + }); + + it('finds the specs at all, so a moved directory cannot pass by finding nothing', () => { + const specs = readdirSync(SPECS).filter((file) => file.endsWith('.e2e.ts')); + + assert.ok(specs.length > 15, `expected the whole suite under ${SPECS}, saw ${specs.length}`); + }); +});