test: drive the Settings screen in a browser and guard every popup element id (closes #229)
All checks were successful
check / check (push) Successful in 27s
e2e / e2e-chrome (push) Successful in 48s
e2e / e2e-firefox (push) Successful in 17s

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 seven cases between the address
removal and dust threshold sections. They assert the About well and the
wallet list were actually written — show() populates those near its end,
only the debug well and the debug-mode checkbox follow, so reading them
back proves show() ran through to there rather than just far enough 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.

What the selectors persist is asserted by a round trip through
NON-DEFAULT values: they are driven to dark and sepolia, the popup is
closed and reopened, both are read back, and both are then restored the
same way and reasserted after a second reopen. Neither value is the
first <option> of its <select>, which is the point — the first option is
what the DOM reports with no JavaScript having run at all, so asserting
it would pass just as happily against a Settings screen that assigned
nothing. One spam filter is likewise toggled off and back on across a
reopen each way. Those round trips run the change handler, saveState(),
loadState() and the assignments show() and init() make, 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.

show() no longer wraps its settings-network lookup in if (networkSelect),
and neither does init(): a null there was silently skipped, which is
exactly the failure this change exists to make loud.

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 four 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. Deleting either persisted
value assignment in settings.js — the theme one in init(), the network
one in show() — reddens the selector round trip and nothing else, each
one on its own.
This commit is contained in:
2026-08-17 06:21:00 +00:00
parent ab1c1846a7
commit 075590ed39
5 changed files with 581 additions and 13 deletions

View File

@@ -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([]);
});
});