diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 38c8af03d..1ac7b2589 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -89,11 +89,15 @@ jobs: # itself without VITE_METRICS, which is exactly how it would silently # stop running. TRANSPORT_SKIPPED=$(jq '[.suites[] | recurse(.suites[]?) | .specs[]? | select(.file == "network-transport.spec.ts") | .tests[]?.results[]? | select(.status == "skipped")] | length' tests/functional/results.json) + # The TOTAL floor above sits far below the suite size, so it would + # not notice this file dropping out of the run list again. + SETTINGS_RAN=$(jq '[.suites[] | recurse(.suites[]?) | .specs[]? | select(.file == "host-settings.spec.ts")] | length' tests/functional/results.json) + SETTINGS_SKIPPED=$(jq '[.suites[] | recurse(.suites[]?) | .specs[]? | select(.file == "host-settings.spec.ts") | .tests[]?.results[]? | select(.status == "skipped")] | length' tests/functional/results.json) echo "loading.spec.ts failures: $LOADING_FAILED" echo "resolution.spec.ts failures: $RESOLUTION_FAILED" echo "network-transport.spec.ts failures: $TRANSPORT_FAILED (skipped: $TRANSPORT_SKIPPED)" echo "ui-smoke.spec.ts failures: $SMOKE_FAILED" - echo "host-settings.spec.ts failures: $SETTINGS_FAILED" + echo "host-settings.spec.ts failures: $SETTINGS_FAILED (ran: $SETTINGS_RAN, skipped: $SETTINGS_SKIPPED)" if [ "$LOADING_FAILED" -gt 0 ]; then echo "::error::loading.spec.ts must have 0 failures" exit 1 @@ -118,6 +122,14 @@ jobs: echo "::error::network-transport.spec.ts skipped $TRANSPORT_SKIPPED test(s); VITE_METRICS is not reaching the run step" exit 1 fi + if [ "$SETTINGS_RAN" -lt 28 ]; then + echo "::error::expected at least 28 host-settings.spec.ts tests, got $SETTINGS_RAN" + exit 1 + fi + if [ "$SETTINGS_SKIPPED" -gt 0 ]; then + echo "::error::host-settings.spec.ts skipped $SETTINGS_SKIPPED test(s); a skip reads as a pass in both gates above" + exit 1 + fi # E2E run of host-playground product tests against the local preview server, # with pairing and signing handled headlessly by the truapi-host CLI from diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0cb121cd5..fe84239b7 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -9,30 +9,40 @@ - **Structure with Given / When / Then.** Every multi-step test body uses `// Given`, `// When`, `// Then` comments to separate setup, action, and assertions. ```ts -test("As a user using per-product smoldot, the host must only spawn one instance of the light client", async ({ - page, +test("As a user on a per-tab light client who turns the dotNS cache off, every visit looks the name up again", async ({ + browser, }) => { // Given - await setBackend(page, "smoldot-direct"); - await mockProtocolIframe(page, successfulResolveResponse("bafyfake...")); - const workerUrls: string[] = []; - page.on("worker", (w) => { - workerUrls.push(w.url()); + const { context, page } = await setupTest(browser, { + backend: "smoldot-direct", + cacheSeed: CACHE_ENABLED, }); + await page.goto(BASE_URL, { waitUntil: "commit" }); + await waitForResolutionOutcome(page, TIMEOUT_MS, "smoldot-direct"); + await waitForCachedCid(page, DOMAIN, 5_000); - // When - await page.goto(HOST_URL, { waitUntil: "domcontentloaded" }); - await page.waitForTimeout(5_000); + try { + // When + await updateCacheSettings(page, SKIP_CID_ONLY); + await page.goto(BASE_URL, { waitUntil: "commit" }); - // Then - const hostShellOrigin = `http://${DOMAIN}.localhost:${PORT}`; - const hostShellSmoldotWorkers = workerUrls.filter( - (url) => url.startsWith(hostShellOrigin) && url.includes("smoldot_worker"), - ); - expect(hostShellSmoldotWorkers).toEqual([]); + // Then + await waitForResolutionOutcome(page, TIMEOUT_MS, "smoldot-direct"); + expect(await hostResolveStarted(page)).toBe(true); + expect(await hasCachedCid(page, DOMAIN)).toBe(true); + } finally { + await context.close(); + } }); ``` +Note what the last assertion buys. The Given already proves an entry existed, so +without it the test would still pass if something deleted that entry between the +two visits, and the assertion above it would then mean "nothing to skip" rather +than "skipped". A test that can pass for a reason other than the +one its title gives is worse than no test, because the suite reports it as +coverage. + ### How to Document Good documentation starts with a single, clear sentence. Everything else comes after a newline. diff --git a/apps/host/tests/functional/fixtures/settings.ts b/apps/host/tests/functional/fixtures/settings.ts index e99794043..7bb663526 100644 --- a/apps/host/tests/functional/fixtures/settings.ts +++ b/apps/host/tests/functional/fixtures/settings.ts @@ -15,6 +15,22 @@ export const BACKENDS = [ export type Backend = (typeof BACKENDS)[number]; +/** + * How each network transport is named in a user story. + * + * Test titles are read by people deciding whether a behaviour is covered, so + * they name the transport after the choice the settings screen offers rather + * than by its stored value. The screen's own wording is "Light Client + * Per-Tab", "Light Client Shared" and "Trusted Providers" + * (`packages/config/src/mode.ts`), reworded here only to read as prose mid + * sentence. + */ +export const TRANSPORT_LABELS: Record = { + "smoldot-shared-worker": "a shared light client", + "smoldot-direct": "a per-tab light client", + "rpc-gateway": "trusted providers", +}; + export interface CacheSeed { skipCidCache: boolean; skipArchiveCache: boolean; diff --git a/apps/host/tests/functional/helpers/cache.ts b/apps/host/tests/functional/helpers/cache.ts index 601ae5af2..124d36103 100644 --- a/apps/host/tests/functional/helpers/cache.ts +++ b/apps/host/tests/functional/helpers/cache.ts @@ -20,26 +20,36 @@ export function hostResolveStarted(page: Page): Promise { /** * Browser-side check for a cached CID entry under `label`. * - * Defined as a standalone function so the two Playwright entry points - * below (`hasCachedCid` via `page.evaluate`, `waitForCachedCid` via - * `page.waitForFunction`) share one IDB query body instead of two - * copies that can drift. + * Opened without a version so the request adopts whatever schema the app + * created. Naming one pins the probe to a number that + * `packages/storage/src/db.ts` is free to bump, and a lower number fails + * the open with `VersionError`, which reads here as "nothing cached". */ const cachedCidExists = (label: string): Promise => new Promise((resolve) => { const open = indexedDB.open("dotli"); open.onsuccess = () => { + const db = open.result; + // `waitForCachedCid` calls this on a timer, so without the close a page + // accumulates one handle per poll. Each of those blocks a schema upgrade + // and the `deleteDatabase` sweep in `packages/ui/src/settings-actions.ts`, + // neither of which any functional test reaches after a probe, so this is + // hygiene rather than a fix for an observed failure. + const done = (found: boolean): void => { + db.close(); + resolve(found); + }; try { - const tx = open.result.transaction("cids", "readonly"); + const tx = db.transaction("cids", "readonly"); const req = tx.objectStore("cids").get(label); req.onsuccess = () => { - resolve(req.result !== undefined); + done(req.result !== undefined); }; req.onerror = () => { - resolve(false); + done(false); }; } catch { - resolve(false); + done(false); } }; open.onerror = () => { diff --git a/apps/host/tests/functional/host-settings.spec.ts b/apps/host/tests/functional/host-settings.spec.ts index ce3f31cd0..23fcb6e45 100644 --- a/apps/host/tests/functional/host-settings.spec.ts +++ b/apps/host/tests/functional/host-settings.spec.ts @@ -2,7 +2,7 @@ // SPDX-License-Identifier: AGPL-3.0-only /** - * Host shell settings: cache flags and chain backend selection. + * Host shell settings: cache flags and network transport selection. * * `skipWorkerCache` is not covered. The flag triggers an IDB purge sweep * in `apps/protocol/src/main.ts`, but the protocol-origin IDB it targets @@ -21,6 +21,7 @@ import { expect } from "@playwright/test"; import type { Page } from "@playwright/test"; +import { defaultBackend } from "@dotli/config/mode"; import { DOMAIN, PORT, TIMEOUT_MS } from "../env"; import { setupTest } from "./helpers/context"; import { waitForResolutionOutcome } from "../product-frame"; @@ -36,6 +37,7 @@ import { CACHE_ENABLED, SKIP_ARCHIVE_ONLY, SKIP_CID_ONLY, + TRANSPORT_LABELS, updateCacheSettings, } from "./fixtures/settings"; import { test } from "./helpers/shared-mode-reset"; @@ -61,6 +63,17 @@ async function readChainBackendState( expected, { timeout: 10_000 }, ); + // A revisit already holds the right value in localStorage, so the wait above + // can be satisfied before the shell has canonicalised the URL. Every + // non-default transport ends up named in the address bar, so wait for that + // too. A default one is stripped, and there is no transition to wait for. + if (expected !== defaultBackend()) { + await page.waitForFunction( + (e) => window.location.href.includes(`chainBackend=${e}`), + expected, + { timeout: 10_000 }, + ); + } return page.evaluate(() => ({ chainBackend: localStorage.getItem("dotli:chain-backend"), cacheSettings: localStorage.getItem("dotli:cache-settings"), @@ -75,7 +88,7 @@ async function disableSharedWorker(page: Page): Promise { } test.describe("Settings works", () => { - test("As a first-time user, when I open an app it runs on its own smoldot instance for this tab", async ({ + test("As a first-time user, I get a per-tab light client without choosing one", async ({ page, }) => { // When @@ -101,7 +114,7 @@ test.describe("Settings works", () => { }); for (const backend of BACKENDS) { - test(`As a user opening a link that selects ${backend}, my session runs in that mode and stays there`, async ({ + test(`As a user opening a link that selects ${TRANSPORT_LABELS[backend]}, my session runs in that mode and stays there`, async ({ page, }) => { // When @@ -130,6 +143,9 @@ test.describe("Settings works", () => { // Then const state = await readChainBackendState(page, "smoldot-direct"); expect(state.chainBackend).toBe("smoldot-direct"); + // The link asked for the default mode, and a default axis is stripped from + // the address bar, so landing in it leaves a clean URL rather than one + // that still names it. See the contract in `packages/config/src/url-settings.ts`. expect(state.url).not.toContain("chainBackend="); }); @@ -144,9 +160,6 @@ test.describe("Settings works", () => { await page.goto(LANDING_URL); // Then - // localStorage already holds the mode, so the read below can pass before - // the app writes it into the address bar during startup. Wait for that. - await expect(page).toHaveURL(/[?&]chainBackend=rpc-gateway(&|$)/); const state = await readChainBackendState(page, "rpc-gateway"); expect(state.chainBackend).toBe("rpc-gateway"); expect(state.url).toContain("chainBackend=rpc-gateway"); @@ -197,14 +210,11 @@ test.describe("Settings works", () => { await page.goto(LANDING_URL); // Then - // localStorage already holds the mode, so the read below can pass before - // the app writes it into the address bar during startup. Wait for that. - await expect(page).toHaveURL(/[?&]chainBackend=smoldot-shared-worker(&|$)/); const state = await readChainBackendState(page, "smoldot-shared-worker"); expect(state.url).toContain("chainBackend=smoldot-shared-worker"); }); - test("As a user who picked trusted providers, my address bar records it on every visit", async ({ + test("As a user who picked trusted providers, my address bar records it", async ({ page, }) => { // Given @@ -216,9 +226,6 @@ test.describe("Settings works", () => { await page.goto(LANDING_URL); // Then - // localStorage already holds the mode, so the read below can pass before - // the app writes it into the address bar during startup. Wait for that. - await expect(page).toHaveURL(/[?&]chainBackend=rpc-gateway(&|$)/); const state = await readChainBackendState(page, "rpc-gateway"); expect(state.url).toContain("chainBackend=rpc-gateway"); }); @@ -243,11 +250,13 @@ test.describe("Settings works", () => { expect(cache.skipWorkerCache).toBe(false); expect(state.url).toContain("skipCidCache=1"); expect(state.url).toContain("skipArchiveCache=1"); + // The worker cache was left at its default, so it is stripped rather than + // written back as `=0`. Only the axes I actually changed travel in the link. expect(state.url).not.toContain("skipWorkerCache="); }); for (const backend of BACKENDS) { - test(`As a user on ${backend} with the dotNS cache on, revisiting a site skips looking its name up again`, async ({ + test(`As a user on ${TRANSPORT_LABELS[backend]} with the dotNS cache on, revisiting a site skips looking its name up again`, async ({ browser, }) => { // Given @@ -272,7 +281,7 @@ test.describe("Settings works", () => { } }); - test(`As a user on ${backend} who turns the dotNS cache off, every visit looks the name up again`, async ({ + test(`As a user on ${TRANSPORT_LABELS[backend]} who turns the dotNS cache off, every visit looks the name up again`, async ({ browser, }) => { // Given @@ -292,6 +301,12 @@ test.describe("Settings works", () => { // Then await waitForResolutionOutcome(page, TIMEOUT_MS, backend); expect(await hostResolveStarted(page)).toBe(true); + // The entry from the first visit is still here, which is what makes the + // assertion above mean "skipped the cache" rather than "had nothing to + // skip". It survives because `updateCacheSettings` writes the stored + // setting directly. A user flipping the same switch on the settings + // screen would also hit `clearCidCache` in + // `packages/ui/src/settings-actions.ts`, which no test covers. expect(await hasCachedCid(page, DOMAIN)).toBe(true); } finally { await context.close(); @@ -300,7 +315,7 @@ test.describe("Settings works", () => { } for (const backend of BACKENDS) { - test(`As a user on ${backend} with the archive cache on, revisiting a site checks my local copy first`, async ({ + test(`As a user on ${TRANSPORT_LABELS[backend]} with the archive cache on, revisiting a site checks my local copy first`, async ({ browser, }) => { // Given @@ -324,7 +339,7 @@ test.describe("Settings works", () => { } }); - test(`As a user on ${backend} who turns the archive cache off, the site is fetched fresh instead of from my local copy`, async ({ + test(`As a user on ${TRANSPORT_LABELS[backend]} who turns the archive cache off, the site is fetched fresh instead of from my local copy`, async ({ browser, }) => { // Given diff --git a/apps/host/tests/functional/loading.spec.ts b/apps/host/tests/functional/loading.spec.ts index 99989cfd6..8cf23e188 100644 --- a/apps/host/tests/functional/loading.spec.ts +++ b/apps/host/tests/functional/loading.spec.ts @@ -14,7 +14,11 @@ import { } from "../../src/errors"; import { test } from "./helpers/shared-mode-reset"; import { findAppFrame } from "../product-frame"; -import { seedBackend, type Backend } from "./fixtures/settings"; +import { + seedBackend, + TRANSPORT_LABELS, + type Backend, +} from "./fixtures/settings"; import { TIMEOUTS } from "@dotli/config/timeouts"; import { METHOD_TIMEOUTS } from "@dotli/protocol/method-timeouts"; @@ -151,7 +155,7 @@ const successfulResolveResponse = (cid: string): string => ` }); `; -test("As a user using smoldot directly, when the light client panics mid-resolution, I see the appropriate error and can switch backend", async ({ +test("As a user on a per-tab light client, when the light client panics mid-resolution, I see the appropriate error and can switch network transport", async ({ page, }) => { // Given @@ -179,7 +183,7 @@ test("As a user using smoldot directly, when the light client panics mid-resolut ); }); -test("As a user using smoldot in shared worker, when the light client panics mid-resolution, I see the appropriate error and can switch backend", async ({ +test("As a user on a shared light client, when the light client panics mid-resolution, I see the appropriate error and can switch network transport", async ({ page, }) => { // Given @@ -207,7 +211,7 @@ test("As a user using smoldot in shared worker, when the light client panics mid ); }); -test("As a user using smoldot in shared worker, when the browser can't create a worker, I see the appropriate error and can switch backend", async ({ +test("As a user on a shared light client, when the browser can't create a worker, I see the appropriate error and can switch network transport", async ({ page, }) => { // Given @@ -238,7 +242,7 @@ test("As a user using smoldot in shared worker, when the browser can't create a ); }); -test("As a user using smoldot in shared worker, when the worker dies silently, I see the appropriate error and can switch backend", async ({ +test("As a user on a shared light client, when the worker dies silently, I see the appropriate error and can switch network transport", async ({ page, }) => { // Given @@ -269,7 +273,7 @@ test("As a user using smoldot in shared worker, when the worker dies silently, I ); }); -test("As a user using smoldot directly, when the sync times out (>45s) I see the appropriate error and can switch backend", async ({ +test("As a user on a per-tab light client, when the sync times out (>45s) I see the appropriate error and can switch network transport", async ({ page, }) => { // Given @@ -304,7 +308,7 @@ test("As a user using smoldot directly, when the sync times out (>45s) I see the ); }); -test("As a user using smoldot directly, when every peer WebSocket is unavailable, I see a typed Hub failure before the generic request timeout", async ({ +test("As a user on a per-tab light client, when every peer WebSocket is unavailable, I see a typed Hub failure before the generic request timeout", async ({ page, }) => { // Given @@ -354,7 +358,7 @@ test("As a user using smoldot directly, when every peer WebSocket is unavailable expect(blockedSockets).toBeGreaterThan(0); }); -test("As a user using smoldot in shared worker, when the sync times out (>45s) I see the appropriate error and can switch backend", async ({ +test("As a user on a shared light client, when the sync times out (>45s) I see the appropriate error and can switch network transport", async ({ page, }) => { // Given @@ -412,7 +416,7 @@ test("As a user, when the app chunks fail to load mid-session, I see the appropr await expect(page.locator("#error-retry-btn")).toContainText("Reload"); }); -test("As a user using smoldot directly, when smoldot rejects the chain spec, I see the appropriate error and can switch backend", async ({ +test("As a user on a per-tab light client, when the light client rejects the chain spec, I see the appropriate error and can switch network transport", async ({ page, }) => { // Given @@ -443,7 +447,7 @@ test("As a user using smoldot directly, when smoldot rejects the chain spec, I s ); }); -test("As a user using smoldot in shared worker, when smoldot rejects the chain spec, I see the appropriate error and can switch backend", async ({ +test("As a user on a shared light client, when the light client rejects the chain spec, I see the appropriate error and can switch network transport", async ({ page, }) => { // Given @@ -524,7 +528,7 @@ test("As a user, when the domain's contenthash is unsupported or malformed, I se await expect(page.locator("#error-retry-btn")).toHaveCount(0); }); -test("As a user, when the same failure survives a reload, the error page escalates from Settings to a one-click backend switch", async ({ +test("As a user, when the same failure survives a reload, the error page escalates from Settings to a one-click network transport switch", async ({ page, }) => { // Given @@ -561,7 +565,7 @@ test("As a user, when the same failure survives a reload, the error page escalat ); }); -test("As a user, after a resolution failure, clicking retry switches backend and the app loads successfully", async ({ +test("As a user, after a resolution failure, clicking retry switches my network transport and the app loads successfully", async ({ page, }) => { // Given @@ -607,7 +611,7 @@ test("As a user, after a resolution failure, clicking retry switches backend and expect(backendAfter).toBe("rpc-gateway"); }); -test("As a user, after a resolution failure, I can refresh instead of switching backend, and the backend stays unchanged", async ({ +test("As a user, after a resolution failure, I can refresh instead of switching network transport, and my network transport stays unchanged", async ({ page, }) => { // Given @@ -643,11 +647,8 @@ test("As a user, after a resolution failure, I can refresh instead of switching expect(backendAfter).toBe("smoldot-direct"); }); -for (const [label, backend] of [ - ["per-product smoldot", "smoldot-direct"], - ["shared smoldot", "smoldot-shared-worker"], -] as const) { - test(`As a user using ${label}, the host must only spawn one instance of the light client`, async ({ +for (const backend of ["smoldot-direct", "smoldot-shared-worker"] as const) { + test(`As a user on ${TRANSPORT_LABELS[backend]}, the host shell spawns no light client worker of its own`, async ({ page, }) => { // Given diff --git a/apps/host/tests/functional/navigation.spec.ts b/apps/host/tests/functional/navigation.spec.ts index a175ec61b..7a3c669b3 100644 --- a/apps/host/tests/functional/navigation.spec.ts +++ b/apps/host/tests/functional/navigation.spec.ts @@ -35,7 +35,7 @@ async function seedBackend(page: Page): Promise { } test.describe("URL parameters are forwarded into the product", () => { - test("when I open http://