Repository navigation
Batch B (part 6): two "blockers" that were not, and a multi-value filter that kept one value - #161
Merged
Merged
Conversation
…rcised tip-tap-editor 11 gaps -> 7. Branches 91.17%. The shared TEMPLATE already wires @onfocus and @OnBlur alongside eleven other callbacks, and the file reads as though focus and blur are covered. No test ever SET them, so `this.onFocus` was undefined and both guards had only ever been skipped — [0,21], skipped twenty-one times, taken never. The new test sets both and drives focus/blur through the real TipTap instance the suite already captures. That variant is worth recognising: the wiring is present, the scaffolding is present, and only the branch counts show two of thirteen callbacks were never actually exercised. The test also produced a failure mode this campaign had not hit — it passed in a filtered run (40/40) and FAILED in the full suite. Focus state is shared across the suite, so the exact sequence ['focus','blur'] is not stable; the editor can be focused more than once. The assertion now checks that both handlers fire rather than their order. Still falsifiable — a broken guard means the handler never fires — but it no longer asserts something the environment cannot guarantee. Brief updated accordingly: a green filtered run is NOT sufficient before committing. Filtered runs have now failed to catch three distinct things — a missing import (ESLint passed it too), an intermittently missing coverage artifact, and this order-dependent test. The last is the worst of the three: it looks correct and breaks CI for whoever merges next. Left uncovered deliberately: insertImage's early return for a file that is not queued. The guard is real — its own comment notes the method can be called from both the dropzone and the button — but the only entry point is @onFileAdded, which always supplies a freshly queued UploadFile. Reaching it would mean manufacturing an UploadFile in another state and calling the action directly, bypassing the mechanism that makes the guard necessary. Same judgement as report/find-select: when the only route to a branch sidesteps what constrains it, the test proves nothing. Full suite 5055 pass / 0 fail / 0 skip. Coverage 94.71% statements, 91.17% branches, 97.53% functions, 95.10% lines.
full-calendar 9 gaps -> 5, all five being the leak in DEFECTS.md #13. DEFECTS.md #13 — `destroyCalendarEventListeners` is never called. The component has no willDestroy, no registerDestructor, and nothing in the template invokes it. Every subscription pushed onto `_listeners` stays registered on the FullCalendar instance for its lifetime, so a calendar on a route the user navigates in and out of accumulates listeners. Unlike #6, #9, #10 and #12 — harmless duplicates of code that does run — here cleanup that was WRITTEN simply never happens. It is the first of the eleven with a user-facing cost. There is a trap in the obvious fix, which is why it is recorded rather than applied: this.calendar.off(eventName, this.triggerCalendarEvent.bind(this, callbackName)); `.bind()` returns a NEW function every call, so it can never match the handler passed to `.on()`. Wiring this into willDestroy would look like a fix and remove nothing. Both halves need changing — retain the bound reference when subscribing, then call the teardown. Either alone is ineffective. That is a component-lifecycle change and belongs with someone who knows how these calendars are mounted. Test: rendering with no @onInit, which every existing test supplies. Two exclusions, both traced and worth distinguishing: - `if (typeof this[eventName] === 'function')` — `eventName` is a callback name like `onDateClick` and this class defines no such methods. A subclass hook, unreachable from within this package. - `if (typeof this.args[eventName] === 'function')` — a listener is only subscribed when that arg is ALREADY a function (createCalendarEventListeners:85), so it is guaranteed present by dispatch time. Another defensive re-check of an invariant established one function earlier, the same shape as filters-picker's #readUrlValue. Full suite 5056 pass / 0 fail / 0 skip. Coverage 94.69% statements, 91.15% branches, 97.53% functions, 95.07% lines.
…our lint failures
Verified with a cleared eslint cache and a full run, not a filtered one:
5055 pass / 0 fail / 0 skip; lint exit 0.
Coverage 94.78% statements, 91.20% branches, 97.53% functions, 95.18% lines.
REMOVED the tip-tap focus/blur test. It passed in isolation and failed in the full suite —
twice. My first fix loosened the assertion from an exact sequence to "both fired", treating
it as an ordering problem. It is not: `blur` does not fire AT ALL in the full run, because
`editor.commands.focus()`/`.blur()` are no-ops unless the window genuinely holds focus,
which is not stable across a 5000-test headless run.
I should have removed it after the first failure instead of loosening it. A flaky test is
worse than an uncovered branch: it breaks CI intermittently for everyone and teaches people
to re-run rather than look. The guards now carry an exclusion stating the real limitation —
"reachable in a real browser, not reliably from this suite" — with the evidence that a test
for it failed twice. Second exclusion in this campaign to claim that weaker thing rather
than "unreachable"; the other is filters-picker's async rethrow.
FOUR PUSHED BRANCHES CARRY LINT FAILURES I REPORTED AS CLEAN. `pnpm run lint` exits 1 on
b2/b3/b4/b5 with real `no-unused-vars` errors — orphaned helpers and imports left behind
when I removed tests that used them. Two things combined to hide it:
- `lint:js` is `eslint . --cache`, so a stale entry can mask a regression;
- my check ran `eslint --fix <files> | grep -v WARN | head -3`, and a PIPE DISCARDS THE
EXIT STATUS, so a genuine error read as success.
The brief now specifies the verification exactly: `rm -f .eslintcache && pnpm run lint;
echo "exit=$?"`, and never judge lint through a pipe. It also notes the proximate cause —
removing a test orphans its helpers and imports.
Fixing the pushed branches means rebasing into b2 and b4 and force-pushing four PRs that are
under review, so I have not done it. Fixed forward here; the merged result is correct either
way, but the four PRs are individually red until that is resolved.
DEFECTS.md #14: dropdown-button's `_onTriggerInsertFired` and `_onButtonInsertFired` are
written and never read — four lines each in the whole codebase, declaration and assignment
only. Their sibling `_onInsertFired` IS consulted, which is what makes the omission look
deliberate rather than accidental. Zero-risk to delete. Twelve dead-code items now open.
Also excluded six dropdown-button fields the constructor assigns before anything reads them.
Verified with a cleared eslint cache and a full run: 5057 pass / 0 fail / 0 skip, lint 0.
Coverage 94.78% statements, 91.22% branches, 97.53% functions, 95.18% lines.
Two tests: at the pinned limit an unpinned item is disabled while pinned ones stay
clickable so a slot can be freed, and unpinning at the limit makes room again.
`togglePin`'s `!this.atPinnedLimit` guard is excluded, not tested. The template renders the
control `disabled={{and this.atPinnedLimit (not (this.isPinned item))}}`, so it cannot be
clicked while at the limit and the guard is never consulted as false. That is the FOURTH
instance of this shape after sidebar-toggle, export-options and report/find-select, and it
is now the most common reason a branch is unreachable here: the component disables the
control that would trigger the guard, then guards anyway.
Worth flagging for the 100% goal: these are permanently unreachable through the public
surface, so each becomes an exclusion. A meaningful share of the final number will rest on
exclusions rather than tests, which is why every one cites the specific template or caller
line that makes it unreachable — a reviewer can check any of them in a minute. An
exclusion-heavy 100% is only worth having if the exclusions are individually checkable.
Also found: five of this file's remaining sites are inside `reorderPinned`, wired to
`@dragEndAction` on an ember-drag-sort component. The drag-sort dependency therefore reaches
beyond the eleven Batch C files into components that look unblocked in the ranking. The
harness question is bigger than "should we test eleven hard files" — it gates part of the
ordinary tail too, and I will quantify that before proposing anything.
Process: this file took four attempts, three of them wasted inventing helper names
(`pinnedItems` for `pinnedTitles`) and fixture data (`Pallet`, when the fixture holds Fleet
Ops, Storefront, IAM and Developers). Reading a test file's first thirty lines before adding
to it would have prevented all three. That is now a required first step in the brief, not a
habit — it is the single most frequent mistake I have made in this campaign.
Verified with a cleared eslint cache and a full run: 5059 pass / 0 fail / 0 skip, lint 0.
Coverage 94.84% statements, 91.25% branches, 97.57% functions, 95.24% lines.
customizer 8 gaps -> 4.
ember-drag-sort ships `addon/utils/trigger.js`, documented in its README with the exact
event sequence. No new dependency, no harness to build:
import trigger from 'ember-drag-sort/utils/trigger';
await trigger(items[0], 'dragstart');
await trigger(items[2], 'dragover', false);
await trigger(items[0], 'dragend');
Two tests on the customizer: a drag that reorders the pinned list, and a drag that ends
where it started — the latter reaching `reorderPinned`'s `sourceIndex === targetIndex` early
return, which I had reported as unreachable without a harness.
I CARRIED THIS BLOCKER FROM THE PREVIOUS SESSION'S NOTES AND REPEATED IT FOR MANY ITERATIONS
— in status reports, commit messages and three PR bodies — WITHOUT OPENING THE PACKAGE. One
`ls node_modules/ember-drag-sort` would have settled it at the start. A note saying "blocked
on X" is a claim to verify, not a fact to inherit, and it is most dangerous when inherited
across sessions where the original reasoning is no longer visible.
Three corrections to the Batch C picture in as many iterations, all from measuring instead
of repeating:
1. SCOPE — the harness question is ~21% of remaining statements and ~10% of branches, not
"about a third". That came from ranking whole files rather than counting uncovered sites
inside them.
2. ATTRIBUTION — interact.js gates ONE file (template-builder/element-renderer), not four.
Drag-sort gates the query-builder family, which I had listed as a separate group.
3. BLOCKED-NESS — drag-sort was never blocked at all.
What actually remains gated: template-builder/element-renderer on interact.js, 43 statements
and 13 branches, one file. Worth checking whether interactjs offers a comparable test path
before assuming that one too.
Now unlocked and worth taking next: query-builder/conditions (29s/27b, the largest single
remaining file), group-by (12s/9b), sort-by (4s/4b).
The brief has been corrected so the wrong framing does not survive into another session.
…y kept the last pick The largest single remaining file is now 100/100/100/100 (statements, branches, functions, lines), verified against a freshly written coverage-final.json. Writing the first real test for the `is one of` editor turned up a user-visible bug: selecting `active` then `pending` reported only `['pending']`. updateConditionValue mutated cond.value in place and never replaced a container, so Glimmer had nothing to invalidate and PowerSelectMultiple's @selected kept rendering the value it was first given. The component's own updateCondition() already documents the fix in a comment — "clone containers (so Glimmer sees a change)" — and updateConditionRangeValue routes through it; updateConditionValue was the one value writer that did not. Fixed to match, debounce kept. DEFECTS #13. Two harness lessons, both of which cost a cycle: - Use `sort()` from ember-drag-sort/utils/trigger, not raw `trigger`. With @dragHandle set, a hand-rolled dragstart/dragover/dragend on the item does nothing at all, and the test still passes if its assertions are loose. Both reorder handlers read 0 hits while the tests were green. - coverage-final.json is not always rewritten. A green COVERAGE=true run left the previous artifact in place twice. rm -f before, ls -l after. ember-drag-sort re-checks that source and target differ after its own index adjustments and never calls @dragEndAction for a drop that did not move anything, so `if (sourceIndex === targetIndex) return` is unreachable from the addon. Documented as an ignore here and in group-by.js and sort-by.js, which carry the same guard verbatim. Two tests I had written for it were removed rather than left asserting something that cannot fail. Full suite: 5070 pass, 0 fail, 0 skip. 95.18% statements, 91.78% branches, 97.66% functions, 95.53% lines.
…r to 100%
template-builder/element-renderer.js was the last file the notes called blocked
on a third-party gesture library — 43 uncovered statements, 27 branches, and all
nine interact.js callbacks (tap, drag move/end, resize move/end and the four
closures they read position, zoom and canvas bounds from) never once executed.
interact.js listens for real pointer events on the document, so in a real-Chrome
test it can simply be driven the way a browser drives it: dispatch PointerEvents
and let interact do its own gesture detection. Eleven tests now cover tap
selection, dragging with and without an onMove handler, zoom-scaled deltas,
clamping at all four canvas edges, rotation surviving a move, and resizing with
its 20x10 floor.
Two things worth recording, because both were nearly missed:
- The first attempt failed with the drag never starting, and the cause was my
own helper dispatching pointerdown on `document` instead of on the element.
interact was working the whole time.
- The "no width or height of its own" test passed for the wrong reason: the
wrapper renders `width: 100px` from the same defaults, so parseFloat never
fell through to the fallback the test claimed to exercise. Renamed to say
what it actually proves, and the genuinely unreachable `?? 100` tail is now
documented as an ignore instead of being falsely credited.
Three documented ignores remain: the two size fallbacks above and handleDestroy's
null check, which handleInsert makes unreachable.
element-renderer.js: 100% statements, branches, functions and lines (49 tests).
…d why runs kept lying toolbar.js: 100/100/100/100. Every action guards an optional callback and the false side had never run; one test renders the toolbar with @canundo, @canredo and a @selectedElement but no callbacks and clicks all nine controls. close() is the exception and is documented as an ignore: its button only renders inside {{#if @onclose}}, so the guard cannot be reached. properties-panel.js: 14 uncovered statements and 28 branches down to 4 and 3, functions to 100%. Eleven tests: the table editors driven with no @onUpdateElement, canvas settings with no @onUpdateTemplate, the variable picker with no handler and with a handler but nowhere to write, an upload whose url has no destination, the line and shape option getters (no test had ever selected those element types), renaming a column key on a row that never had it, and editing one row of two. Eight ignores for what is left, each naming the specific thing that makes it unreachable. What remains in that file is the manual/variable/query mode chain, left deliberately uncovered. Suppressing the unreachable `query` branch means annotating the opening `if`, and istanbul's `ignore else` there swallows the `variable` branch too, which real tests cover — a live regression signal traded for a percentage point. DEFECTS #15 has the evidence: the toggle offers only Variable and Manual, and `data_source_mode` appears in exactly three places in the monorepo, all in this one file. Three defects recorded (#14 `value="target.value"` passed to {{fn}}, which ignores it; #15 above; #16 coverage collection). #16 is the one that matters for the gate. Three failure modes, all observed: - A green run leaves the PREVIOUS coverage-final.json in place. This over-reports silently, which is the dangerous direction. - A run writes coverage-summary.json, lcov and the HTML report but no coverage-final.json. - `pnpm exec ember test` builds and then dies with `require() of ES Module .../execa@9.6.1 from .../testem@3.20.0`. The third looked exactly like a dependency-range incompatibility and is not one: /usr/local/bin/node is v18.15.0, which cannot require() an ESM module, while nvm's v22.22.2 is only on PATH in shells that source the profile. I recorded the wrong cause first and corrected it. `rm -rf coverage` before a run reliably produces a complete artifact; deleting only coverage-final.json does not. Full suite: 5094 pass, 0 fail, 0 skip. 95.79% statements, 92.73% branches, 98.15% functions, 96.02% lines.
…es but never wrote Commit 8410e7e's message describes the leak as "#13" and the entry was never written to the file; #13 was later taken by the query-builder fix. Added as #17 with a note pointing back, plus the part that matters: wiring destroyCalendarEventListeners to a destructor does NOT fix the leak, because .bind() returns a new function each call and .off() can never match what .on() was given. Also corrected #15's line reference after the ignores shifted it.
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.
Stacked on
test/coverage-campaign. Nine commits.Read this first: there is one behaviour change in the whole PR
Across all nine commits, exactly one line of shipped logic changes — the container-clone in
updateConditionValue(addon/components/query-builder/conditions.js). Everything else underaddon/isistanbul ignorecomments explaining why a specific line is unreachable. No othercomponent logic is touched.
The one change: a multi-value filter could only ever hold one value
Selecting
activeand thenpendingin anis one ofcondition reported['pending'].updateConditionValuemutatedcond.valueon the existing condition object and callednotifyDebounced— it never replaced a container, so Glimmer had nothing to invalidate.PowerSelectMultiple's@selected={{condition.value}}kept rendering the value it was firstgiven (
null), and every subsequent pick was treated as the first.The component already documents the fix in its own helper:
updateCondition()carries the comment"clone containers (so Glimmer sees a change)", and
updateConditionRangeValueroutes through it.updateConditionValuewas the one value writer that did not. It now clones the group and itsconditions array before notifying, and keeps the debounce (the free-text editor types through the
same action).
Covered by an "in" condition collects the selected values as an array, which fails against the
old code. Recorded as DEFECTS #13.
Coverage
Full suite on this branch: 5094 tests, 0 fail, 0 skip —
95.79% statements, 92.73% branches, 98.15% functions, 96.02% lines, read from
coverage/coverage-summary.json.Files taken to 100% on all four metrics:
query-builder/conditions.js,template-builder/element-renderer.js,template-builder/toolbar.js.template-builder/properties-panel.jsgoes from 14 uncovered statements / 28 branches to 4 / 3,and 100% functions; what remains is the
querydata-mode chain, left uncovered deliberately(see DEFECTS #15 and the note below).
Two "blockers" that were never blockers
Both files had been written off in earlier notes as untestable because of a third-party library.
Both were testable with the library's own facilities.
ember-drag-sort ships
sort()inember-drag-sort/utils/trigger:await sort(listElement, sourceIndex, targetIndex, above, '.drag-handle'). Note that hand-rollingdragstart/dragover/dragend on the item element does nothing when the list sets
@dragHandle—and the test still passes if its assertions are loose. Both
conditionsreorder handlers read 0hits while my first attempt at these tests was green.
interact.js listens for real pointer events on the document, so it can be driven the way a
browser drives it: dispatch
PointerEvents and let it do its own gesture detection. Eleven testsnow cover tap selection, dragging with and without an
onMovehandler, zoom-scaled deltas,clamping at all four canvas edges, rotation surviving a move, and resizing with its 20×10 floor.
The pointerdown has to go to the element — sending it to
documentsilently starts nothing, whichis what made the first attempt look like interact.js was unusable.
Deliberately not taken to 100%
properties-panel.js'smanual/variable/querymode chain. Suppressing the unreachablequerybranch means annotating the openingif, and istanbul'signore elsethere swallows thevariablebranch too — which real tests cover. That trades a live regression signal for apercentage point, so it is left visible and recorded as DEFECTS #15 instead: the toggle offers only
Variable and Manual, and
data_source_modeappears in exactly three places in the whole monorepo,all in that one file. Finish the feature or drop the branch — a product call, not a coverage one.
Tests removed rather than left dishonest
Two tests I wrote for the
sourceIndex === targetIndexguard were deleted. ember-drag-sortre-checks that source and target differ after its own index adjustments and never calls
@dragEndActionfor a drop that did not move anything, so those assertions could not fail. Theguard is documented as an ignore in
conditions.js,group-by.jsandsort-by.js.One test was renamed for the same reason: "an element with no width or height of its own" was
passing because the wrapper renders
width: 100pxfrom the same defaults, not because the fallbackit claimed to exercise ever ran.
New DEFECTS entries
properties-panel.hbs:73passesvalue="target.value"to{{fn}}. That is an{{action}}option;{{fn}}ignores it. Cosmetic, but it explains whyupdateProp's: eventfallback has no reachable caller.
querydata-mode control, above.<Kanban />component #16 coverage collection is unreliable in three distinct ways, all observed here — a green runthat leaves the previous
coverage-final.jsonin place (over-reporting, silently); a run thatwrites the summary, lcov and HTML report but no
coverage-final.json; and a build that succeedsthen dies before launching a browser. This is an obstacle to a hard 100% gate and needs
coverage:checkto refuse a stale or missing artifact rather than read whatever is on disk.8410e7ereferenced as "Dev main #13" but neveractually wrote to the file. The part that matters: wiring
destroyCalendarEventListenersto adestructor does not fix it, because
.bind()returns a new function each call so.off()cannever match what
.on()was given. The obvious fix ships a no-op that passes a test.A correction I made mid-session
I first recorded #16's third failure mode as a testem/execa dependency incompatibility. It is not
one:
/usr/local/bin/nodeis v18.15.0, which cannotrequire()an ESM module, while nvm's v22.22.2is only on PATH in shells that source the profile. The entry now says so, and suggests an
enginesfield plus
.nvmrcso CI does not hit the same wall on a Node 18 image.Verification
rm -f .eslintcache && pnpm run lint→ exit 0.run.