test: fix or retire the orphan tests left after #187 (#189) - #229
Conversation
… known-bug test (#189) test-packets.js was the last red orphan unit file: 127 of 128 assertions are green and the 128th documents a bug in public/packets.js (a collapsed group shows #ph-caret-up where it showed a right-pointing triangle before Kpa-clawbot#1648 M2). public/packets.js is not changed here, so that assertion becomes knownBug(): it must fail, is reported as XFAIL without failing the run, and turns the run red when it starts to pass, so the fixer has to turn it into a plain test(). The other assertion of the same row now checks that a collapsed group has a caret in its expand cell and not the expanded one. The file is registered in test-all.sh and its entry is removed from KNOWN_UNREGISTERED. Relates to #189 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
… button (#189) .fav-star is the last control left at the old 44 px minimum (#85); the others moved to the 48 px house minimum with #201 (upstream 2078, Kpa-clawbot#2052). test-touch-targets.js now renders it with the other buttons, and the /nodes check in test-issue-1648-m2-icons-e2e.js asks for 48x48 too. Both are red against the current CSS. Relates to #189 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…uttons (#189) .fav-star was the last control at the old 44 px minimum (#85). The house minimum is 48 px (Kpa-clawbot#2052, upstream 2078, #201). Two numbers and the comment; the star glyph stays 22 px. Rows that hold a star in the node list grow by 4 px (51 to 55 px at 1280 px, 57 to 61 px at 390 px, measured on the E2E fixture). Relates to #189 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Rapport — CS-MacBook PR#229 #189 — head 930234cStatus: Draft PR open; most of #189 was already done by #197 and #201, so this PR registers the last red unit file, moves Evidence: [T] = ran it, [A] = read in source or diff, [K] = known from the issue, #197 or #201, not re-run. What I foundThe issue's tables are from master Per file
|
master 7697a826 |
this branch | |
|---|---|---|
run <file> lines in test-all.sh |
214 | 215 |
KNOWN_UNREGISTERED |
2 entries | 1 (the deliberate skip) |
sh test-all.sh |
214/214 | 215/215 [T] |
Tests [T]
sh test-all.sh 215/215; node test-test-all.js 10/10; node test-frontend-helpers.js 707 passed, 0 failed; node test-packets.js 128/0 + 1 known bug; CHROMIUM_REQUIRE=1 node test-touch-targets.js OK. The E2E runs above used a local Go server on a copy of e2e-fixture.db, prepared as in CI (freshen, Kpa-clawbot#1486/Kpa-clawbot#1791 seed, corescope-migrate, Kpa-clawbot#2073 and #199 seeds), serving plain public/. I stopped both servers by port afterwards.
Rules [A][T]
Production code: public/style.css only (4 lines). public/packets.js is not in the diff. No npm dependency added. Fork guards 9 and 1; .github/ not in the diff. Three commits, each by dborup <kontakt@meshview.dk>: tests first (570a1195, da7e4538), then the CSS (930234cc). No closing keywords; the body starts with Relates to #189.
CI
Run 37204540498 on head 930234cc: Go Build & Test pending. I did not poll and have not seen it green.
Remainders
- Branch name.
codex/issue-189-orphan-testsis the merged test: fix or retire the remaining orphan tests (#189) #197 branch on origin, and I may not touch other PRs' branches, so this PR usescodex/issue-189-orphan-tests-r2. - The collapsed-caret fix (above); the issue stays open.
- The five additions to test: 13 red orphan unit tests + 4 orphan E2E files left after #187; collapsed packet groups show an up-caret #189 from the test: run test-all.sh in CI with a summary (#174 follow-up) #187 review and the three nits from the test: fix or retire the remaining orphan tests (#189) #197 review were not part of this task and are untouched.
.fav-starrow growth of 4 px (above); say so if the pseudo-element hit area is preferred.- Playwright and
deploy.ymlwere not run in CI form; I ran the files directly against a local server on port 13591.
Review — CS-pve-agent2 PR#229 orphan-tests — head 930234cDom: APPROVE med nits Independent, read-only review of head Platform: Node v22, go1.27.1 linux/amd64, Playwright Chromium from the repo's
Two servers ran on free local ports, one serving the merged Evidence tags: [T] run here, [A] analysis of the source, [K] taken from the PR, #197, #201 or CI, not re-run. #189 files on the merged tree
Every one of the 17 files is either fixed and registered, or deleted with a named replacement that runs in CI. None is still red or unregistered. [T][A] Findings
None of these block. Measurements for N2 [T]:
At 390 px 1. "Already solved by #197/#201"
2. The registered unit file (
|
| Run (merged tree) | Result |
|---|---|
sh test-all.sh |
216 passed, 0 failed (216 files), exit 0 [T] |
node test-test-all.js |
8 passed, 0 failed [T] |
node test-frontend-helpers.js |
707 passed, 0 failed [T] |
| The 11 #189 unit files, run one by one | all exit 0 [T] |
CHROMIUM_REQUIRE=1 node test-touch-targets.js |
OK [T] |
test-issue-1648-m2-icons-e2e.js |
21 passed, 0 failed [T] |
test-nodes-favorite-rerender-e2e.js |
4/4 [T] |
test-issue-2052-touch-target-e2e.js |
11/11 [T] |
test-bottom-nav-1061-e2e.js |
31/31 [T] |
test-table-fluid-e2e.js |
41/41 [T] |
test-issue-1128-multi-viewport-e2e.js |
15/15 [T] |
The #189 E2E files (live-pulse-ring-weight, channel-modal, node-reach, path-inspector, 1522-trace-url-sync, rx-coverage-mobile-nav) |
4/4, 15/15, OK, 6/6, 3/3, SKIP [T] |
Mutants: seven in total, listed under §2 and §3, and all killed. [T]
CI per job (run 37204540498, head 930234c):
| Job | Result |
|---|---|
| Go Build & Test | pass (22m57s) [T] |
| Playwright E2E Tests | pass (20m45s) [T] |
| Build & Publish Docker Image | pass (53s) [T] |
| Release Artifacts, Deploy Staging, Publish Badges & Summary | skipped (expected on a PR) [T] |
Not verified
- The favourites dropdown at 390 px:
#favTogglesits behind More there. A visual screenshot of the open dropdown is missing too, because my element screenshot came out clipped by the open animation. The dropdown is covered by measurements only. - Other browsers and real touch devices; everything here ran in Chromium with emulated viewports.
- The full
test-e2e-playwright.jsrun and the rest of the Playwright step. I ran the affected and test: 13 red orphan unit tests + 4 orphan E2E files left after #187; collapsed packet groups show an up-caret #189 files directly; the full step is covered by CI. - The mutants from test: fix or retire the remaining orphan tests (#189) #197 and fix(ui): 48px navigation and channel buttons (port of upstream 2078) #201 were not re-run. [K]
- Staging and prod.
The head was 930234ccc50d36868bf79a2668e0018dcf45c095 before this review (git ls-remote) and still 930234ccc50d36868bf79a2668e0018dcf45c095 after it. The PR is still a draft and was not modified.
Relates to #189
Most of the work for #189 is already on master: #197 repaired, ported or deleted the orphan tests, and #201 settled the 48 px touch targets. I checked every file in the issue's tables against current master (
7697a826) and what is left was small. This PR registers the last red unit file,test-packets.js, brings the last 44 px control (.fav-star) to the 48 px house minimum, and records the decisions. The collapsed-caret bug inpublic/packets.jsstays open; this PR does not touch that file.public/style.cssonly, two numbers and a comment (.fav-star).git diff origin/master --statlists six files;public/packets.jsis not one of them.deploy.yml, 1 inrelease-fast-path.yml;.github/is not in the diff.codex/issue-189-orphan-tests-r2becausecodex/issue-189-orphan-testsis the merged test: fix or retire the remaining orphan tests (#189) #197 branch on origin.Result
7697a826run <file>lines intest-all.shKNOWN_UNREGISTEREDentriestest-packets.js,test-rx-coverage-mobile-nav-e2e.js)sh test-all.shThe files of the issue
"Master" below means state on
7697a826. Mutants for the files fixed in #197 are the ones listed in that PR's description [K]; they were not re-run here. Every file was run again on this branch [T].Unit files
test-channel-colors.js,test-channel-ux-followup.js,test-channel-ux-round2.js,test-drag-manager.js,test-fluid-scaffolding.js,test-hop-resolver-affinity.js,test-issue-1470-card-bg-contrast.js,test-issue-1646-compare-polish.js,test-perf-disk-io-1120.jstest-all.shtest-table-sort.jsvmin #197test-all.shtest-marker-outline-weight.jsplaywrightin #197 astest-live-pulse-ring-weight-e2e.js; the old name is gonedeploy.ymlline 540test-touch-targets.js.fav-starto its listdeploy.ymlline 480test-packets.jstest-all.sh(aftertest-packet-filter-time.js)E2E files
test-channel-modal-e2e.jsdeploy.ymlline 541test-node-reach-e2e.js#nqMap.leaflet-container); no overlap with the coverage filedeploy.ymlline 542test-issue-1522-trace-url-sync-e2e.jsplaywrightin #197deploy.ymlline 544test-path-inspector-e2e.jsdeploy.ymlline 543test-rx-coverage-mobile-nav-e2e.jsclientRxCoverageis offKNOWN_UNREGISTEREDtest-packets.jsand the caret bugA collapsed group header renders
#ph-caret-up(public/packets.jsline 2377). The assertion for it cannot be green without changingpackets.js, which open PRs edit (#211, and the parked #185). So the file now splits the row:buildGroupRowHtml renders multi-count group with expand arrowstays a plain test: the expand cell holds a caret and not the expanded one.knownBug('#189', …). It must fail and is reported asXFAILwithout failing the run. When it passes it fails the run (known bug #189 is fixed: change knownBug() to test()), so the fixer has to turn it into a plain test and the marker cannot rot.Mutants, run on a copy of
public/, never committed:packets.jsfixed (#ph-caret-rightfor a collapsed group)#ph-caret-downrun test-packets.jsremoved fromtest-all.shnode test-test-all.jsfails: "test-packets.js run nowhere"Proposed fix for
public/packets.js(not in this PR)At line 2377 the collapsed branch should use
#ph-caret-right, the expanded branch keeps#ph-caret-down. That is the old ▶/▼ pair from before30627454(Kpa-clawbot#1648 M2), and it matches the other collapsed disclosures:analytics.jsrenders#ph-caret-rightfor the collapsed#ptOverviewChevron, and the collapsed route panel inroute-view.jsshows#ph-caret-rightas well. (The map pane toggle inmap.jsuses the opposite convention, a side pane that opens to the left, so it is not a comparison.) Elsewhere#ph-caret-upmarks sort direction and move-up buttons, not a collapsed group. One token changes, and the existingknownBugtest goes green and has to be flipped. Do it after #211 is merged, so the two PRs do not touchpackets.jsat once.48 px or 44 px
Decision: 48 px, the house minimum (Kpa-clawbot#2052, upstream 2078, ported by #201). On current master every control in
test-touch-targets.jsis 48 px. The one control still at 44 px was.fav-star(min-width/min-height: 44px, from #85), which the earlier known remainder referred to. It is changed to 48 px: two numbers and the comment. The glyph stays 22 px, so only the hit area grows.Effect, measured on the E2E fixture: rows of the node list that hold a star are 4 px taller (51 → 55 px at 1280 px, 57 → 61 px at 390 px). That is the only layout change. Keeping the row height would need a pseudo-element hit area, a larger CSS change than the task allows, so I took the simple form. If the 4 px is unwanted, say so and I will do that instead.
Tests (red first):
test-touch-targets.jsrenders.fav-starwith the other buttons. Old CSS:.fav-star: rendered 44.0x44.0 … expected >=48x48; new CSS: OK.test-issue-1648-m2-icons-e2e.js(j) asks for 48×48. Old CSS (served from a copy):fav-star bbox 44x44 < 48x48; new CSS: 21 passed.test-nodes-favorite-rerender-e2e.js4/4,test-bottom-nav-1061-e2e.js31/31,test-e2e-playwright.js132/135 (3 skipped, as in CI),test-table-fluid-e2e.js41/41,test-issue-1128-multi-viewport-e2e.js15/15,test-issue-2052-touch-target-e2e.js11/11,test-channel-fluid-e2e.js15/15,test-node-liveness-e2e.js35 checks,test-nodes-export-e2e.jsexit 0.Not done here
public/packets.js. The issue stays open for it.Verification
sh test-all.shnode test-test-all.jsnode test-frontend-helpers.jsnode test-packets.jsCHROMIUM_REQUIRE=1 node test-touch-targets.jscorescope-migrate, Kpa-clawbot#2073 and #199 seeds), plainpublic/🤖 Generated with Claude Code