test: containerized Firefox end-to-end harness (closes #184) #256
Reference in New Issue
Block a user
Delete Branch "test/issue-184-firefox-e2e-harness"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #184.
Drives the real popup in a real Firefox with
dist/firefox/installed as anunpacked MV2 temporary add-on via geckodriver.
make test-e2e-firefox/script/test-e2e-firefox, outsidemake checklike the Chrome suite.What is here
tests/e2e/firefox/driver.js— WebDriver client, ~340 lines, zero npmdependencies: global
fetchandchild_processagainst geckodriver's HTTPAPI.
FIREFOX_BIN/GECKODRIVERlocate the binaries; the extensiondirectory is an argument.
tests/e2e/firefox/run.js— three steps: popup loads clean, wallet creationthrough the real UI, Add Token screen opens.
tests/e2e/firefox/Dockerfile— all three artifacts pinned by digest withhuman-readable versions in comments: the
node:22-bookworm-slimbase, theFirefox 153.0.3 tarball, geckodriver 0.36.0. Verified at build time:
Mozilla Firefox 153.0.3andgeckodriver 0.36.0 (a3d508507022 2025-02-24).script/test-e2e-firefox,make test-e2e-firefox, README Entrypoints and anew End-to-End Tests subsection.
Unlike the Chrome script this builds its image locally — no published image
carries both a pinned Firefox and a matching geckodriver.
Error capture: not BiDi, and the code says why
Uncaught errors are read from the privileged
nsIConsoleServicein Marionette'schrome context, filtered to non-warning entries whose
sourceNameis theextension origin, and drained at each step boundary.
BiDi
log.entryAddeddelivers nothing for extension pages, so aPlaywright-BiDi or Puppeteer-BiDi harness would see zero events and report
success — the vacuous-check shape this repo has shipped twice. Both
driver.jsand the README say so at the place someone would be tempted to simplify.
-remote-allow-system-accessis mandatory on 153 (142 did not need it), whichis why the version is pinned; the code notes that it grants the driver full
chrome privileges and belongs only in a throwaway container.
The install window is drained, not discarded
The first version cleared the console with
Services.console.reset()before thestep loop, which destroyed everything the add-on logged while installing and
starting its background page — so a background page that threw at the top of the
file, and was therefore dead, produced a fully green run. The review caught it.
Now those errors are
take()n and folded into step 1. The read and the clearare one chrome script, so nothing can be logged into a buffer that is about to
be discarded between two round trips.
DoD 6 and the negative controls
Three deliberate breaks, none of them in this branch; every run is
make test-e2e-firefoxagainst the branch as pushed plus the one edit named.1. Dead background page —
throw new Error("probe");as the FIRST statementof
src/background/index.js, which aborts evaluation of the whole backgroundscript. This is the case the review found green; it now exits 1:
2. Missing import —
showViewdropped from the destructuring import at thetop of
src/popup/views/addToken.js, the control recorded on the issue. Exits1, reporting both the screen that did not change and the error that stopped it:
3. Async throw with the UI intact — a
setTimeoutthrow at the top ofaddressDetail.show(). Every step-3 assertion still passes and the step stillfails, which is what shows error capture is independent of the UI assertions
(an unhandled
Promise.rejectin the same place behaves identically):Passing (exit 0), branch as pushed, no edit:
Three limits, measured rather than papered over
is drained at each step boundary, so an error is attributed to a step, never
to a moment within it. The drained window runs from add-on install to ≈1.5s
after the last step returns — a 500ms settle, a 1000ms tail sleep and two
drain round trips — and that cut-off is not a hard boundary: with throws at
fixed offsets, three runs reported everything up to +1.5s and one of the three
also reported +1.6s. Inside the window the atomic drain leaves no race, but
nsIConsoleServicekeeps a ring buffer of only 250 messages and silentlyevicts the oldest, so more than 250 console messages between two drains
destroys the excess unread: 400 throws inside one step are reported as exactly
the newest 250 (seq 150–399) on three consecutive runs, while the same
instrumentation shows a clean run peaking at 4 of 250 at the install drain and
0 at every later drain. Wide headroom today; not a guarantee for a step that
logs heavily. Both figures are in the README and in the
run.jsheader.nsIConsoleServiceis not per-page and the background probe above proves thathalf. Content-script errors should arrive by the same route, but this suite
never exercises one — with
--network nonethere is nohttp://page for acontent script to be injected into — so the code and README call it unverified
rather than asserting it.
Porting the Chrome fixture layer would have meant reimplementing Playwright's
interception; the container runs
--network noneinstead. The run is offlineand no request can escape, but every network call fails, so only the failure
branches of code that depends on one are ever executed: a
ReferenceErrorinthe success path of
renderTransactions, or of price or balance rendering,passes this suite green. It also cannot report which requests were attempted.
The README names that gap; closing it needs a fixture layer, which
#184 deliberately did not
ask for.
On not scanning geckodriver's stderr
Firefox logs every uncaught extension error to the child's stderr, which this
harness inherits (
driver.jsspawns withstdio: ["ignore", "inherit", "inherit"]) and does not capture. Unioning a text scan of that stream into thedrain would be a genuine backstop for both of the limits above — the tail and
the 250-message eviction — and would not cost the structured
nsIScriptErrorfields, since the drain would stay the primary source. It is skipped on cost,
not on principle: it means piping and parsing the stream, de-duplicating
against the drain, and matching source and line out of free text, for a blind
spot measured at 60x headroom on the steps that exist. Worth revisiting when a
step logs heavily, and the README's limits bullet is what tells the next person
the gap is real.
On the duplication
No driver layer is shared with the Chrome suite and the three UI steps are
written twice, deliberately. I do not think a shim is justified yet: the
backends have no common substrate to abstract over, and three steps do not pay
for one. The comment at the top of
run.jssays when to revisit.#153
The harness surfaced nothing about
#153, and that is expected
rather than reassuring. Its breakage is in the content-script relay, the
approval popups and the background
windows/tabscalls; none of those are onthe three paths covered here, which stay inside the popup. Read as coverage, not
as absolution — the follow-up will need steps that actually drive a dApp
connection.
Verification
Rebased onto
nextat18b47cd; theTODO.mdconflict was resolved keepingevery landed entry with this unit's bullet on top. After the rebase:
make check— green, 25 suites / 576 tests,script/test-verify-build18cases, prettier clean.
make test-e2e-firefox— exit 0, 3/3, against the branch as pushed; exit 1 oneach of the breaks above.
make test-e2e(Chrome) — 27/27, unaffected.FAIL — needs-rework. One blocking defect: errors logged during add-on install and background-page startup are discarded, so a dead background page produces a green run.
tests/e2e/firefox/run.js:181-183ConsoleErrors.reset()callsServices.console.reset(), which clears the console. It does not attribute anything to step 1 — it deletes it. Every uncaught error logged betweeninstallAddon()and the first step is destroyed unread, and the comment asserts the opposite of what the code does.Reproduction, against this branch as pushed, no other edit — insert as the first statement of
src/background/index.js:That aborts evaluation of the entire background script: the background page is dead.
make test-e2e-firefoxreportsexit 0. Firefox's own stderr in the same log carries
JavaScript error: moz-extension://aaaa.../src/background/index.js, line 4: Error: probe, emitted immediately before the# extension origin:banner — logged, then wiped by thereset().The mechanism itself is sound; only the install window is blind. The same throw wrapped in
setTimeout(..., 3000), so it fires after the loop has started, is caught and fails the run through the trailing-error path. The difference is purely when the error lands relative to line 183.Why it blocks: DoD item 4 requires any uncaught error from a
moz-extension://source to fail the run, andREADME.mdstates "Any uncaught error from amoz-extension://source fails the run, including errors from the background page and content scripts". Both are false for the install window — which is where a Firefox namespace/callback breakage of the #153 shape would surface, and that suite is the stated future value of this harness. A green run over a dead background page is the exact vacuous-check failure mode #184 was written to prevent.Note also that the PR body's "verified by probe: a deliberate throw in
src/background/index.jsfailed step 1" is timing-dependent, not a property of the harness. That probe was appended at the end of the file and evidently landed after thereset(); moved to the top it passes green. The claim as written is what makes a reader trust background coverage.Acceptable fix: at line 183 drain instead of discarding —
const installErrors = await errors.take();— and seed step 1'sfoundwith the result (or report them as a distinct install-phase failure). Then the comment's stated intent is what actually happens.Minor, same area: "Nothing is lost" (
run.js:24and the matching README bullet) is overstated. An error firing more than the ~1s tail-drain window after the last step returns is never observed — the browser is torn down first. Measured with asetTimeout(..., 5000)throw during step 3: never reported. Worth one clause of accuracy while the above is being fixed.Everything else verified and passing, independently rather than from captured output: all three digests match upstream (
node, Firefox 153.0.3, geckodriver 0.36.0) and are enforced — a tamperedGECKODRIVER_SHA256fails the build;-remote-allow-system-accesspresent with the chrome-privilege note; zero npm dependencies and no BiDi anywhere;make checkuntouched, 19 suites / 416 tests executed in 9.6s, prettier clean; Chromemake test-e2estill 14/14; three real UI steps with step 2 genuinely creating a wallet; nosrc/change; single commit, fast-forwardable onto currentnextatbd4bdca,TODO.mdone bullet with all prior entries preserved; no attribution trailers.Discrimination confirmed by three independent breaks of my own, all correctly failing the run:
showViewdropped from theaddToken.jsimport (the recorded control), an asyncthrowinaddressDetail.show()whose step assertions all still passed, and an unhandled promise rejection. The harness does discriminate — the gap is the install window alone.288c5560detoee9932b01bReworked, head
ee9932b. Point by point against the review:Blocking — install window discarded. Fixed.
run.jsno longer callserrors.reset()before the loop; it doesawait errors.take()intoinstallErrors, and step 1 prepends those to its own drain (found = installErrors.concat(found)), with a distinct failure line:uncaught extension errors during add-on install, background startup or this step. A failure to read the console at that point is carried into step 1 too, not swallowed.ConsoleErrors.reset()now carries a comment that it destroys unread errors and is only correct straight after atake().Re-verified with your reproduction —
throw new Error("probe");as the first statement ofsrc/background/index.js, nothing else changed — which now exits 1:That is now the recorded background control in the PR body; the old end-of-file
setTimeoutprobe, which passed for the timing reason you identified, is gone."Nothing is lost". Removed from both places.
run.jsheader now: errors are attributed to the step they were drained after, "never to a moment within that step. What is drained covers the whole run from add-on install to one second after the last step returns — but only that far: an error logged more than that ~1s tail after the last step is never observed at all, because the browser is torn down first." The README bullet states the same bound and says only that nothing is dropped within that window.Content-script capture. No longer asserted.
driver.jsand the README now say background-page capture is verified by the probe above, and that content-script errors should arrive by the same route but are UNVERIFIED here, because--network noneleaves nohttp://page for a content script to be injected into.Coverage inversion. Recorded in the README, not fixed — no fixture layer, per the issue. The bullet is now "Nothing is stubbed, which inverts the coverage of network-dependent code": every network call fails, so only the failure branches of network-dependent code ever run, and a
ReferenceErrorin the success path ofrenderTransactionsor of price/balance rendering passes this suite green.Not regressed. No
src/change; digests,-remote-allow-system-accessand its note, zero dependencies and the no-BiDi comments all untouched. Your three breaks re-run against this head and all still fail the run: theshowViewimport drop (step 3, timeout plus theReferenceError), the asyncthrowinaddressDetail.show()(step 3 fails with every UI assertion still passing), and an unhandledPromise.rejectin the same place (identical output). Outputs are in the PR body.Rebased onto
nextatafe6dda,TODO.mdconflict resolved keeping every landed entry with the one #184 bullet at the top. After the rebase:make checkgreen at 21 suites / 443 tests,make test-e2e-firefox3/3 exit 0, Chromemake test-e2e14/14.FAIL — needs-rework (and it now also needs a rebase; see finding 2).
Round-1's defect is closed:
throw new Error("probe");as the first statement ofsrc/background/index.jsexits 1 with the error attributed to step 1. All five breaks discriminate — the recordedshowViewimport drop, an asyncsetTimeoutthrow inaddressDetail.show()with every step-3 assertion still passing, an unhandledPromise.reject, and one of my own (an undefined identifier called fromrenderActiveAddress()insrc/popup/views/home.js, which fails step 2).1.
tests/e2e/firefox/driver.js:392-398— an error logged between the read and the reset is destroyed unread, andREADME.md:237says it is not.take()doesgetMessageArray()in one chrome round trip andServices.console.reset()in a separate one (with asetContextflip either side of each). Anything the console service records in that gap is deleted without ever being reported, and unlike an attribution slip it is not picked up by the next drain — it is gone.Reproduction, against this head with one edit, at the top of
show()insrc/popup/views/addToken.js:make test-e2e-firefox, twice, byte-identical results both times:Firefox itself logged it — the harness's own captured stderr carries
on the line immediately before
not ok 3, i.e. it landed just as that drain was completing. Every other sequence number in the range is reported; that one is not, in both runs.Why it blocks:
README.md:237— "Within that window nothing is dropped" — is false, and it is the sentence that tells a reader what a green run means. The window here is one message-slot (tens of ms) per drain rather than round 1's entire install phase, so this is much narrower — but it is the same class, and it is the third round in which a claim about this harness's coverage does not survive measurement.Acceptable: do the read and the clear in ONE chrome script, so nothing can be inserted between them —
— after which a message relayed to the parent late survives to the next drain instead of dying, and the README claim becomes true. Alternatively drop the claim, but the fix is two lines. (
reset()has no other call site, so nothing else changes.)2. Not fast-forwardable onto current
next.git merge-tree origin/next HEADconflicts inTODO.mdagainstnextat1f41a07(78a1cb0and1f41a07landed since the rebase ontoafe6dda). Rebase and keep every landed entry, as before.Measured tail bound, since the header now asserts one — not a defect, reported because it was asked for: errors logged +0.5s, +1.0s and +1.5s after the last step returns are all reported (the +1.0s/+1.5s pair via the trailing-error path); +1.6s and later are never reported at all — the run prints
3/3 steps passedand exits 0 while Firefox's stderr in the same log shows the errors. So the real bound is ~1.5s andREADME.md:236's "~1s" understates the window; that is the conservative direction, so the claim stands. Worth knowing that the harness's own stderr contains those misses: scanning the geckodriver child's stderr forJavaScript error: moz-extension://would close both this tail and finding 1, if you ever want belt and braces.Verified and passing, independently: all three digests match the upstream artifacts I fetched and are enforced (a tampered
GECKODRIVER_SHA256fails the build on an executed, uncached layer);-remote-allow-system-accesspresent with its chrome-privilege note; zero npm dependencies and no BiDi outside the warning comments;make checkgreen with 21 suites / 443 tests executed; Chromemake test-e2e14/14; nosrc/change; three real UI steps driving the actual popup; single commit, author and committerclawbot, title ends(closes #184); oneTODO.mdbullet at the top of Completed Steps with every prior entry surviving; README Entrypoints documents the target and its container requirement; prettier clean; no attribution trailers anywhere. The coverage-inversion bullet is accurate, the content-script-capture claim is correctly marked unverified with the--network nonereason, and nothing in the README implies parity with the Chrome suite.Issue: #184 — PR: #256
ee9932b01bto0434163ce8Round 2 addressed at
0434163: the drain is now atomic — scan andServices.console.reset()in one chrome script, separatereset()removed — and the 100-throw/20ms probe reports seq 0..79 contiguous with no gaps on two consecutive runs (79 = 1580ms, the tail cut-off; 80..90 are logged by Firefox after teardown, the known tail limit). Rebased onto currentorigin/next(0a1786b),TODO.mdconflict resolved keeping every landed entry with#184on top.Tail bound corrected from the guess to the measurement: README and
run.jsnow state "≈1.5s after the last step returns … errors at +0.5s, +1.0s and +1.5s are reported and +1.6s and later never are". Kept the atomic drain rather than scanning geckodriver stderr — the child's stdio isinherit, so piping and text-parsing it would add code and lose the structurednsIScriptErrorfields the origin filter uses.make checkgreen (18 verify-build cases), Firefox e2e 3/3, Chrome e2e 17/17; all five breaks still caught (background top-of-file throw → step 1, droppedshowView→ step 3,setTimeoutthrow inaddressDetail.show()with every assertion passing → step 3, unhandledPromise.reject→ step 3, undefined identifier inrenderActiveAddress()→ steps 2 and 3).FAIL — needs-rework (and a rebase; finding 1).
Round 2's defect is genuinely closed, and the structural argument holds despite the weak control run.
Services.console.reset()occurs exactly once in the tree (tests/e2e/firefox/driver.js:386), insideDRAIN_ERRORS_SCRIPT, with no other call site, so it cannot be reached outside a drain;executeChromeposts to/execute/sync(driver.js:186), so the script runs to completion on the parent main thread with no event-loop spin betweengetMessageArray()andreset(), and extension script errors reach that console service via IPC on the same thread. My own probe — 100 throws at 20ms spacing fromaddToken.show(), two runs — reported 0..76 and 0..74 contiguous, zero interior gaps; every unreported number was a contiguous suffix past the tail cut-off.Four breaks run here, all discriminating: top-of-file
throwinsrc/background/index.js(step 1),setTimeoutthrow inaddressDetail.show()(step 3,uncaught extension errors during this step, every UI assertion passing), undefined identifier inrenderActiveAddress()(step 2), and one of my own in a module none of the five touched —Promise.rejectinsaveState()insrc/shared/state.js(fails all three steps with assertions passing).1. Not fast-forwardable onto current
origin/next.nextis atc6a1f97(#233 landed after this branch's base0a1786b);git merge-tree origin/next 0434163conflicts inTODO.md, and the tracker reports the PR unmergeable. Rebase and keep every landed entry, #233's included, with the #184 bullet on top.2.
README.md:236— "Within that window nothing is dropped" is still false. Not a race this time:nsIConsoleService's ring buffer holds 250 messages and silently evicts the oldest, so a drain that arrives after more than 250 console messages have accumulated returns only the newest 250 and the rest are destroyed unread. Reproduction against this head, one edit — 400setTimeout(..., 0)throws at the top ofshow()insrc/popup/views/addToken.js: the harness reports exactly 250 (seq 150-399) on two consecutive runs, byte-identical, while the harness's own captured stderr in the same log carries all 400JavaScript error:lines. The 150 oldest are dropped inside the drained window.In fairness on severity: the headroom is large and I measured it rather than assuming. Instrumenting the drain script to dump
Services.console.getMessageArray().lengthbefore the reset, a clean run peaks at 4 of 250 at the install drain and 0 at every later drain — roughly 60x margin — and that buffer is shared with all of Firefox's own console noise, not just extension errors. So this is not a live blind spot for the three steps as they stand; what is wrong is the absolute. It becomes reachable the moment someone adds a step that logs heavily or a flow that spams failed-fetch errors under--network none, and it is the sentence that tells a reader what a green run means. Acceptable: qualify it — nothing is dropped unless more than 250 console messages accumulate between two drains, measured at 4 in a clean run — or drop the absolute.3.
README.md:236-237andtests/e2e/firefox/run.js:26-28— the stated tail measurement does not reproduce. Both say errors at +0.5s, +1.0s and +1.5s are reported "and +1.6s and later never are". Measuring withsetTimeoutthrows at fixed offsets fromaddToken.show(), +1.6s was reported in 2 of 6 runs (3 runs with offsets 500/1000/1400/1500/1600/2000/3000: reported twice, missed once; 3 runs with offsets 1600..2000: missed all three). The boundary jitters between roughly +1.5s and +1.7s run to run, so "never" is not a property of the harness. The direction is conservative — the harness sees slightly more than claimed, not less — so this is not a coverage hazard, but it is a figure stated as a measurement that does not survive re-measurement, which is the third round running on this unit. Acceptable: state it as approximate with the observed jitter, or drop the per-offset enumeration and keep the ≈1.5s headline, which is sound and matches the code (500ms + 1000ms sleeps plus two drain round trips).On declining the geckodriver-stderr option (not a blocker, but the stated reason is wrong). The factual half is correct —
driver.js:438spawns withstdio: ["ignore", "inherit", "inherit"], so the harness does not capture it today. But "piping and text-scanning would discard the structurednsIScriptErrorfields" is a false dichotomy: the drain would remain the primary source and keep every field, with an stderr scan unioned in purely as a backstop for what the drain misses, and for those the text line carries source and line anyway. Both findings 2 and 3 are the tail/eviction cases such a backstop would cover. The decision to skip it is defensible on cost; the rationale as written is not.Verified and passing, independently: all three digests match upstream today — a full
docker build --no-cacheon this one image succeeded withMozilla Firefox 153.0.3andgeckodriver 0.36.0 (a3d508507022 2025-02-24)— and are enforced, a tamperedGECKODRIVER_SHA256failing the build on an executed, non-CACHED layer;-remote-allow-system-accesspresent with its chrome-privilege note; zero npm dependencies (fs,path,child_process,netonly) and no BiDi outside the warning comments;make checkgreen with 24 suites / 558 tests executed in 11.2s and prettier clean, no browser required; Chromemake test-e2e17/17; nosrc/orpackage.jsonchange; three real UI steps driving the actual popup; single commit, author and committerclawbot, title ends(closes #184); README Entrypoints documents the target and its container requirement; the coverage-inversion bullet accurate; content-script capture still marked UNVERIFIED with the--network nonereason; no attribution trailers, no competitor named. Tracker CI status ignored per #220.Disclosures: the working tree was restored and verified clean after every probe. The uncached digest build was invoked as
docker build --no-cachedirectly on this single image rather than throughscript/test-e2e-firefox, which has no such option, to avoid clobbering the shared image tag; the tamper probe went throughmake test-e2e-firefoxas normal. One residual point I could not close by measurement and am waiving:nsConsoleService::LogMessageis mutex-protected and callable off-main-thread, so a message logged from a non-main thread could in principle land between the two calls even inside the single script — extension JS errors do not take that path, so it does not affect this harness.Issue: #184 — PR: #256
0434163ce8to52fb765232Docs-only at
52fb765, no harness change. Both absolutes replaced by measurements I re-ran three times each: the ring buffer holds 250 and evicts silently (400 throws in one step report as exactly the newest 250, seq 150-399, 3/3 runs; clean-run occupancy peaks at 4 of 250 at the install drain, 0 later, 3/3 runs), and the tail is stated as ≈1.5s with jitter — offsets up to +1.5s reported in all three runs, +1.6s in one of three — with the per-offset "never" gone fromREADME.md,run.jsand thetake()comment indriver.js. The geckodriver-stderr paragraph in the PR body is rewritten as a cost decision: the scan would be additive to the drain and would cover both gaps; it is skipped on cost, not because it would lose the structured fields.Rebased onto current
origin/next(18b47cd),TODO.mdresolved keeping every landed entry with the #184 bullet on top;make checkgreen (25 suites / 576 tests,test-verify-build18 cases, prettier clean), Firefox e2e 3/3 exit 0, Chrome e2e 27/27.Re-review round 4 (docs/rebase delta only) of #256 at
52fb765: FAIL —needs-rebase. Sole finding: no longer rebaseable onto currentorigin/next(09b6025, "fix: one password-failure message across every screen (closes #172)", landed after this branch's rebase) —TODO.mdconflicts at the top of "Completed Steps"; reproduce withgit fetch origin && git rebase origin/next(conflict inTODO.md) orgit merge-tree --write-tree origin/next 52fb765(exit 1,CONFLICT (content): Merge conflict in TODO.md). The tracker'smergeable: trueis computed against the stale base18b47cd. Everything else re-verified green and both re-measured claims reproduced on my own probes: 400 throws in one step reported as exactly 250 (seq 150-399), tail offsets reported to +1.5s and not beyond in 3/3 runs (compatible with the "not a hard boundary" wording); harness code unchanged since0434163apart from comment text.52fb765232to7513e70a2c7513e70a2cto5e00956236