E2E: isolated test cases in their own CI job; mem-check measures only (#38) #40
No reviewers
Labels
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
schaefera/utStudyBIble!40
Loading…
Reference in a new issue
No description provided.
Delete branch "phase1/e2e"
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?
What
The user flows mem-check used to assert are now 8 isolated cases in
tools/e2e/(own HOME/XDG, KJV-only Bible folder, own Xvfb, fixed window; taps by the geometry the app logs). mem-check measures only start time and PSS (D10). CI gets ane2ejob betweenappandclick; it fails on any skip, and reruns the four timer cases in the slow profile. Locally:cargo make e2e-docker,E2E_CASES="picker-back aa-sheet"for some.Closes #38.
Blast radius
Tier: trunk
Trunk paths touched:
.forgejo/workflows/ci.yml,Makefile.toml,tools/ci/,tools/e2e/(new, added to the trunk list),tools/mem-check/,qml/pages/ReadingPane.qml,CLAUDE.mdTesting tier: maximum (CI config) (also: standard)
Gate / rollback: revert this merge. Nothing ships to users: the QML changes only add
console.loggeometry lines (utsb: grid,utsb: sheet) and aTheme.where()helper.Old check → case
restoreinactive-savescroll-chapters(reach guard also stays in mem-check)picker-backback-after-killjn 3,16jumpstyped-referenceaa-sheetaa-sheet-killProof
e2e: no case passed, exit 1; withE2E_STRICT=1(CI's default) skips count as failures.picker-backtwice 2/2.91873a8, localcargo make e2e-docker, KJV-only folder:e2e: 8 passed, 0 failed, 0 skipped, thene2e: 4 passed, 0 failed, 0 skipped, slow profile.cargo make app-docker(mem-check normal and SLOW=1) on both folders andcargo make: exit 0.91873a8: rust 3m22, audit 11s, app 1m54, e2e 3m04, click 1m04, all success. The app job went from 3m48 on main to about 1m55.Review
Reviewer: fresh-context
code-reviewat high (238), re-review at medium (239), delta review at medium (240): 6 findings (1+1 medium, 4 low), 6 fixed (84d6614,91873a8), 0 declined, 0 deferred, 0 fixed after the delta review, 0 left open.Review-Confidence: 100/100, a full high review plus two re-reviews cover every trunk file and every fix, CI is green on
91873a8, and each case is shown to fail against the defect it guards.Confidence by part
tools/e2e/lib.shwaits (e2e_settle,e2e_scroll): medium. Proven by defects and orders, but timing-based under software GL; flaked once on a CPU-starved runner before the box was power-cycled.a0b80e8e2e) predate the log artifacts; the artifacts now catch the next one.🤖 Generated with Claude Code
Level: HIGH (single pass). Commit:
7b48d19325. Range: origin/main...HEAD (22 files, +826/-383). Blast radius: trunk; testing tier: maximum.Files reviewed: .forgejo/workflows/ci.yml, Makefile.toml, tools/ci/e2e.sh, tools/ci/app.sh (context), tools/e2e/lib.sh, tools/e2e/run.sh, tools/e2e/cases/*.sh (8), tools/mem-check/run.sh (and origin/main's copy, compared assertion by assertion), tools/mem-check/budget-start-ms, qml/pages/PickerPage.qml, QuickSettings.qml, ReadingPane.qml, qml/theme/Theme.qml, CLAUDE.md, docs/09-quality-gates.md, docs/03-architecture.md.
Assertion parity: every assertion main's mem-check had is kept by a case or by mem-check (PSS and start budgets, chapters reached, no-jump, restore, inactive save, picker and Back, Back after kill, typed reference, Aa sheet and its kill). Not verified here: the targeted maximum-tier check (each case fails against the defect it guards, in the CI image). I could not run the CI image from this review.
Findings outside the diff lines:
click(needs: e2e) proceeds. On main this would have failed mem-check. Fix: in CI (e.g. E2E_REQUIRE=1 from e2e.sh) treat SKIP as failure, or fail when pass == 0.No cleanup path deletes or kills outside its own mktemp dir, pid or Xvfb. Isolation (HOME, XDG, Bible folder, display via -displayfd, per-launch logs) is sound.
@ -0,0 +68,4 @@e2e_bible=$e2e_work/data/utstudybible.schaefera/swordmkdir -p "$e2e_bible/mods.d"for e2e_m in "$@"; doe2e_module "$e2e_m" || { echo "$e2e_tag: SKIP: module $e2e_m is not in $E2E_SWORD"; exit 77; }medium · SKIP exits 77 and the runner counts it as non-failing, so a CI run where KJV is not found is green with 0 passed. Make SKIP a failure in CI, or fail when nothing passed.
Answering review 238 (reply to inline comment 1626 and the four lows). Fixed in
84d6614; no trunk path changed (tools/e2e, tools/ci, tools/mem-check, docs only; tools/ci/ and tools/e2e/ are in the trunk list of CLAUDE.md, so: yes,tools/e2e/andtools/ci/are trunk paths, no Rust/QML/C++ changed).84d6614.run.shnow exits 1 if no case passed, and withE2E_STRICT=1(set bytools/ci/e2e.sh, so by CI) a skip counts as a failure. Proof: against a SWORD folder without KJV,run.shgives "0 passed, 0 failed, 2 skipped / e2e: no case passed", exit 1 (default), and with E2E_STRICT=1 "(failed: restore(skipped) picker-back(skipped))", exit 1.e2e_quietone-gap on saves: fixed. Newe2e_settlewaits for the top-verse reports to go quiet, then for asavedline after the lasttopline (up to 15 s), then the saved-quiet gap, and starts over if a report came meanwhile. All cases ande2e_scroll_into_verseuse it.e2e_scrollpassing on a late initial report: fixed. It first waits for thetopreports to go quiet (the start's), then baselines.E2E_SLOW=1; mem-check uses it too) andtools/ci/e2e.shruns restore, inactive-save, picker-back and back-after-kill again in it (logs in<case>-slow).timeout -kleaking the sandbox: fixed. The runner points TMPDIR at the case's own out folder and removes it after the case, so a killed case leaves nothing outside.Runs:
cargo make e2e-dockerwith the full cache and with the KJV-only folder, each: 8 passed, 0 failed, 0 skipped, then slow profile 4 passed.cargo make app-dockerboth ways: exit 0, mem-check normal and slow peak PSS 140-146 MB (budget 165000 kB), 14 chapters. CI on the new head is running.Re-review (fresh context), level MEDIUM
Commit:
bbb6133771Range: 7b48d19325ffba43690e2f4648a6156b4f093392..HEAD
Files reviewed: docs/09-quality-gates.md, tools/ci/e2e.sh, tools/e2e/cases/aa-sheet.sh, back-after-kill.sh, inactive-save.sh, picker-back.sh, restore.sh, scroll-chapters.sh, tools/e2e/lib.sh, tools/e2e/run.sh, tools/mem-check/run.sh, .forgejo/workflows/ci.yml, core/src/worker/session.rs
Findings: one medium, inline on tools/e2e/lib.sh:205.
@ -0,0 +202,4 @@e2e_quiet 'utsb: top ' 1.1e2e_tops=$(e2e_count 'utsb: top ')e2e_j=0while [ "$e2e_tops" -gt 0 ] && [ "$(e2e_lineno 'utsb: saved ')" -le "$(e2e_lineno 'utsb: top ')" ]; domedium: e2e_lineno prints an empty string, not 0, when the pattern is absent (grep|tail|cut yields nothing, and
|| truedoes not add a 0). The core only logsutsb: savedwhen the position changed, so on a fresh sandbox, after the first scroll, the log hastoplines but nosavedline yet. Then[ "" -le N ]errors (integer expression expected) and returns 2, the wait loop is skipped, and e2e_settle falls through to e2e_quiet 'saved' 1.6 s. That is the gap-in-saves heuristic that this function exists to replace. On a loaded or slow-profile runner the first save arrives late, so restore, inactive-save and the first settle of picker-back or aa-sheet can read e2e_saved empty or stale. Fix: make e2e_lineno print 0 when there is no match (e.g.... | cut -d: -f1;then${n:-0}), as its comment says.Re review 239, comment 1633 (medium,
e2e_linenoprints nothing when nothing matches): fixed in91873a8. It now prints${e2e_ln:-0}. Checked undersh -eu: a log with twotoplines and nosavedgivessaved=0 top=2, the comparison holds and the wait goes on; after asavedline it gives 3. A delta review of bbb6133..91873a8 is running.Also in
bbb6133: the scroll-chapters limit is 15 verses a step, settled by the owner between the two agents' 10 and 15. On a quiet runner (run 104) steps were at most 7 verses; a loaded runner reported 11; a real jump is a whole chapter, 22 verses or more in Gen 1–11.Delta review (fresh context), level MEDIUM
Commit:
91873a83f2Range: bbb61337710d4dd5ac3b0e7d541cbd8d6241a6bd..HEAD
Files reviewed:
Findings: none. e2e_lineno has one caller (e2e_settle). Its output is now always a number: the assignment's status is cut's, so it is 0 under set -eu in dash even with no match or a missing log, and the empty case prints 0. The comparison
saved <= topwith saved=0 correctly keeps waiting for the first save, and with no top lines the inner loop is skipped by the e2e_tops guard.WIP: E2E: isolated test cases in their own CI job; mem-check measures only (#38)to E2E: isolated test cases in their own CI job; mem-check measures only (#38)Merge gate (MEDIUM) at
91873a8.Checked:
91873a8; no commits after it on origin/phase1/e2e.echo "${e2e_ln:-0}"), so e2e_settle's numeric comparison is safe.