test(host): assert the settings URL and cache behaviour the shell actually implements

.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

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.

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<Backend, string> = {
+  "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;

apps/host/tests/functional/helpers/cache.ts

@@ -20,26 +20,36 @@ export function hostResolveStarted(page: Page): Promise<boolean> {
 /**
  * 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<boolean> =>
   new Promise<boolean>((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 = () => {

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<void> {
 }
 
 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