test: drive the Settings screen in a browser and guard every popup element id (closes #229)

Nothing exercised the Settings view in a browser, and jest runs in the
node environment with no DOM, so the densest run of $("...") lookups in
the codebase was unverified at runtime. A wrong id is valid JavaScript
naming a defined function: $() returns null and the next property access
throws, which inside a view's init() aborts the rest of the popup's
init() and leaves every screen blank.

Two halves, because they catch different things.

The e2e suite (tests/e2e/run.js) gains six cases between the address
removal and dust threshold sections. They assert the About well and the
wallet list were actually written — show() populates those last, so
reading them back proves the whole of show() ran rather than just enough
of it to unhide the section — that the four Token Spam Protection
controls are real input[type=checkbox] elements defaulted on, and that
the theme and network selectors offer exactly the choices
src/shared/networks.js and index.html define while carrying the
persisted value. One filter is then toggled off and back on across a
popup reopen each way, which runs the change handler, saveState(),
loadState() and the init() assignment rather than only looking at the
screen. Each group records a coverage key and a final case demands the
exact set, so a section that silently stopped running reddens the suite
instead of shrinking it.

tests/popupElementIds.test.js is the general half and needs no browser,
so jest picks it up and it runs in make check: every literal id reached
through $(), document.getElementById(), showError()/hideError() and
showView() must exist in src/popup/index.html, no id in index.html may
be defined twice, and the scan asserts it found the code and the markup
so it cannot pass by covering nothing. Only literal arguments are
resolvable statically; $(containerId) and a lookup naming the wrong
existing element are the browser suites' job, and README says so.

Demonstrated against three deliberate breaks. A typo'd id in
settings.js reddens both halves, the e2e run reporting
"pageerror: Cannot set properties of null (setting 'checked')" against
its first test. A handler bound to the wrong but existing element passes
the static guard and reddens only the new functional case. A typo in a
view no browser suite opens reddens only the static guard.

tests/e2e/run.js

@@ -42,6 +42,7 @@ const {
     STUB_TX_HASH,
 } = require("./network");
 const { DUST_THRESHOLD_MESSAGE } = require("../../src/popup/dustThreshold");
+const { NETWORKS } = require("../../src/shared/networks");
 
 const TEST_TIMEOUT_MS = 120000;
 
@@ -940,6 +941,256 @@ test("confirming removes the address and returns Home (#162)", async (env) => {
     );
 });
 
+// ------------------------------------------------- Settings screen (#229)
+
+// Settings holds the densest run of $("...") lookups in the codebase, and
+// until this section nothing drove it in a browser. One wrong id makes
+// settings.init() throw, which aborts the rest of index.js init() before it
+// renders anything at all — so a broken id does not degrade Settings, it
+// leaves the whole popup blank. These tests assert the controls are there
+// AND that they work, because "the view is visible" would still pass
+// against a screen whose handlers were never wired.
+
+// The four Token Spam Protection checkboxes, in markup order, with the
+// src/shared/state.js key each one is bound to. All four default true.
+const SPAM_FILTER_CHECKBOXES = [
+    { id: "settings-hide-spoofed-symbols", key: "hideSpoofedSymbols" },
+    { id: "settings-hide-low-holders", key: "hideLowHolderTokens" },
+    { id: "settings-hide-fraud-contracts", key: "hideFraudContracts" },
+    { id: "settings-hide-dust", key: "hideDustTransactions" },
+];
+
+// The one toggled through a reopen. Chosen because nothing later in this
+// suite depends on it: the other three filter token and transaction lists
+// that the ConfirmTx and dApp sections go on to drive.
+const TOGGLED_FILTER = "settings-hide-dust";
+
+// Everything the Settings assertions below must observe, recorded as each
+// group of them completes. The final test demands the exact set.
+//
+// The point is that a green run cannot mean the assertions were skipped.
+// Navigation that silently fails already fails a test — visible() throws
+// on a timeout — but an early return, a deleted test, or a body that
+// stopped being reached would otherwise shrink this section quietly
+// instead of reddening the run.
+const SETTINGS_COVERAGE = [
+    "about-well",
+    "spam-checkbox-defaults",
+    "theme-select",
+    "network-select",
+    "toggle-off-survives-reopen",
+    "toggle-on-survives-reopen",
+    "wallet-list",
+];
+
+// A control read as the DOM has it, not as a selector claims: tag name and
+// type distinguish a real <input type="checkbox"> from a <div> that merely
+// carries the id, and `checked` is the live property rather than the
+// attribute, so it reflects what init() assigned.
+function controlState(page, id) {
+    return page.evaluate((elementId) => {
+        const el = document.getElementById(elementId);
+        if (!el) return null;
+        return {
+            tag: el.tagName.toLowerCase(),
+            type: el.type || "",
+            checked: el.checked,
+            value: el.value,
+            options: Array.from(el.options || []).map((o) => o.value),
+        };
+    }, id);
+}
+
+async function checkboxStates(page) {
+    const out = {};
+    for (const { id } of SPAM_FILTER_CHECKBOXES) {
+        out[id] = await controlState(page, id);
+    }
+    return out;
+}
+
+function assertSpamCheckbox(st, id, expected, where) {
+    assert(st !== null, "no element with id " + id + " on Settings " + where);
+    assert(
+        st.tag === "input" && st.type === "checkbox",
+        id + " is a <" + st.tag + " type=" + st.type + ">, not a checkbox",
+    );
+    assert(
+        st.checked === expected,
+        id +
+            " reads " +
+            st.checked +
+            " " +
+            where +
+            ", expected " +
+            expected +
+            " — the checkbox is on screen but not carrying the persisted value",
+    );
+}
+
+test("Settings renders with the whole screen populated (#229)", async (env) => {
+    await visible(env.page, "#view-main");
+    await openSettings(env.page);
+
+    // show() writes the About well last thing before showView(), so an id
+    // it cannot find aborts before Settings is ever displayed. Reading the
+    // values back proves the whole of show() ran, not just enough of it to
+    // unhide the section. These are filled from build-time constants that
+    // always have a value, so empty means the write did not happen.
+    const about = await env.page.evaluate(() => {
+        const out = {};
+        for (const id of [
+            "about-license",
+            "about-author",
+            "about-version",
+            "about-release-date",
+            "about-commit-link",
+        ]) {
+            const el = document.getElementById(id);
+            out[id] = el === null ? null : el.textContent.trim();
+        }
+        return out;
+    });
+    for (const [id, text] of Object.entries(about)) {
+        assert(
+            text !== null && text.length > 0,
+            "the About well left #" +
+                id +
+                " unwritten: " +
+                JSON.stringify(about),
+        );
+    }
+    env.settingsCoverage.add("about-well");
+
+    // The wallet list is rendered by settings.js rather than authored in
+    // index.html, so an empty container means renderWalletListSettings()
+    // did not run even though the screen came up.
+    const wallets = await env.page
+        .locator("#settings-wallet-list .settings-wallet-name")
+        .count();
+    assert(
+        wallets >= 2,
+        "Settings lists " +
+            wallets +
+            " wallets; the suite created two by this point",
+    );
+    env.settingsCoverage.add("wallet-list");
+});
+
+test("the four Token Spam Protection checkboxes render, defaulted on (#229)", async (env) => {
+    await openSettings(env.page);
+    const states = await checkboxStates(env.page);
+    for (const { id } of SPAM_FILTER_CHECKBOXES) {
+        assertSpamCheckbox(states[id], id, true, "on first render");
+    }
+    env.settingsCoverage.add("spam-checkbox-defaults");
+});
+
+test("the theme and network selectors render their real choices (#229)", async (env) => {
+    await openSettings(env.page);
+
+    const theme = await controlState(env.page, "settings-theme");
+    assert(theme !== null, "no #settings-theme element on Settings");
+    assert(
+        theme.tag === "select",
+        "#settings-theme is a <" + theme.tag + ">, not a <select>",
+    );
+    assert(
+        theme.options.join(",") === "system,light,dark",
+        "the theme selector offers " + JSON.stringify(theme.options),
+    );
+    assert(
+        theme.value === "system",
+        "the theme selector shows " +
+            JSON.stringify(theme.value) +
+            ", expected the persisted default 'system'",
+    );
+    env.settingsCoverage.add("theme-select");
+
+    const network = await controlState(env.page, "settings-network");
+    assert(network !== null, "no #settings-network element on Settings");
+    assert(
+        network.tag === "select",
+        "#settings-network is a <" + network.tag + ">, not a <select>",
+    );
+    const wantNetworks = Object.keys(NETWORKS).sort().join(",");
+    assert(
+        network.options.slice().sort().join(",") === wantNetworks,
+        "the network selector offers " +
+            JSON.stringify(network.options) +
+            ", expected the networks in src/shared/networks.js: " +
+            wantNetworks,
+    );
+    assert(
+        network.value === "mainnet",
+        "the network selector shows " +
+            JSON.stringify(network.value) +
+            ", expected the persisted default 'mainnet'",
+    );
+    env.settingsCoverage.add("network-select");
+});
+
+// The functional half. A checkbox that renders but is not wired looks
+// identical on screen; only a value that survives being written to storage
+// and read back by a fresh page load tells the two apart. That round trip
+// runs through the change handler, saveState(), loadState() and the
+// assignment init() makes — every part of the wiring at once.
+test("a spam filter toggled in Settings survives a popup reopen (#229)", async (env) => {
+    await openSettings(env.page);
+    await env.page.click("#" + TOGGLED_FILTER);
+
+    const immediately = await controlState(env.page, TOGGLED_FILTER);
+    assert(
+        immediately.checked === false,
+        "clicking #" + TOGGLED_FILTER + " did not clear it",
+    );
+
+    await reopenPopup(env, "#view-settings");
+
+    const after = await checkboxStates(env.page);
+    for (const { id } of SPAM_FILTER_CHECKBOXES) {
+        assertSpamCheckbox(
+            after[id],
+            id,
+            id !== TOGGLED_FILTER,
+            "after reopening the popup",
+        );
+    }
+    env.settingsCoverage.add("toggle-off-survives-reopen");
+});
+
+test("turning the same filter back on survives a reopen too (#229)", async (env) => {
+    await openSettings(env.page);
+    await env.page.click("#" + TOGGLED_FILTER);
+
+    await reopenPopup(env, "#view-settings");
+
+    // Restores the fixture the later sections inherit, and rules out a
+    // checkbox that persists "off" only because it is stuck there.
+    const after = await checkboxStates(env.page);
+    for (const { id } of SPAM_FILTER_CHECKBOXES) {
+        assertSpamCheckbox(after[id], id, true, "after toggling back on");
+    }
+    env.settingsCoverage.add("toggle-on-survives-reopen");
+
+    await env.page.click("#btn-settings-back");
+    await visible(env.page, "#view-main");
+});
+
+test("the Settings assertions above all ran (#229)", async (env) => {
+    const seen = [...env.settingsCoverage].sort();
+    const want = SETTINGS_COVERAGE.slice().sort();
+    assert(
+        seen.join(",") === want.join(","),
+        "the Settings section covered " +
+            JSON.stringify(seen) +
+            " but must cover " +
+            JSON.stringify(want) +
+            " — a green run here would otherwise mean only that fewer " +
+            "assertions ran, not that they passed",
+    );
+});
+
 // ------------------------------------------------ dust threshold (#233)
 
 // The popup size README documents the UI as designed for. Pages in this
@@ -2668,6 +2919,11 @@ async function main() {
         // The recovery phrase of the wallet created in test 2, so later
         // tests can assert on the real secret rather than its shape.
         phrase: null,
+        // What the Settings section (#229) actually observed. A guard test
+        // at the end of that section demands the full set, so a skipped or
+        // silently shortened assertion reddens the run instead of shrinking
+        // it.
+        settingsCoverage: new Set(),
         // Confirmation-screen heights, measured in the pending state and
         // compared against every later state of the same screen.
         ethPendingHeight: null,

tests/popupElementIds.test.js

@@ -0,0 +1,182 @@
+// Every element id the popup views look up must exist in the markup they
+// look it up in.
+//
+// The failure this catches: `$("settings-hide-dsut")` is valid JavaScript
+// referring to a defined function, so neither jest (node environment, no
+// DOM) nor a linter has anything to object to. At runtime `$()` returns
+// null and the next property access throws, which in `init()` aborts the
+// rest of that view's wiring and takes the whole screen down. Settings is
+// the densest concentration of these lookups in the codebase.
+//
+// This is the cheap general half of the guard: it runs in `make check`
+// with no browser and covers every id in every view, not the ones some
+// test happens to click. The expensive specific half is the Settings
+// section of the end-to-end suite (tests/e2e/run.js), which proves the
+// screen actually comes up and its controls work.
+//
+// Scope and limits, stated rather than implied:
+//   - Only literal string arguments are resolvable statically. A call
+//     like `$(containerId)` is invisible here; those are covered by the
+//     e2e run instead.
+//   - `document.getElementById()` is checked too, minus the ids listed in
+//     RUNTIME_CREATED_IDS, which name nodes the code creates itself and
+//     which are legitimately absent from the static markup.
+
+"use strict";
+
+const fs = require("fs");
+const path = require("path");
+
+const POPUP_DIR = path.join(__dirname, "..", "src", "popup");
+const POPUP_HTML_PATH = path.join(POPUP_DIR, "index.html");
+
+// Nodes built at runtime rather than authored in index.html. Each one must
+// be created unconditionally by the code before it is ever looked up.
+const RUNTIME_CREATED_IDS = new Set([
+    // Created by updateDebugBanner() in src/popup/views/helpers.js.
+    "debug-banner",
+]);
+
+// Every id lookup the popup performs with a literal argument, as
+// {id, file, line, source} records.
+//
+// showView("x") is included because it resolves to the element id
+// "view-x": a view name with no matching section is the same defect one
+// indirection further out.
+const PATTERNS = [
+    { re: /\$\(\s*"([^"\n]+)"\s*\)/g, id: (m) => m[1], source: "$()" },
+    {
+        re: /document\.getElementById\(\s*"([^"\n]+)"\s*\)/g,
+        id: (m) => m[1],
+        source: "getElementById()",
+    },
+    {
+        re: /\b(?:showError|hideError)\(\s*"([^"\n]+)"/g,
+        id: (m) => m[1],
+        source: "showError()/hideError()",
+    },
+    {
+        re: /\bshowView\(\s*"([^"\n]+)"\s*\)/g,
+        id: (m) => "view-" + m[1],
+        source: "showView()",
+    },
+];
+
+function jsFilesUnder(dir) {
+    const out = [];
+    for (const entry of fs.readdirSync(dir, { withFileTypes: true })) {
+        const full = path.join(dir, entry.name);
+        if (entry.isDirectory()) {
+            out.push(...jsFilesUnder(full));
+        } else if (entry.name.endsWith(".js")) {
+            out.push(full);
+        }
+    }
+    return out.sort();
+}
+
+function lineOf(text, index) {
+    return text.slice(0, index).split("\n").length;
+}
+
+function collectReferences() {
+    const refs = [];
+    for (const file of jsFilesUnder(POPUP_DIR)) {
+        const text = fs.readFileSync(file, "utf8");
+        const rel = path.relative(path.join(__dirname, ".."), file);
+        for (const { re, id, source } of PATTERNS) {
+            re.lastIndex = 0;
+            let m;
+            while ((m = re.exec(text)) !== null) {
+                refs.push({
+                    id: id(m),
+                    file: rel,
+                    line: lineOf(text, m.index),
+                    source,
+                });
+            }
+        }
+    }
+    return refs;
+}
+
+function collectHtmlIds(html) {
+    const ids = [];
+    const re = /\bid="([^"]+)"/g;
+    let m;
+    while ((m = re.exec(html)) !== null) ids.push(m[1]);
+    return ids;
+}
+
+const HTML = fs.readFileSync(POPUP_HTML_PATH, "utf8");
+const HTML_IDS = collectHtmlIds(HTML);
+const HTML_ID_SET = new Set(HTML_IDS);
+const REFERENCES = collectReferences();
+
+describe("every element id the popup looks up exists in its markup", () => {
+    // A guard that found nothing to check would pass forever. If a
+    // refactor renames the directory, changes the helper, or moves the
+    // markup, this fails instead of quietly covering zero call sites.
+    // The floors are far below the counts measured when this was written
+    // (434 lookups across 20 of the 24 files under src/popup/, against 274
+    // ids in the markup), so ordinary churn does not trip them.
+    test("the scan actually found the code and the markup", () => {
+        const files = new Set(REFERENCES.map((r) => r.file));
+        expect(files.size).toBeGreaterThanOrEqual(15);
+        expect(REFERENCES.length).toBeGreaterThanOrEqual(300);
+        expect(HTML_IDS.length).toBeGreaterThanOrEqual(200);
+
+        // The densest screen, named explicitly: a scan that stopped
+        // covering src/popup/views/settings.js is the exact regression
+        // this file was written for.
+        expect(
+            files.has(path.join("src", "popup", "views", "settings.js")),
+        ).toBe(true);
+        expect(
+            REFERENCES.some((r) => r.id === "settings-hide-spoofed-symbols"),
+        ).toBe(true);
+        expect(REFERENCES.some((r) => r.id === "view-settings")).toBe(true);
+    });
+
+    test("no lookup names an id that src/popup/index.html does not define", () => {
+        const missing = REFERENCES.filter(
+            (r) => !HTML_ID_SET.has(r.id) && !RUNTIME_CREATED_IDS.has(r.id),
+        ).map(
+            (r) =>
+                r.file +
+                ":" +
+                r.line +
+                " " +
+                r.source +
+                ' looks up id "' +
+                r.id +
+                '", which is not in src/popup/index.html',
+        );
+
+        expect(missing).toEqual([]);
+    });
+
+    test("every id excused as runtime-created is still looked up somewhere", () => {
+        // Otherwise the exception list becomes a place stale names
+        // accumulate, and the next real miss can be waved through by
+        // adding one more.
+        for (const id of RUNTIME_CREATED_IDS) {
+            expect(REFERENCES.some((r) => r.id === id)).toBe(true);
+            expect(HTML_ID_SET.has(id)).toBe(false);
+        }
+    });
+
+    test("index.html defines no id twice", () => {
+        // getElementById returns the first match, so a duplicate id means
+        // one of the two elements can never be reached by the code that
+        // thinks it owns it.
+        const seen = new Set();
+        const duplicated = [];
+        for (const id of HTML_IDS) {
+            if (seen.has(id)) duplicated.push(id);
+            seen.add(id);
+        }
+
+        expect(duplicated).toEqual([]);
+    });
+});