test: drive the Settings screen in a browser and guard every popup element id (closes #229)
Some checks failed
check / check (push) Has been cancelled
e2e / e2e-chrome (push) Has been cancelled
e2e / e2e-firefox (push) Has been cancelled

This commit was merged in pull request #299.
This commit is contained in:
2026-08-17 09:14:59 +02:00
parent 47bf38644d
commit a60c4a616a
5 changed files with 581 additions and 13 deletions

View File

@@ -165,6 +165,23 @@ fixtures in `tests/e2e/network.js`, so the run is deterministic and fully
offline; unrecognised outbound requests are reported as failures rather than offline; unrecognised outbound requests are reported as failures rather than
silently allowed. silently allowed.
It also covers the **Settings screen**, which holds the densest run of element
id lookups in the codebase and where one wrong id leaves the whole popup blank
rather than only degrading Settings: that the screen renders populated — the
About well and the wallet list are read back, so a `show()` that stopped early
is caught rather than merely a view that failed to appear — that the four Token
Spam Protection controls are real checkboxes 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 then driven through the UI to
`dark` and `sepolia` and reasserted after a popup reopen before being restored
the same way; neither value is its selector's first `<option>`, so neither can
be read back from the markup with no JavaScript having run. One spam filter is
likewise toggled off and back on across a reopen each way, which exercises the
change handler, `saveState()`, `loadState()` and the assignment `init()` makes.
Each group of these assertions records a coverage key and a final case demands
the exact set, so a section that silently stopped running reddens the suite
instead of quietly shrinking it.
It also covers the confirmation screen, for both a native ETH send and an ERC-20 It also covers the confirmation screen, for both a native ETH send and an ERC-20
send: Send disabled while the fee estimate is in flight, enabled once it lands, send: Send disabled while the fee estimate is in flight, enabled once it lands,
the fee block quoting the expected cost and the reserve separately, the distinct the fee block quoting the expected cost and the reserve separately, the distinct
@@ -400,6 +417,24 @@ the one-time pull of the pinned ~800MB Playwright layer, and well under a minute
once that layer is cached. `e2e-firefox` about 1m05s cold, and it caches its once that layer is cached. `e2e-firefox` about 1m05s cold, and it caches its
Firefox and geckodriver downloads the same way. Firefox and geckodriver downloads the same way.
### 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-<name>` 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 ## Rationale
Common popular EVM wallets have become bloated with swap UIs, portfolio Common popular EVM wallets have become bloated with swap UIs, portfolio

28
TODO.md
View File

@@ -44,6 +44,34 @@ but the review is broader than any of them.
# Completed Steps # 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. Seven 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, and assert the theme and network selectors offer
the choices `src/shared/networks.js` and `index.html` define. The selectors
are then driven to `dark` and `sepolia` — neither is the first `<option>`, so
neither can be read back from the markup with no JavaScript involved — and
reasserted after a popup reopen before being restored the same way, and one
spam filter is toggled off and back on across a reopen each way. Those round
trips run the change handler, `saveState()`, `loadState()` and the
`init()`/`show()` assignments rather than just looking at the screen. `show()`
no longer guards its `settings-network` lookup with `if (networkSelect)`: a
missing element must fail loudly, which is the whole failure mode this unit
exists to catch. 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 four deliberate breaks — a typo'd id (both halves red), a
handler bound to the wrong but existing element (only the functional e2e case
red), a typo in a view no browser suite opens (only the static guard red), and
the deletion of both persisted-value assignments in `settings.js` (only the
selector round-trip case red)
([#229](https://git.eeqj.de/sneak/AutistMask/issues/229)).
- 2026-08-17: One wording for an empty password field on every screen that asks - 2026-08-17: One wording for an empty password field on every screen that asks
for one. The private key export screen said "Password is required." where the for one. The private key export screen said "Password is required." where the
other five say "Please enter your password.", the same one-condition-two- other five say "Please enter your password.", the same one-condition-two-

View File

@@ -167,10 +167,7 @@ function renderWalletListSettings() {
function show() { function show() {
$("settings-rpc").value = state.rpcUrl; $("settings-rpc").value = state.rpcUrl;
$("settings-blockscout").value = state.blockscoutUrl; $("settings-blockscout").value = state.blockscoutUrl;
const networkSelect = $("settings-network"); $("settings-network").value = state.networkId;
if (networkSelect) {
networkSelect.value = state.networkId;
}
renderTrackedTokens(); renderTrackedTokens();
renderSiteLists(); renderSiteLists();
renderWalletListSettings(); renderWalletListSettings();
@@ -282,15 +279,13 @@ function init(ctx) {
}); });
const networkSelect = $("settings-network"); const networkSelect = $("settings-network");
if (networkSelect) { networkSelect.addEventListener("change", async () => {
networkSelect.addEventListener("change", async () => { const newId = networkSelect.value;
const newId = networkSelect.value; const net = await onChainSwitch(newId);
const net = await onChainSwitch(newId); $("settings-rpc").value = state.rpcUrl;
$("settings-rpc").value = state.rpcUrl; $("settings-blockscout").value = state.blockscoutUrl;
$("settings-blockscout").value = state.blockscoutUrl; showFlash("Switched to " + net.name + ".");
showFlash("Switched to " + net.name + "."); });
});
}
$("settings-show-zero-balances").checked = state.showZeroBalanceTokens; $("settings-show-zero-balances").checked = state.showZeroBalanceTokens;
$("settings-show-zero-balances").addEventListener("change", async () => { $("settings-show-zero-balances").addEventListener("change", async () => {

View File

@@ -42,6 +42,7 @@ const {
STUB_TX_HASH, STUB_TX_HASH,
} = require("./network"); } = require("./network");
const { DUST_THRESHOLD_MESSAGE } = require("../../src/popup/dustThreshold"); const { DUST_THRESHOLD_MESSAGE } = require("../../src/popup/dustThreshold");
const { NETWORKS } = require("../../src/shared/networks");
const TEST_TIMEOUT_MS = 120000; const TEST_TIMEOUT_MS = 120000;
@@ -944,6 +945,328 @@ 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",
"selector-round-trip",
"selector-restore",
"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 near its end — only the debug well and
// the debug-mode checkbox follow it — and showView() is the last thing
// of all, so an id show() cannot find aborts before Settings is ever
// displayed. Reading these values back proves show() ran through to
// there, not just far enough to unhide the section. They 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),
);
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,
);
env.settingsCoverage.add("network-select");
});
// Reads both selectors in one page task, so a round trip cannot observe
// them at two different moments.
async function selectorValues(page) {
const theme = await controlState(page, "settings-theme");
const network = await controlState(page, "settings-network");
assert(theme !== null, "no #settings-theme element on Settings");
assert(network !== null, "no #settings-network element on Settings");
return { theme: theme.value, network: network.value };
}
function assertSelectors(got, wantTheme, wantNetwork, where) {
assert(
got.theme === wantTheme,
"the theme selector shows " +
JSON.stringify(got.theme) +
" " +
where +
", expected " +
JSON.stringify(wantTheme),
);
assert(
got.network === wantNetwork,
"the network selector shows " +
JSON.stringify(got.network) +
" " +
where +
", expected " +
JSON.stringify(wantNetwork),
);
}
// The two values the selectors are driven to. NEITHER is the first
// <option> of its <select> (`system` and `mainnet` are), and that is the
// entire point: the first option is what the DOM reports with no
// JavaScript involved at all, so asserting it would pass just as happily
// against a Settings screen that never assigned anything. Only a value
// that went out through the change handler and saveState(), and came back
// through loadState() and the assignment show()/init() makes, can be read
// here.
const NONDEFAULT_THEME = "dark";
const NONDEFAULT_NETWORK = "sepolia";
test("the theme and network selectors carry a non-default persisted value (#229)", async (env) => {
await openSettings(env.page);
// selectOption() fires "change", which is what the handlers bind.
await env.page.selectOption("#settings-theme", NONDEFAULT_THEME);
await env.page.selectOption("#settings-network", NONDEFAULT_NETWORK);
await reopenPopup(env, "#view-settings");
assertSelectors(
await selectorValues(env.page),
NONDEFAULT_THEME,
NONDEFAULT_NETWORK,
"after reopening the popup",
);
env.settingsCoverage.add("selector-round-trip");
// Restore, the same way round, and assert the restore actually took
// rather than trusting it: the later sections inherit this fixture,
// and a selector stuck on `dark`/`sepolia` would otherwise be
// indistinguishable here from one that persists correctly. Switching
// the network back also returns state.rpcUrl and state.blockscoutUrl
// to the mainnet defaults that onChainSwitch() overwrote, which are
// the values src/shared/state.js starts with.
await env.page.selectOption("#settings-theme", "system");
await env.page.selectOption("#settings-network", "mainnet");
await reopenPopup(env, "#view-settings");
assertSelectors(
await selectorValues(env.page),
"system",
"mainnet",
"after restoring and reopening the popup",
);
env.settingsCoverage.add("selector-restore");
});
// 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) // ------------------------------------------------ dust threshold (#233)
// The popup size README documents the UI as designed for. Pages in this // The popup size README documents the UI as designed for. Pages in this
@@ -2672,6 +2995,11 @@ async function main() {
// The recovery phrase of the wallet created in test 2, so later // The recovery phrase of the wallet created in test 2, so later
// tests can assert on the real secret rather than its shape. // tests can assert on the real secret rather than its shape.
phrase: null, 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 // Confirmation-screen heights, measured in the pending state and
// compared against every later state of the same screen. // compared against every later state of the same screen.
ethPendingHeight: null, ethPendingHeight: null,

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