test(web-wallet): E2E follow-ups from #177 (PR 1 — light bundle) - #178
test(web-wallet): E2E follow-ups from #177 (PR 1 — light bundle)#178raul-oliveira wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
raul-oliveira
moved this from In Progress (WIP)
to In Progress (Done)
in Hathor Network
raul-oliveira
changed the title
Low-risk follow-ups from #177; no production runtime change. - item 1: e2e scripts build @hathor/hathor-rpc-handler before Playwright - item 3: move timeouts to shared tests/e2e/timeouts.ts + TIMEOUTS.dapp; route the 14 hardcoded webWallet.ts waits through E2E_TIMEOUT_SCALE - item 4: bounded retry on the SRP recovery-word read (slow renders) - item 5: unit test pinning the SNAP_TIMEOUT_MS default to 10s - item 7: disposable testnet-stub / dry-wallet top-up docs note - item 8: loud guard on an unsupported MetaMask Flask version - item 2a: document the non-gating contract in e2e.md - fix: keep token-lifecycle test titles static — Date.now()-derived symbols in the titles made Playwright abort with "Test not found in the worker process" Refs #177
c8b266e to
15b74c3
Compare
|
|
||
| ## Two timeout knobs — which to reach for | ||
|
|
||
| - **`E2E_TIMEOUT_SCALE`** (test side) multiplies every deadline in `tests/e2e/timeouts.ts` — |
There was a problem hiding this comment.
suggestion(non-blocking): file map still lists timeouts.ts under driver/ after the move
Pinned to this changed line; the concern is the ## File map block at e2e.md:266, outside the diff. It still nests timeouts.ts under driver/, and tests/e2e/timeouts.ts is missing from it — so the doc now contradicts the section right here. Since e2e.md opens with Read this before reading or writing any E2E test, that map is what the next contributor navigates by: they look under driver/, find nothing, and inline a magic number — the regression the move was meant to prevent.
Worth knowing a stale-path grep won't catch this; the map is an indented tree, so the path never appears as a literal string.
| @@ -1,4 +1,5 @@ | |||
| import { expect, type Page } from '@playwright/test'; | |||
| import { TIMEOUTS } from '../timeouts'; | |||
There was a problem hiding this comment.
suggestion(non-blocking): two connect-path deadlines escaped the centralization
Pinned to the new import; the sites are waitForMetaMaskProvider(timeoutMs = 30_000) at line 96 and the 8s wallet_getSnaps probe cap at line 83, both outside the diff. Neither scales with E2E_TIMEOUT_SCALE, and both sit on the cold-start connect path that item 2b targets — provider injection and the service-worker wake are exactly what run long on a slow machine. waitForMetaMaskProvider also swallows its own timeout (.catch(() => undefined)), so it degrades into a later, opaque failure rather than a clear one.
Suggest TIMEOUTS.dapp.providerInject / .providerWarm.
| // Chips populate asynchronously; on a slow render a one-shot read can catch an empty chip | ||
| // and turn a transient into a hard failure. Poll the read (bounded by TIMEOUTS.seedReveal) | ||
| // until every word is present, THEN keep the fail-fast throw for a genuinely missing word. | ||
| const deadline = Date.now() + TIMEOUTS.seedReveal; |
There was a problem hiding this comment.
suggestion(non-blocking): seedReveal bounds the loop but not a single pass
The deadline is only checked after readSeedWords returns, and each chip read inherits actionTimeout: 45_000 (playwright.e2e.config.ts:67). For attached-but-empty chips — the case your comment describes — this works as intended. If a chip isn't attached yet, one pass alone can burn 45s × 12 and blow the 180s test timeout, so Playwright kills the test generically and neither dumpScreen('recovery-phrase-missing-word') nor the actionable error message ever fires.
Passing a short per-read { timeout } inside readSeedWords would make the 15s bound real.
| }; | ||
| actual = manifest.version ?? ''; | ||
| } catch { | ||
| return; // an unreadable/absent manifest is already handled by findManifestDir; don't double-fail |
There was a problem hiding this comment.
nitpick: an unparseable manifest silently bypasses the version guard
findManifestDir only tests existsSync(manifest.json) — it never parses it. So a truncated or half-extracted .cache/flask-*/manifest.json throws here and returns, skipping the guard entirely; that is one of the two scenarios the docstring above names (a stale .cache). Re-throwing, or at least warning, would keep the failure loud.
| * the driver's MetaMask deadlines above and from `SNAP_TIMEOUT_MS` (the dApp's 10s snap-RPC | ||
| * cap). Scaling `E2E_TIMEOUT_SCALE` now stretches these too. | ||
| */ | ||
| dapp: { |
There was a problem hiding this comment.
nitpick: driver/windows.ts deadlines also sit outside the centralized system
Pinned here in the file they'd move into; the sites are driver/windows.ts:25 (timeout: 15_000) and :87 (timeoutMs = 10_000), outside the diff. Neither scales with E2E_TIMEOUT_SCALE. Lower stakes than the connect-path ones — this is the headed-only two-window split — but they're the last inline deadlines left in the E2E tree, so folding them in would let the docs claim every deadline without an asterisk.
| onboarding + import screens, confirming each `data-testid` in `driver/selectors.ts` | ||
| (`MetaMask.onboarding.*`, `MetaMask.importing.*`, `Srp.*`, `Unlock.*`) still resolves. | ||
| 3. Update any changed selectors, then set `EXPECTED_FLASK_VERSION` to the new version. | ||
| 4. Update the pinned download checksum (see the Flask-integrity note, item 6 / PR 4). |
There was a problem hiding this comment.
nitpick: checklist step 4 points at a checksum that doesn't exist yet
Item 6 is explicitly out of scope for this PR and driver/flask.ts has no checksum today, so anyone following this checklist right now stalls on a step with nothing to update. Worth marking it as pending until that PR lands.
2 participants
Motivation
Follow-up improvements to the real-MetaMask E2E suite added in #169, tracked in #177. This is PR 1 of 4 and lands the six low-risk items plus item 2's docs slice. None of these change production runtime behavior. The heavier items get their own PRs: de-flake (item 2b), nightly CI (item 2c), and Flask download checksum (item 6).
Acceptance Criteria
Addresses these #177 items:
yarn e2eis self-sufficient from a clean checkout: builds@hathor/hathor-rpc-handlerfirst (only the handler —snap-utilsis webpack-aliased to source).E2E_TIMEOUT_SCALE-aware system (moved totests/e2e/timeouts.ts, addedTIMEOUTS.dapp; routed the 14 hardcodedwebWallet.tswaits).TIMEOUTS.seedReveal) before failing, for the slow renders this suite targets.SNAP_TIMEOUT_MSproduction default to 10s (empty→10s, valid override applies, invalid→10s).EXPECTED_FLASK_VERSION+assertFlaskVersion,E2E_ALLOW_FLASK_MISMATCH=1escape hatch) plus an upgrade checklist.e2e.md).token-lifecycle.spec.tsembeddedDate.now()-derived token symbols in its test titles, so Playwright aborted the journey with "Test not found in the worker process"; titles are now static and the per-run symbols stay in the test bodies.Out of scope (separate PRs): item 2b (de-flake), 2c (nightly CI), 6 (Flask download checksum).
Validation
Run headed against real MetaMask Flask 13.31.0, inside
nix develop:yarn lint— clean.yarn test --run— 268/268 (incl. the newSNAP_TIMEOUT_MStest; sanity-checked by breaking the default).e2e:depsrebuildsdistafter removal (item 1).yarn e2e— all four journeys green:onboarding9/9,import2/2,feature-example1/1,token-lifecycle9 passed (+1 flaky-then-green). Cold-start connect flakes on some first attempts (item 2b's target) and clears on retry.Checklist
master, confirm this code is production-ready and can be included in future releases as soon as it gets mergedRefs #177