Repository navigation
Work through DEFECTS.md: sixteen of seventeen entries - #163
Merged
Merged
Conversation
The comments indexed findings under an earlier scheme running to #160, which this file restarted at #1. That numbering still appears in source comments — full-calendar-test.js cites "DEFECTS.md #94 for Leaflet" — and resolved to nothing once the comments went. Appendix A maps the old numbering to what actually happened. I verified each against current source rather than copying the claims across, and every finding in that set has since been resolved: filter/multi-option's task-called-as-a- function, overlay.resize clamping the wrong dimension, custom-field/form (the file is gone), set-height's "px", leaflet's never-set `initialized`, transition-to's self-satisfying assert, resource-context-panel's validate order, the money arm, and the whole dead-member list. #148 in particular — the three query-builder validate* actions nothing performed, so a panel kept sorting by deselected columns — is wired to {{did-update}} in all three panels now. Appendix B keeps the seven categories the remaining gaps fall into, including the distinction that cost the most time: a default on a plain function is real reachable surface, a default on a framework-invoked signature is dead, and istanbul's branch map cannot tell them apart. Category 7 is annotated with what has happened since — most of the "harness question" files turned out to be drivable, so it is now flagged as unverified-until-tried. Appendix C keeps the habits, led by the one that keeps proving itself: a green test is not evidence that the branch you aimed at ran. Nothing is imported as an open item. The tracker's open list is unchanged at #1-#17.
Also names what the workflow's existing `test -s coverage/lcov.info` step does and does not catch: it guards the crudest form of the missing-artifact case, but it checks lcov.info rather than coverage-final.json and only tests existence, so a stale artifact from a previous run passes it.
Every open entry in the tracker except #15, which is deferred by decision. Each fix is pinned by a test, and every status in DEFECTS.md now records what was decided and why. FIXES #1 chat-window/attachment — a filename with no dot (README, Dockerfile) returned a null extension, getWithDefault asserted on it, and the component threw during render. Guarded the way file-icon.js already did. #2 overlay/header — @titleEllipsis opts an open overlay into the truncation that only minimizing used to trigger, and @titleEllipsisLength sets the threshold the useEllipsis getter has always encoded (default 15). A minimized overlay still truncates regardless, so no existing call site changes. The misspelled internal getter is now titleWithEllipsis. #3 report-builder/condition-value — boolean columns got a free-text field because the template had no boolean arm. Added a True/False radio group using the addon's own <RadioButton>, normalising values that round-trip as 'true'/1 and giving each editor its own group name so two boolean conditions cannot clear each other. #4 layout/sidebar — see below. #5 layout/resource/panel — the template wired @onToggle to an action that did not exist. Defined it, forwarding through contextComponentCallback like onOpen and onClose. #6 chat-tray — the unread badge summed only the channels currently loaded. getUnreadCount now runs after countUnread on load and reload so the server total wins. Restartable, so a slow earlier response cannot overwrite a newer one, and a failure leaves the summed count rather than blanking the badge. #7 metadata-editor — the template read @Label directly, so the getter's 'Metadata' default never applied. It reads this.label now. Note that ?? is nullish-coalescing, so @Label="" is how you opt out of the heading. #8 countdown — restartCountdown was unreachable. It is an @action now and both end callbacks receive { restartFn }, so a consumer can write handleEnd({ restartFn }) { restartFn(); }. #11 filters-picker — #rebuildFilters(onColumn) was called with no argument at all three call sites, so the guard ran and the callback never did. It reads this.args.onColumn, which is the hook it was written for. #14 template-builder/properties-panel — dropped value="target.value", which {{fn}} ignores ({{action}} is what understands it). #9, #10, #12 — deleted, as decided: an orphaned duplicate of ActionItem's onClick, a superseded page-list getter, and a second country-select change action. Also removed the arrayRange import #10 orphaned. #4 — AND WHY THE FIRST TWO ATTEMPTS WERE WRONG sidebar.js moved the suite total by ±2 statements between identical runs. The racing statements are NOT the requestAnimationFrame callback, which runs reliably — they are the cancel branches in flushResizeFrame() and teardown(), reached only when a frame is still pending, which depended on whether the browser painted first. My first fix awaited the frame before releasing the gutter. That made it strictly worse: it guaranteed nothing was pending by the time stopResize ran, permanently closing the only path to those lines. The artifact is what caught it — the mechanism I had reasoned my way to matched the ±2 signature exactly and was still the wrong one. The fix is two tests that dispatch synchronously, with no await between the events, so no paint can intervene. sidebar.js: 199/215 -> 203/215, and the final run reports those four lines covered rather than sometimes-covered. #16 — DETECTION, NOT A CURE scripts/stamp-coverage-run.js clears coverage/ and stamps the run start; test:coverage runs it first. The gate now rejects a missing stamp, an unreadable one, an artifact older than the run, and a coverage-final.json that was never written — before reading a single percentage. Self-test: 10 cases to 17, including the dangerous one, a green suite that leaves the previous artifact in place. The cause is untouched. 7 of 9 fast filtered runs this session produced no coverage/ directory at all, while every full run produced one — consistent with the addon POSTing coverage at test end and a short run tearing down first. That makes it a local-development tax rather than a CI risk. DEFECTS #16 records the unstarted next step and retracts two things I claimed earlier: `rm -rf coverage` does not make collection reliable, and the Node 18 ESM failure cannot affect CI, which pins Node 22. #15 is deferred with a written-down list of what it needs first, including reconciling it with the __queries__ variable route that already solves the same problem. THREE OF MY OWN TESTS FAILED BEFORE THIS LANDED Recorded because each was a different kind of mistake and none was visible by reading: - a stale expectation: FontAwesome 6 renders the file-alt alias as file-lines, so the component was right and the assertion was wrong; - a harness bug: the countdown stub only captures the first 1000ms interval, so a restart fell through to the real timer and leaked a live interval into the remaining ~4,700 tests; - a wrong assumption: Overlay reports a toggle only from toggle(); open() and close() each report their own event, which is why the context exposes toggle separately. One assertion of mine was also replaced for being unable to fail.
Member
Author
Confirming run — clean
One correction to the PR body: it says branches 93.12% (5825/6255). The clean run reads
The files this PR touchedFour files reached 100% outright. The rest still carry gaps that predate this work — |
Member
Author
Second full run — identical, which is the real proof for #4A second independent full run finished after the one above. Every figure matches:
This is worth more than the per-file number on its own. #4 was defined by two full runs Which matters here specifically, because the mechanism reasoning was wrong twice: first blaming the |
roncodes
added a commit
that referenced
this pull request
Aug 25, 2026
…(DEFECTS #16) The cause, at last, and it is a known upstream bug rather than anything odd about this repo. sendCoverage() POSTs ~1.9 MB to /write-coverage — 401 instrumented files, because a per-file 100% gate needs forceModulesToBeLoaded() to evaluate everything so untested files stay in the denominator. In CI mode testem tears the browser down the moment QUnit reports the run finished, truncating the upload mid-body. raw-body aborts, coverageHandler is never reached, and nothing is written. That is why it looked like flakiness rather than a bug: it depends on how long the run took. After 139 tests testem has almost nothing to serialize and kills the browser in milliseconds; after 5130 tests, emitting the results buys the upload enough time to land. Measured 2 of 9 artifacts on fast filtered runs against 100% on full runs, with `BadRequestError: request aborted` correlating 1:1 with every failure. Testem.afterTests hands testem a callback it WAITS for, so the upload finishes before teardown. It does not fire under --server, hence the branch on config.APP.isRunningWithServerArgs. Upstream, all with the identical raw-body:245 stack: ember-cli-code-coverage#420 (Aug 2024), #421, testem#1577 Our measurements are added to #420. Measured: 2 of 2 fast filtered runs now produce an artifact with no abort. Full suite unchanged — 5130 pass, 0 fail, lint 0. Ruled out and recorded so nobody retries them: keepalive:true and sendBeacon (the Fetch spec caps both at 64 KiB, ~30x under our payload), and moving forceModulesToBeLoaded() to QUnit.begin (0 of 3, and against the guidance in the addon's own source, which says to call it after the suite). Also recorded a debugging trap: instrumenting coverageHandler shows nothing on a failing run, which reads as "the POST never arrived". It does arrive and dies inside bodyParser, before the handler. I drew the wrong conclusion from that and had to retract it; instrument in front of bodyParser, not behind it. The freshness gate from PR #163 stays. It is what made this failure loud instead of silent, and it is still the guard against reading a stale report. DEFECTS #18 opened: branch totals still vary by +/-1 between identical runs (5825 vs 5826 across three otherwise-identical full runs). Same class of gate blocker #4 was, culprit not yet located.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Works through every open entry in
DEFECTS.mdexcept #15, which is deferred by decision. Each fix is pinned by a test, and every status in the tracker now records what was decided and why.The fixes
chat-window/attachmentREADME,Dockerfile) produced a null extension,getWithDefaultasserted on it, and the component threw during render. Guarded the wayfile-icon.jsalready did.overlay/header@titleEllipsisopts an open overlay into the truncation only minimizing used to trigger;@titleEllipsisLengthsets the threshold theuseEllipsisgetter always encoded (default 15).report-builder/condition-valuelayout/sidebarlayout/resource/panel@onToggleto an action that did not exist.chat-traymetadata-editor@labeldirectly, so the getter's'Metadata'default never applied.countdownrestartCountdownwas unreachable; both end callbacks now receive{ restartFn }.filters-picker#rebuildFilters(onColumn)was called with no argument at all three call sites.template-builder/properties-panelvalue="target.value", which{{fn}}ignores.Details worth a second look:
titleWithEllipsis.'true'or1, and gives each rendered editor its own radio group name viaguidFor, so two boolean conditions in one report cannot clear each other.restartable, so a slow earlier response cannot overwrite a newer one, and a failure leaves the summed count in place rather than blanking the badge. Neither was in the brief; shipping without them would have traded a stale badge for a broken tray.??is nullish-coalescing, so@label=""is now how you opt out of the heading, since omitting the argument no longer does.#4, and why my first two attempts were wrong
sidebar.jsmoved the suite total by ±2 statements between identical runs.The racing statements are not the
requestAnimationFramecallback — that runs reliably. They are the cancel branches influshResizeFrame()andteardown(), reached only when a frame is still pending, which depended on whether the browser painted first.My first fix awaited the frame before releasing the gutter. That made it strictly worse: it guaranteed nothing was pending by the time
stopResizeran, permanently closing the only path to those lines. The artifact caught it — the mechanism I had reasoned my way to matched the ±2 signature exactly and was still the wrong one.The fix is two tests that dispatch synchronously, with no
awaitbetween the events, so no paint can intervene. That idiom was already in use a few tests earlier in the same file.sidebar.js: 199/215 → 203/215, with those four lines reported covered rather than sometimes-covered.#16 — detection, not a cure
scripts/stamp-coverage-run.jsclearscoverage/and stamps the run start;test:coverageruns it first. The gate now rejects a missing stamp, an unreadable one, an artifact older than the run, and acoverage-final.jsonthat was never written — before reading a single percentage. The self-test grew from 10 cases to 17, including the dangerous one: a green suite that leaves the previous artifact in place.The cause is untouched. 7 of 9 fast filtered runs this session produced no
coverage/directory at all, while every full run produced one — consistent with the addon POSTing coverage at test end and a short run tearing down first. That makes it a local-development tax rather than a CI risk, since CI runs the full suite.The entry also retracts two things I claimed earlier in the session:
rm -rf coveragedoes not make collection reliable (the 7-of-9 figure is with it), and the Node 18 ESM failure cannot affect CI, which pinsNODE_VERSION: 22.x.#15 — deferred
Confirmed as intended behaviour (fetch from a url with params) and left for its own session rather than half-built. The entry lists what it needs first: an endpoint contract, an auth story, loading and error states, a shape for
query_response_path, something on the render side that consumes a query-backed table, and reconciliation with the__queries__variable route that already solves the same problem.The branch stays uncovered rather than suppressed — an
istanbul ignorehere would sit on the openingif, andignore elsethere also swallows thevariablebranch, which real tests cover.Three of my own tests failed before this landed
Recorded because each was a different kind of mistake and none was visible by reading:
file-altalias under its canonical namefile-lines— the component was right, the assertion was wrong.setIntervalstub captures only the first 1000 ms interval, so a restart fell through to the real timer and leaked a live interval into the remaining ~4,700 tests.Overlayreports a toggle only fromtoggle();open()andclose()each report their own event, which is exactly why the panel's context exposestoggleseparately.One assertion of mine was also replaced for being unable to fail (
first.active ? first.value : undefinedcompares a value to itself when active).Verification
Last complete run before this commit: 5130 tests, 5129 pass, the single failure being this batch's own panel test, fixed here. Coverage 96.13% statements, 93.12% branches, 98.60% functions, 96.37% lines, up from 95.79 / 92.73 / 98.15 / 96.02.
rm -f .eslintcache && pnpm run lintexits 0.A final full run confirming the panel fix is in progress; I will post its numbers on this PR rather than leave these as the last word.