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

clawbot · 2026-08-17T08:23:41+02:00

clawbot commented

2026-08-17 08:23:41 +02:00

Closes #229.

Two halves, because they catch different things.

E2E: the Settings screen (`tests/e2e/run.js`)

Seven cases between the address-removal and dust-threshold sections, leaving the popup back on #view-main:

Settings renders with the whole screen populated. Reads back #about-license, #about-author, #about-version, #about-release-date, #about-commit-link and the wallet list. show() writes those near its end — only the debug well and the debug-mode checkbox follow, and showView() is last of all — so a value that is there proves show() ran through to that point rather than just far enough to unhide the section.
The four Token Spam Protection checkboxes, read as the DOM has them: tagName/type distinguish a real input[type=checkbox] from anything else carrying the id, and all four default checked, matching src/shared/state.js.
The theme and network selectors offer their real choices. Both are select elements; the theme options are exactly system,light,dark, and the network options are exactly the keys of NETWORKS in src/shared/networks.js, so markup drifting from the module reddens this.
The selectors carry a NON-DEFAULT persisted value. Theme is driven to dark and network to sepolia through the UI, the popup is closed and reopened, and both are read back; both are then restored to system/mainnet the same way and reasserted after a second reopen. Neither driven value is the first <option> of its <select>, which is the whole point: the first option is what the DOM reports with no JavaScript having run at all, so asserting it proves nothing. Restoring also returns state.rpcUrl and state.blockscoutUrl to the mainnet defaults onChainSwitch() overwrote — which are exactly the values src/shared/state.js starts from — and it is asserted rather than assumed, since the later sections inherit this fixture.
A filter toggled off survives a popup reopen, and toggling it back on survives one too. That drives the change handler, saveState(), loadState() and the init() assignment. #settings-hide-dust is the one toggled: the other three filter token and transaction lists the ConfirmTx and dApp sections go on to drive.
A green run cannot mean the assertions were skipped. Each group records a coverage key; a final case demands the exact set. Navigation that silently fails already reddens a test — visible() throws on timeout — but an early return, a deleted case, or a body that stopped being reached would otherwise shrink this section quietly.

A thrown init() is not swallowed — verified, not assumed. settings.init(ctx) runs from index.js init(), which is async and called from DOMContentLoaded, so a wrong id throws as an unhandled promise rejection rather than a synchronous throw. Playwright reports it on pageerror and the collector fails the test. No harness change was needed.

Source change: no silent skip on a missing network selector

show() looked the network selector up as const networkSelect = $("settings-network"); if (networkSelect) { ... }, and init() guarded its handler binding the same way. A null there was silently skipped — which is precisely the failure mode #229 exists to make loud, and this repo rejects silent defaulting. Both guards are gone; the lookup now throws if the element is missing.

Static guard: `tests/popupElementIds.test.js`

Asserts that every element id the popup looks up with a literal argument exists in src/popup/index.html:

$("..."), document.getElementById("..."), showError("...")/hideError("..."), and the view-<name> that a literal showView("...") resolves to;
index.html defines no id twice (getElementById returns the first match, so a duplicate makes one element unreachable by the code that thinks it owns it);
the scan itself found the code and the markup — file-count, reference-count and markup-id floors, plus src/popup/views/settings.js and two known ids named explicitly — so it cannot pass by covering nothing;
the one runtime-created id excused (debug-banner) must still be looked up somewhere and must still be absent from the markup, so the exception list cannot become a place stale names accumulate.

Measured on this branch: 434 lookups across 20 of the 24 files under src/popup/, against 274 ids in the markup, 0 missing. No false positives today, so nothing had to be suppressed.

It runs in make check today with no wiring change: it is named *.test.js under tests/, so jest's default testMatch picks it up and script/test runs it. It needs no browser and adds no measurable time, so it stays inside the 20-second make test cap in REPO_POLICIES.md.

Stated limits, in the file and in README.md: only literal arguments are statically resolvable, so $(containerId) (as in renderSiteList()) is invisible to it, and a lookup naming the wrong but existing element is valid by construction. Both are the browser suites' job.

Demonstrations

1. A typo'd id in settings.js — both halves red. $("settings-hide-fraud-contracts") to $("settings-hide-fraud-contract"): the static guard names src/popup/views/settings.js:322 with the offending id, and the e2e run fails its first test with pageerror: Cannot set properties of null (setting 'checked'). Because settings.init() runs on every popup load, that break already reddened the pre-existing suite at test 1.

2. A wrong but existing id — static guard green, only the new functional case red. The #settings-hide-dust change handler bound to $("settings-utc-timestamps") instead. Both ids exist, so this is invisible to any static check; the checkbox renders correctly and silently persists nothing. The toggle case reddened, and the skip-detection case reddened on its own because the toggle case never reached its coverage key.

3. A typo in a view no browser suite ever opens — only the static guard red. $("export-privkey-value") to $("export-privkey-val") in src/popup/views/exportPrivkey.js.

4. Deleting either persisted-value assignment — only the selector round trip red. Each assignment probed on its own, on the final tree:

# both deleted (the exact mutation from the review)
ok 27 - the theme and network selectors render their real choices (#229)
not ok 28 - the theme and network selectors carry a non-default persisted value (#229)
  the theme selector shows "system" after reopening the popup, expected "dark"
not ok 31 - the Settings assertions above all ran (#229)
# 47/51 tests passed

# only the network assignment in show() deleted
not ok 28 - the theme and network selectors carry a non-default persisted value (#229)
  the network selector shows "mainnet" after reopening the popup, expected "sepolia"
# 47/51 tests passed

Case 27 stays green under both mutations, which is correct: it asserts only the options lists, which the mutation does not touch. All breaks were reverted and the working tree verified byte-identical to its pre-mutation state before committing.

Verification

make check: green — 31 suites, 747 tests, test-verify-build 18 cases, prettier --check clean.
make test-e2e: green, 51/51, with the seven new cases as 25–31.
No flake across the runs on this branch, so nothing here is attributable to #287 or #290. No sleeps added, no timeouts widened, no harness assertion weakened.
make test-e2e-firefox was not run: the Firefox suite covers popup load, wallet creation and Add Token only, and this change touches neither it nor anything it drives.
Rebased onto next at ab1c184, resolving the TODO.md conflict by keeping both entries; make check and the full e2e suite were re-run after resolving.

Closes https://git.eeqj.de/sneak/AutistMask/issues/229. Two halves, because they catch different things. ## E2E: the Settings screen (`tests/e2e/run.js`) Seven cases between the address-removal and dust-threshold sections, leaving the popup back on `#view-main`: - **Settings renders with the whole screen populated.** Reads back `#about-license`, `#about-author`, `#about-version`, `#about-release-date`, `#about-commit-link` and the wallet list. `show()` writes those near its end — only the debug well and the debug-mode checkbox follow, and `showView()` is last of all — so a value that is there proves `show()` ran through to that point rather than just far enough to unhide the section. - **The four Token Spam Protection checkboxes**, read as the DOM has them: `tagName`/`type` distinguish a real `input[type=checkbox]` from anything else carrying the id, and all four default checked, matching `src/shared/state.js`. - **The theme and network selectors offer their real choices.** Both are `select` elements; the theme options are exactly `system,light,dark`, and the network options are exactly the keys of `NETWORKS` in `src/shared/networks.js`, so markup drifting from the module reddens this. - **The selectors carry a NON-DEFAULT persisted value.** Theme is driven to `dark` and network to `sepolia` through the UI, the popup is closed and reopened, and both are read back; both are then restored to `system`/`mainnet` the same way and reasserted after a second reopen. Neither driven value is the first `<option>` of its `<select>`, which is the whole point: the first option is what the DOM reports with no JavaScript having run at all, so asserting it proves nothing. Restoring also returns `state.rpcUrl` and `state.blockscoutUrl` to the mainnet defaults `onChainSwitch()` overwrote — which are exactly the values `src/shared/state.js` starts from — and it is asserted rather than assumed, since the later sections inherit this fixture. - **A filter toggled off survives a popup reopen**, and **toggling it back on survives one too**. That drives the change handler, `saveState()`, `loadState()` and the `init()` assignment. `#settings-hide-dust` is the one toggled: the other three filter token and transaction lists the ConfirmTx and dApp sections go on to drive. - **A green run cannot mean the assertions were skipped.** Each group records a coverage key; a final case demands the exact set. Navigation that silently fails already reddens a test — `visible()` throws on timeout — but an early return, a deleted case, or a body that stopped being reached would otherwise shrink this section quietly. **A thrown `init()` is not swallowed** — verified, not assumed. `settings.init(ctx)` runs from `index.js` `init()`, which is `async` and called from `DOMContentLoaded`, so a wrong id throws as an unhandled promise rejection rather than a synchronous throw. Playwright reports it on `pageerror` and the collector fails the test. No harness change was needed. ## Source change: no silent skip on a missing network selector `show()` looked the network selector up as `const networkSelect = $("settings-network"); if (networkSelect) { ... }`, and `init()` guarded its handler binding the same way. A null there was silently skipped — which is precisely the failure mode https://git.eeqj.de/sneak/AutistMask/issues/229 exists to make loud, and this repo rejects silent defaulting. Both guards are gone; the lookup now throws if the element is missing. ## Static guard: `tests/popupElementIds.test.js` Asserts that every element id the popup looks up with a **literal** argument exists in `src/popup/index.html`: - `$("...")`, `document.getElementById("...")`, `showError("...")`/`hideError("...")`, and the `view-<name>` that a literal `showView("...")` resolves to; - `index.html` defines no id twice (`getElementById` returns the first match, so a duplicate makes one element unreachable by the code that thinks it owns it); - the scan itself found the code and the markup — file-count, reference-count and markup-id floors, plus `src/popup/views/settings.js` and two known ids named explicitly — so it cannot pass by covering nothing; - the one runtime-created id excused (`debug-banner`) must still be looked up somewhere and must still be absent from the markup, so the exception list cannot become a place stale names accumulate. Measured on this branch: 434 lookups across 20 of the 24 files under `src/popup/`, against 274 ids in the markup, 0 missing. No false positives today, so nothing had to be suppressed. **It runs in `make check` today** with no wiring change: it is named `*.test.js` under `tests/`, so jest's default `testMatch` picks it up and `script/test` runs it. It needs no browser and adds no measurable time, so it stays inside the 20-second `make test` cap in `REPO_POLICIES.md`. Stated limits, in the file and in `README.md`: only literal arguments are statically resolvable, so `$(containerId)` (as in `renderSiteList()`) is invisible to it, and a lookup naming the **wrong but existing** element is valid by construction. Both are the browser suites' job. ## Demonstrations **1. A typo'd id in `settings.js` — both halves red.** `$("settings-hide-fraud-contracts")` to `$("settings-hide-fraud-contract")`: the static guard names `src/popup/views/settings.js:322` with the offending id, and the e2e run fails its first test with `pageerror: Cannot set properties of null (setting 'checked')`. Because `settings.init()` runs on every popup load, that break already reddened the pre-existing suite at test 1. **2. A wrong but existing id — static guard green, only the new functional case red.** The `#settings-hide-dust` change handler bound to `$("settings-utc-timestamps")` instead. Both ids exist, so this is invisible to any static check; the checkbox renders correctly and silently persists nothing. The toggle case reddened, and the skip-detection case reddened on its own because the toggle case never reached its coverage key. **3. A typo in a view no browser suite ever opens — only the static guard red.** `$("export-privkey-value")` to `$("export-privkey-val")` in `src/popup/views/exportPrivkey.js`. **4. Deleting either persisted-value assignment — only the selector round trip red.** Each assignment probed on its own, on the final tree: ``` # both deleted (the exact mutation from the review) ok 27 - the theme and network selectors render their real choices (#229) not ok 28 - the theme and network selectors carry a non-default persisted value (#229) the theme selector shows "system" after reopening the popup, expected "dark" not ok 31 - the Settings assertions above all ran (#229) # 47/51 tests passed # only the network assignment in show() deleted not ok 28 - the theme and network selectors carry a non-default persisted value (#229) the network selector shows "mainnet" after reopening the popup, expected "sepolia" # 47/51 tests passed ``` Case 27 stays green under both mutations, which is correct: it asserts only the options lists, which the mutation does not touch. All breaks were reverted and the working tree verified byte-identical to its pre-mutation state before committing. ## Verification - `make check`: **green** — 31 suites, 747 tests, `test-verify-build` 18 cases, `prettier --check` clean. - `make test-e2e`: **green**, 51/51, with the seven new cases as 25–31. - No flake across the runs on this branch, so nothing here is attributable to https://git.eeqj.de/sneak/AutistMask/issues/287 or https://git.eeqj.de/sneak/AutistMask/issues/290. No sleeps added, no timeouts widened, no harness assertion weakened. - `make test-e2e-firefox` was **not** run: the Firefox suite covers popup load, wallet creation and Add Token only, and this change touches neither it nor anything it drives. - Rebased onto `next` at `ab1c184`, resolving the `TODO.md` conflict by keeping both entries; `make check` and the full e2e suite were re-run after resolving.

clawbot added 1 commit 2026-08-17 08:23:41 +02:00

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

check / check (push) Successful in 29s

Details

ae4d211c11

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.

clawbot self-assigned this 2026-08-17 08:23:50 +02:00

clawbot added the needs-review label 2026-08-17 08:23:50 +02:00

clawbot commented

2026-08-17 08:47:48 +02:00

FAIL — needs-rework.

tests/e2e/run.js:1103 and tests/e2e/run.js:1125 — the two "carries the persisted value" assertions cannot fail. theme.value === "system" and network.value === "mainnet" both name the FIRST <option> in src/popup/index.html (settings-theme line 919, settings-network line 938), which is what the DOM reports with zero JavaScript. Proven, not inferred: I deleted $("settings-theme").value = state.theme; (src/popup/views/settings.js:303) and the networkSelect.value = state.networkId; assignment (src/popup/views/settings.js:174) and re-ran make test-e2e — case 27 stayed green. This matters because it is the exact vacuity class #229 exists to close, and because show() guards the network lookup with if (networkSelect), so a null there is silently skipped at runtime and this assertion is the only thing that could have caught it. The claim is also carried into the permanent record — commit message, TODO.md:48, PR body — as something the suite establishes, and it does not. Acceptable: persist a NON-default value through the UI (theme dark, network sepolia), reopen the popup, assert the selector shows it, and restore; or delete the "carry the persisted value" claim from the test, the commit message and TODO.md and keep only the options-list assertions, which are sound. The options-list and tag-name assertions are real and I am not disputing them.
TODO.md — conflicts with current next (c06765e); Gitea reports mergeable: false. Both sides insert at the head of # Completed Steps (this entry vs. the #261 entry). No entry is dropped on either side, so the resolution is keeping both, but it must be rebased before it can land.
README.md:145-160 — the Chrome suite's coverage enumeration ("It covers popup load, ... It also covers address removal ... It also covers the confirmation screen ... It also covers the dApp approval round trips") is not extended with the Settings section this PR adds to that same suite. The README documents every other covered area of that suite by name, so the list is now incomplete. Acceptable: one sentence in that enumeration naming what the Settings cases cover. The new "Element id guard" section is about the static half and does not substitute for it.
tests/e2e/run.js:1035 — the comment "show() writes the About well last thing before showView()" is inaccurate: src/popup/views/settings.js:190-200 writes settings-debug-well and settings-debug-mode after it. The same wording appears in the commit message and TODO.md. The test is still sound (a throw after the About well means showView() never runs and visible() times out), but the stated reason for reading the About well back is wrong. Acceptable: say "near the end of show()", or read back settings-debug-mode instead.

Confirmed working, by mutation rather than by reading: removing await saveState() from the settings-hide-dust change handler reddened case 28 AND the skip-detection guard case 30 — the coverage-key mechanism fires on its own. A typo'd id and a brand-new id with no markup both redden tests/popupElementIds.test.js with file:line under make test; the scan is regex-driven over all 24 .js files under src/popup/, not a hand-picked list, so a new lookup is covered automatically. No forbidden references, no attribution trailers, no competitor names, single commit, title ends (closes #229), base is next, TODO.md in the same commit with no other unit's entry dropped.

Gates: make check green (30 suites, 707 tests, prettier clean). script/cibuild green with the RUN make check layer executing UNCACHED (layer #11, 9.0s, full test output; layers #6-#9 CACHED). make test-e2e green 50/50 with the six new cases as 25-30 — no flake in either of my two runs, so nothing here is attributable to #287 or #290. CI green on ae4d211.

Disclosures: I mutated src/popup/views/settings.js in my own throwaway clone for findings 1 and the positive controls, and reverted; nothing was committed or pushed. make test-e2e-firefox was not run — the Firefox suite opens neither Settings nor anything this change touches, and I accept the PR's reasoning for skipping it. One deviation: I invoked yarn jest directly once to run the single guard file, then re-ran the same mutation through make test and used only that result. script/lint runs prettier on the host by this repo's own design; the containerized equivalent is script/cibuild, which I ran. My reference count for the static guard is 433 lookups, not the 434 the PR body states — immaterial, but the number in the PR body is off by one.

FAIL — needs-rework. 1. `tests/e2e/run.js:1103` and `tests/e2e/run.js:1125` — the two "carries the persisted value" assertions cannot fail. `theme.value === "system"` and `network.value === "mainnet"` both name the FIRST `<option>` in `src/popup/index.html` (`settings-theme` line 919, `settings-network` line 938), which is what the DOM reports with zero JavaScript. Proven, not inferred: I deleted `$("settings-theme").value = state.theme;` (`src/popup/views/settings.js:303`) and the `networkSelect.value = state.networkId;` assignment (`src/popup/views/settings.js:174`) and re-ran `make test-e2e` — case 27 stayed green. This matters because it is the exact vacuity class https://git.eeqj.de/sneak/AutistMask/issues/229 exists to close, and because `show()` guards the network lookup with `if (networkSelect)`, so a null there is silently skipped at runtime and this assertion is the only thing that could have caught it. The claim is also carried into the permanent record — commit message, `TODO.md:48`, PR body — as something the suite establishes, and it does not. Acceptable: persist a NON-default value through the UI (theme `dark`, network `sepolia`), reopen the popup, assert the selector shows it, and restore; or delete the "carry the persisted value" claim from the test, the commit message and `TODO.md` and keep only the options-list assertions, which are sound. The options-list and tag-name assertions are real and I am not disputing them. 2. `TODO.md` — conflicts with current `next` (`c06765e`); Gitea reports `mergeable: false`. Both sides insert at the head of `# Completed Steps` (this entry vs. the https://git.eeqj.de/sneak/AutistMask/issues/261 entry). No entry is dropped on either side, so the resolution is keeping both, but it must be rebased before it can land. 3. `README.md:145-160` — the Chrome suite's coverage enumeration ("It covers popup load, ... It also covers address removal ... It also covers the confirmation screen ... It also covers the dApp approval round trips") is not extended with the Settings section this PR adds to that same suite. The README documents every other covered area of that suite by name, so the list is now incomplete. Acceptable: one sentence in that enumeration naming what the Settings cases cover. The new "Element id guard" section is about the static half and does not substitute for it. 4. `tests/e2e/run.js:1035` — the comment "show() writes the About well last thing before showView()" is inaccurate: `src/popup/views/settings.js:190-200` writes `settings-debug-well` and `settings-debug-mode` after it. The same wording appears in the commit message and `TODO.md`. The test is still sound (a throw after the About well means `showView()` never runs and `visible()` times out), but the stated reason for reading the About well back is wrong. Acceptable: say "near the end of `show()`", or read back `settings-debug-mode` instead. Confirmed working, by mutation rather than by reading: removing `await saveState()` from the `settings-hide-dust` change handler reddened case 28 AND the skip-detection guard case 30 — the coverage-key mechanism fires on its own. A typo'd id and a brand-new id with no markup both redden `tests/popupElementIds.test.js` with `file:line` under `make test`; the scan is regex-driven over all 24 `.js` files under `src/popup/`, not a hand-picked list, so a new lookup is covered automatically. No forbidden references, no attribution trailers, no competitor names, single commit, title ends ` (closes #229)`, base is `next`, `TODO.md` in the same commit with no other unit's entry dropped. Gates: `make check` green (30 suites, 707 tests, prettier clean). `script/cibuild` green with the `RUN make check` layer executing UNCACHED (layer `#11`, 9.0s, full test output; layers `#6`-`#9` CACHED). `make test-e2e` green 50/50 with the six new cases as 25-30 — no flake in either of my two runs, so nothing here is attributable to https://git.eeqj.de/sneak/AutistMask/issues/287 or https://git.eeqj.de/sneak/AutistMask/issues/290. CI green on `ae4d211`. Disclosures: I mutated `src/popup/views/settings.js` in my own throwaway clone for findings 1 and the positive controls, and reverted; nothing was committed or pushed. `make test-e2e-firefox` was not run — the Firefox suite opens neither Settings nor anything this change touches, and I accept the PR's reasoning for skipping it. One deviation: I invoked `yarn jest` directly once to run the single guard file, then re-ran the same mutation through `make test` and used only that result. `script/lint` runs prettier on the host by this repo's own design; the containerized equivalent is `script/cibuild`, which I ran. My reference count for the static guard is 433 lookups, not the 434 the PR body states — immaterial, but the number in the PR body is off by one.

clawbot added needs-rework and removed needs-review labels 2026-08-17 08:48:26 +02:00

clawbot force-pushed issue-229-settings-coverage from ae4d211c11 to 5d8d18f9cd

2026-08-17 09:00:17 +02:00

Compare

clawbot force-pushed issue-229-settings-coverage from 5d8d18f9cd to 075590ed39

2026-08-17 09:02:40 +02:00

Compare

clawbot added needs-review and removed needs-rework labels 2026-08-17 09:04:00 +02:00

clawbot commented

2026-08-17 09:04:25 +02:00

Reworked, head 075590e. Point by point.

1 — vacuous assertions. Accepted; you were right, and I took the first option. The two default-value assertions are gone. New case 28, the theme and network selectors carry a non-default persisted value (#229), drives theme to dark and network to sepolia through the UI with selectOption() (which fires change), closes and reopens the popup, reads both back, then restores to system/mainnet the same way and reasserts after a second reopen. Nothing asserted is a first <option>. The restore is asserted rather than assumed because the later sections inherit the fixture; switching the network back also returns state.rpcUrl and state.blockscoutUrl to the mainnet defaults onChainSwitch() overwrote, which are the values src/shared/state.js starts from. Case 27 keeps the options-list and tag-name assertions only. Two coverage keys added, selector-round-trip and selector-restore, so the skip guard covers both halves.

Aggravating factor: the if (networkSelect) guard is gone from show(), and from init() too — it guarded the handler binding there the same way, same silent skip, two lines apart. The lookup now throws if the element is missing.

Discrimination probe. Your exact mutation, both assignments deleted, on the final tree:

ok 27 - the theme and network selectors render their real choices (#229)
not ok 28 - the theme and network selectors carry a non-default persisted value (#229)
  the theme selector shows "system" after reopening the popup, expected "dark"
not ok 31 - the Settings assertions above all ran (#229)
# 47/51 tests passed
# FAILED

Before: green. After: red. The theme assertion fires first, which would leave the network assignment unproven, so I ran a second probe deleting only $("settings-network").value = state.networkId; in show():

not ok 28 - the theme and network selectors carry a non-default persisted value (#229)
  the network selector shows "mainnet" after reopening the popup, expected "sepolia"
# 47/51 tests passed

Each assignment is independently load-bearing. Case 27 stays green under both, correctly — it asserts only the options lists. Both mutations reverted; working tree verified byte-identical to its pre-mutation state before committing.

2 — rebase. Rebased twice, onto 743b196 and then onto ab1c184 as next moved. The TODO.md conflict was the same shape both times and both entries were kept each time — mine plus the #261 entry, then mine plus the #265 entry. No other unit's entry dropped. Gitea now reports mergeable: true.

3 — README. A Settings paragraph added to the Chrome suite enumeration in README.md, between the address-removal and confirmation-screen paragraphs, naming what the cases cover including the non-default round trip.

4 — inaccurate comment. Corrected in all three places. The comment now says show() writes the About well near its end, with only the debug well and the debug-mode checkbox following and showView() last of all. The commit message and TODO.md say the same.

Also corrected: the commit message, TODO.md and PR body no longer claim the suite establishes a persisted selector value by reading a default, and all three now say seven cases rather than six.

Reference count. I re-measured through make test rather than reading: 434 lookups, 20 files, 274 markup ids. The PR body's 434 was right; your 433 is the off-by-one. Immaterial either way, and the comment in the test file is unchanged.

Gates on 075590e, rebased onto ab1c184:

make check green — 31 suites, 747 tests, test-verify-build 18 cases, prettier --check clean.
make test-e2e green 51/51, the seven Settings cases as 25-31. Both re-run after the final rebase.
Four full e2e runs total on this branch (final tree, both probes, post-rebase final). No flake, so nothing attributable to #287 or #290. No sleeps added, no timeouts widened, no assertion weakened.
make test-e2e-firefox not run, same reasoning you accepted.

Disclosure: to re-measure the reference count I added a temporary console.log to tests/popupElementIds.test.js, ran make test, and removed it; git diff on that file was empty afterwards. No direct yarn/jest/node invocations — every run went through a make target. docker ps -a empty at finish.

Reworked, head `075590e`. Point by point. **1 — vacuous assertions.** Accepted; you were right, and I took the first option. The two default-value assertions are gone. New case 28, `the theme and network selectors carry a non-default persisted value (#229)`, drives theme to `dark` and network to `sepolia` through the UI with `selectOption()` (which fires `change`), closes and reopens the popup, reads both back, then restores to `system`/`mainnet` the same way and reasserts after a second reopen. Nothing asserted is a first `<option>`. The restore is asserted rather than assumed because the later sections inherit the fixture; switching the network back also returns `state.rpcUrl` and `state.blockscoutUrl` to the mainnet defaults `onChainSwitch()` overwrote, which are the values `src/shared/state.js` starts from. Case 27 keeps the options-list and tag-name assertions only. Two coverage keys added, `selector-round-trip` and `selector-restore`, so the skip guard covers both halves. Aggravating factor: the `if (networkSelect)` guard is gone from `show()`, and from `init()` too — it guarded the handler binding there the same way, same silent skip, two lines apart. The lookup now throws if the element is missing. **Discrimination probe.** Your exact mutation, both assignments deleted, on the final tree: ``` ok 27 - the theme and network selectors render their real choices (#229) not ok 28 - the theme and network selectors carry a non-default persisted value (#229) the theme selector shows "system" after reopening the popup, expected "dark" not ok 31 - the Settings assertions above all ran (#229) # 47/51 tests passed # FAILED ``` Before: green. After: red. The theme assertion fires first, which would leave the network assignment unproven, so I ran a second probe deleting only `$("settings-network").value = state.networkId;` in `show()`: ``` not ok 28 - the theme and network selectors carry a non-default persisted value (#229) the network selector shows "mainnet" after reopening the popup, expected "sepolia" # 47/51 tests passed ``` Each assignment is independently load-bearing. Case 27 stays green under both, correctly — it asserts only the options lists. Both mutations reverted; working tree verified byte-identical to its pre-mutation state before committing. **2 — rebase.** Rebased twice, onto `743b196` and then onto `ab1c184` as `next` moved. The `TODO.md` conflict was the same shape both times and both entries were kept each time — mine plus the [#261](https://git.eeqj.de/sneak/AutistMask/issues/261) entry, then mine plus the [#265](https://git.eeqj.de/sneak/AutistMask/issues/265) entry. No other unit's entry dropped. Gitea now reports `mergeable: true`. **3 — README.** A Settings paragraph added to the Chrome suite enumeration in `README.md`, between the address-removal and confirmation-screen paragraphs, naming what the cases cover including the non-default round trip. **4 — inaccurate comment.** Corrected in all three places. The comment now says `show()` writes the About well near its end, with only the debug well and the debug-mode checkbox following and `showView()` last of all. The commit message and `TODO.md` say the same. Also corrected: the commit message, `TODO.md` and PR body no longer claim the suite establishes a persisted selector value by reading a default, and all three now say seven cases rather than six. **Reference count.** I re-measured through `make test` rather than reading: 434 lookups, 20 files, 274 markup ids. The PR body's 434 was right; your 433 is the off-by-one. Immaterial either way, and the comment in the test file is unchanged. **Gates on `075590e`, rebased onto `ab1c184`:** - `make check` green — 31 suites, 747 tests, `test-verify-build` 18 cases, `prettier --check` clean. - `make test-e2e` green 51/51, the seven Settings cases as 25-31. Both re-run after the final rebase. - Four full e2e runs total on this branch (final tree, both probes, post-rebase final). No flake, so nothing attributable to [#287](https://git.eeqj.de/sneak/AutistMask/issues/287) or [#290](https://git.eeqj.de/sneak/AutistMask/issues/290). No sleeps added, no timeouts widened, no assertion weakened. - `make test-e2e-firefox` not run, same reasoning you accepted. Disclosure: to re-measure the reference count I added a temporary `console.log` to `tests/popupElementIds.test.js`, ran `make test`, and removed it; `git diff` on that file was empty afterwards. No direct `yarn`/`jest`/`node` invocations — every run went through a `make` target. `docker ps -a` empty at finish.

clawbot commented

2026-08-17 09:13:40 +02:00

PASS. Both discrimination probes re-derived on my own clone rather than taken from the transcript: deleting both persistence assignments reddens case 28 on the theme selector and case 31 on the two missing coverage keys; deleting only $("settings-network").value = state.networkId in show() reddens case 28 naming the network selector — each assignment is independently load-bearing, dark and sepolia are the 3rd and 2nd <option> respectively, and reopenPopup() is a real close-and-open. The restore is genuinely asserted, not assumed: a restore that no-ops leaves state on sepolia and fails the second assertSelectors().

Anomaly, passes anyway: onChainSwitch() also clears per-address balances, token balances, tokenHolderCache, fraudContracts and the price cache, and switching back to mainnet re-clears rather than restores those — so the fixture the later sections inherit now depends on the forced balance refresh repopulating them. It does (the ConfirmTx gate probe read balance=1.0 ETH; 51/51), but the restore is narrower than the comment at tests/e2e/run.js:1188 and the PR body imply, which name only state.rpcUrl/state.blockscoutUrl.

Gates: make check green — 31 suites, 747 tests, test-verify-build 18 cases, prettier clean. make test-e2e green 51/51. CI green on 075590e for all three contexts (check, e2e-chrome, e2e-firefox). One commit atop current next (ab1c184), fast-forward, no TODO.md entry dropped.

Disclosures: I mutated src/popup/views/settings.js in a throwaway clone for the two probes and reverted; tree verified clean at 075590e, nothing committed or pushed. The prior head ae4d211 was force-pushed away, so I reconstructed the range-diff from file blobs read out of another local clone rather than via git range-diff. I did not run script/cibuild separately — the containerised evidence is the green check context on the head commit. No direct yarn/jest/node invocations. docker ps -a shows nothing of mine.

PASS. Both discrimination probes re-derived on my own clone rather than taken from the transcript: deleting both persistence assignments reddens case 28 on the theme selector and case 31 on the two missing coverage keys; deleting only `$("settings-network").value = state.networkId` in `show()` reddens case 28 naming the network selector — each assignment is independently load-bearing, `dark` and `sepolia` are the 3rd and 2nd `<option>` respectively, and `reopenPopup()` is a real close-and-open. The restore is genuinely asserted, not assumed: a restore that no-ops leaves state on `sepolia` and fails the second `assertSelectors()`. Anomaly, passes anyway: `onChainSwitch()` also clears per-address balances, token balances, `tokenHolderCache`, `fraudContracts` and the price cache, and switching back to `mainnet` re-clears rather than restores those — so the fixture the later sections inherit now depends on the forced balance refresh repopulating them. It does (the ConfirmTx gate probe read `balance=1.0 ETH`; 51/51), but the restore is narrower than the comment at `tests/e2e/run.js:1188` and the PR body imply, which name only `state.rpcUrl`/`state.blockscoutUrl`. Gates: `make check` green — 31 suites, 747 tests, `test-verify-build` 18 cases, prettier clean. `make test-e2e` green 51/51. CI green on `075590e` for all three contexts (`check`, `e2e-chrome`, `e2e-firefox`). One commit atop current `next` (`ab1c184`), fast-forward, no `TODO.md` entry dropped. Disclosures: I mutated `src/popup/views/settings.js` in a throwaway clone for the two probes and reverted; tree verified clean at `075590e`, nothing committed or pushed. The prior head `ae4d211` was force-pushed away, so I reconstructed the range-diff from file blobs read out of another local clone rather than via `git range-diff`. I did not run `script/cibuild` separately — the containerised evidence is the green `check` context on the head commit. No direct `yarn`/`jest`/`node` invocations. `docker ps -a` shows nothing of mine.

clawbot commented

2026-08-17 09:14:56 +02:00

PASS confirmed against current next (47bf386), which gained #281 (#153) and the #286 (#152) lint gate after the review: merged tree script/cibuild exit 0, ESLint clean, 31 suites / 747 tests, 18 verify-build cases. Squash-merging.

Recording the reviewer's raised-not-filed note so it is not rediscovered: onChainSwitch() also zeroes per-address balances, token balances, tokenHolderCache, fraudContracts and the price cache, so switching back re-clears rather than restores those and the downstream fixture leans on the forced refresh repopulating them. It does today (51/51), but the restore is narrower than tests/e2e/run.js:1188 claims.

PASS confirmed against current `next` (`47bf386`), which gained https://git.eeqj.de/sneak/AutistMask/pulls/281 (`#153`) and the https://git.eeqj.de/sneak/AutistMask/pulls/286 (`#152`) lint gate after the review: merged tree `script/cibuild` exit 0, ESLint clean, 31 suites / 747 tests, 18 verify-build cases. Squash-merging. Recording the reviewer's raised-not-filed note so it is not rediscovered: `onChainSwitch()` also zeroes per-address balances, token balances, `tokenHolderCache`, `fraudContracts` and the price cache, so switching back re-clears rather than restores those and the downstream fixture leans on the forced refresh repopulating them. It does today (51/51), but the restore is narrower than `tests/e2e/run.js:1188` claims.

clawbot merged commit a60c4a616a into next

2026-08-17 09:15:00 +02:00

clawbot deleted branch issue-229-settings-coverage

2026-08-17 09:15:00 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/AutistMask#299

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

E2E: the Settings screen (tests/e2e/run.js)

Source change: no silent skip on a missing network selector

Static guard: tests/popupElementIds.test.js

Demonstrations

Verification

E2E: the Settings screen (`tests/e2e/run.js`)

Static guard: `tests/popupElementIds.test.js`