From ae4d211c117a057a860fd92b0863b7f71eb1a7d6 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 17 Aug 2026 06:21:00 +0000 Subject: [PATCH] test: drive the Settings screen in a browser and guard every popup element id (closes #229) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- README.md | 18 +++ TODO.md | 21 +++ tests/e2e/run.js | 256 ++++++++++++++++++++++++++++++++++ tests/popupElementIds.test.js | 182 ++++++++++++++++++++++++ 4 files changed, 477 insertions(+) create mode 100644 tests/popupElementIds.test.js diff --git a/README.md b/README.md index f88d33f..20c09a7 100644 --- a/README.md +++ b/README.md @@ -321,6 +321,24 @@ pick it up either. Neither is wired into the Gitea workflow yet — docker-in-docker in CI is a separate question. Run them locally before changing anything under `src/popup/views/`. +### Element id guard (part of `make check`) + +`tests/popupElementIds.test.js` asserts statically that every element id the +popup looks up — `$("...")`, `document.getElementById("...")`, +`showError()`/`hideError()`, and the `view-` a literal `showView("...")` +resolves to — exists in `src/popup/index.html`, and that `index.html` defines no +id twice. A wrong id is valid JavaScript naming a defined function, so neither +jest (node environment, no DOM) nor a linter objects to it; at runtime `$()` +returns `null` and the next property access throws, which inside a view's +`init()` aborts the rest of `src/popup/index.js` `init()` and leaves the popup +blank. + +It runs with no browser, so unlike the e2e suites it fits inside `make check`, +and it covers every view rather than the ones some test happens to open. It only +sees literal arguments: a call like `$(containerId)` is invisible to it, and a +lookup naming the wrong existing element is valid by construction. Both of those +are the browser suites' job. + ## Rationale Common popular EVM wallets have become bloated with swap UIs, portfolio diff --git a/TODO.md b/TODO.md index 73ff168..8579b6a 100644 --- a/TODO.md +++ b/TODO.md @@ -45,6 +45,27 @@ undefined identifiers, which is how # Completed Steps +- 2026-08-17: The Settings screen is driven in a browser, and every element id + the popup looks up is checked statically. Nothing exercised Settings in the + e2e suite, and jest runs with no DOM, so the densest run of `$("...")` lookups + in the codebase was unverified at runtime. Six new cases in `tests/e2e/run.js` + reach Settings, assert the About well and the wallet list were actually + written, assert the four Token Spam Protection checkboxes are real checkboxes + defaulted on, assert the theme and network selectors offer the choices + `src/shared/networks.js` defines and carry the persisted value, and toggle one + filter off and back on across a popup reopen each way — which runs the change + handler, `saveState()`, `loadState()` and the `init()` assignment rather than + just looking at the screen. Each group records a coverage key and a final case + demands the exact set, so a shortened or skipped section reddens the run + instead of shrinking it. `tests/popupElementIds.test.js` is the general half + and runs in `make check` with no browser: every literal id reached through + `$()`, `document.getElementById()`, `showError()`/`hideError()` and + `showView()` must exist in `src/popup/index.html`, which no id in `index.html` + may define twice. Demonstrated on three deliberate breaks — a typo'd id (both + halves red), a handler bound to the wrong but existing element (only the + functional e2e case red), and a typo in a view no browser suite opens (only + the static guard red) + ([#229](https://git.eeqj.de/sneak/AutistMask/issues/229)). - 2026-08-17: `README.md` no longer advertises a defect the wallet does not have. The End-to-End Tests section listed the EIP-1193 code being dropped in the last hop into the page as a standing limit of the dApp coverage; that diff --git a/tests/e2e/run.js b/tests/e2e/run.js index d22de3c..360d2f7 100644 --- a/tests/e2e/run.js +++ b/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 from a
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 ", + ); + 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, diff --git a/tests/popupElementIds.test.js b/tests/popupElementIds.test.js new file mode 100644 index 0000000..c4a188f --- /dev/null +++ b/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([]); + }); +});