Repository navigation
Add verified test coverage, coverage gating in CI, and Codecov upload - #143
Merged
Merged
Conversation
Brings the addon from a suite that could not complete to 4968 passing
tests at 92.02% statements / 87.50% branches / 94.81% functions /
92.48% lines, with coverage measured on every CI run and uploaded to
Codecov so it cannot silently regress.
Test suite
- 401 test files covering every addon component, service, helper,
modifier and util except schedule-calendar (dead code, see PR notes).
- Replaced 210 generated blueprint stubs with real tests.
- Zero skips, no must-stay-failing pins.
Coverage tooling
- ember-cli-code-coverage wired up behind COVERAGE=true.
- scripts/check-coverage.js enforces per-file 100% and fails on any file
missing from the report; scripts/check-coverage-test.js self-tests it
(9 cases) so the gate itself is covered.
- codecov.yml at 100% project/patch scoped to addon/; CI uploads
coverage/lcov.info via codecov/codecov-action@v5 under the ember-ui flag.
Production fixes needed to make the suite runnable or to close a defect
the tests exposed. Notably: kanban/column read its drop position after
resetting it, so a card dropped between two cards always landed at the
end; thirteen unguarded {{on}}/{{fn}} handler bindings across five
components crashed at render time when the argument was absent.
Deleted addon/components/availability-editor.js (no template, no
re-export, no consumer). Added app/components/schedule-item-card.js, the
only component of 220 missing its app-tree re-export.
Remaining uncovered code is documented site by site in the PR comments;
no istanbul ignore comments were added.
Each fix is pinned by a test that fails against the previous code. Three tests that had been written to pin the buggy behaviour are updated here to assert the corrected behaviour instead. - transition-to: prefixMountPoint asserted the same condition as the `if` wrapping it, so the guard could never fire and a non-string route name was interpolated into the route. Asserts `=== 'string'` now. - resource-context-panel: open() dereferenced `definition` before calling #validateDefinition, so a missing definition died with a TypeError and the intended "Overlay definition is required" error was unreachable. - set-height / set-max-height: a value with no numeric part (`auto`, `fit-content`) was reduced to the invalid string "px" and dropped by the browser, and a unit the modifier cannot convert (`%`, `vh`, `ch`) was parsed off and discarded, so `100%` silently became `100px`. Both are now applied verbatim. - services/leaflet: `initialized` is assigned in exactly one place, inside the branch that only runs when `instance` is undefined — so a host that set an instance before load() left the polling interval running forever. - custom-field/input: removed the isMoneyInput arm. <MoneyInput> reports `onChange(storedValue, detail)` with a number, so the arm's isObject() test could never pass, and it would have reported the formatted value where the raw arm correctly reports stored cents. - query-builder/column-select: the alias field was a two-way <Input @value>, whose write-back landed after updateAlias and undid its trimming, mutating the aliases hash already handed to onChange. It is one-way now, committed on change. - query-builder/actions: the three handlers passed `this.queryObject`, which does not exist on that component, so a standalone consumer always received undefined. Reads `this.args.queryObject` now. - is-menu-item-active: removed the `slugOnly && view` branch, a contradiction since slugOnly already requires `view === null`. 4973 tests pass, 0 skips. Coverage 92.08% statements / 87.60% branches / 94.81% functions / 92.55% lines; the branch total drops from 6714 to 6704 as the dead branches go away.
Group C of the defect triage. Each fix is pinned by tests that could not
be written before it — three of them were written during the coverage
work, failed against the production code, and had to be deleted.
- filter/multi-option: `search` now RETURNS its matches, as power-select
expects. The remote path called `this.fetchOptions(...)`, a Task object
rather than a function, and threw; the local path assigned
`this.options` from inside a modifier update, which raised a
backtracking assertion and permanently discarded every non-matching
option, so clearing the query could not bring them back. The four tests
deleted for this are restored, including one asserting that clearing
the query restores the full list.
- overlay: the resize clamps tested WIDTH whatever the position and
returned before the horizontal/vertical fork, so a top/bottom overlay
whose width fell outside [min, max] — which a full-width drawer always
does — could never be resized, and each vertical drag silently rewrote
its width to the clamp. Clamps the dimension being dragged now, with
minResizeHeight/maxResizeHeight alongside the width pair.
- modal: `@usesTransition('_fade')` named a getter, but the decorator
reads `this.args[prop]`, so `this.args._fade` was always undefined and
`@fade={{false}}` never disabled the transitions. Names the argument.
- query-builder sort-by / group-by / conditions: the three `validate*`
actions existed but nothing called them, so narrowing the selected
columns left the panel sorting, grouping and filtering by columns that
were no longer selected. Wired to `{{did-update}}` on the column list.
- attach/popover: removed the inert `isOffset` flag. It was never
assigned from an argument or anywhere else, and the method it guarded,
`isCursorBetweenTargetAndAttachment`, does not exist on the component —
so had anything ever set it, every mousemove would have thrown.
Left for a product decision, unchanged here: model-select's infinite
scroll (#105) needs the options to come from ember-infinity, which is
feature work, not a repair; layout/resource/panel's redundant save path
(#98) duplicates a button panel/header-actions already renders.
4983 tests pass, 0 skips. Coverage 92.44% statements / 87.97% branches /
95.29% functions / 92.89% lines.
Fix eight contained defects found during the coverage work
Fix five behavioural defects; two need a product decision
…urce
Removes the members and files confirmed unreachable during the coverage
work. Four whole components go, along with 26 members across ten files.
Whole components (no template, no re-export, or no consumer anywhere in
the monorepo — verified by sweeping every package outside ember-ui):
- schedule-calendar and schedule-item-card
- custom-field/form
- report-builder/results-table
Members:
- query-builder: toggleQueryPreview, showQueryPreview, exportQuery
- query-builder/conditions: conditionsMessage, canAddConditions
- event-calendar: changeView, today, refetchEvents, refetchResources
- template-builder/properties-panel: the query-parameter editor
- smart-nav-menu: reorderPinned (passed as @onReorder to a customizer
that ignores it; the customizer's own copy is wired and stays)
- smart-nav-menu/customizer: unpinnedItems
- dashboard/widget-panel: hoveredWidget, onHover, onUnhover
- custom-field/yield: toggleGroupEdit, resolveSubject
- custom-field/options-input: addMetaOption
- layout/resource/panel: save, saveButtonText, controller, onTabChange,
onPressEdit, onViewDetails, and the now-orphaned resourceName and
resourceType. The live save button is header-actions', driven by the
caller's @saveTask; these were a second, unreachable implementation.
KEPT after checking: query-builder's `get columns()`. It was listed as
dead, but conditions.hbs gates its entire editor on `{{#if @columns}}` —
deleting it would blank that panel. Only the four bindings that group-by
and sort-by ignore are removed.
Also scopes the coverage gate to this package. A pnpm workspace link
(@fleetbase/ember-core) is instrumented by the same build and appeared in
the report as `../ember-core/...`, adding 561 failure lines and dragging
the reported global from 93.66% to 67.42%. check-coverage.js now skips
files outside the package and recomputes the global from first-party
entries rather than trusting istanbul's total. Self-test covers it (10
cases, up from 9).
4941 tests pass, 0 skips. First-party coverage 93.66% statements /
89.14% branches / 97.18% functions / 94.06% lines.
Both tooling recommendations from the defect review.
eslint-plugin-qunit, recommended config, scoped to tests/**/*-test.js.
`no-hooks-from-ancestor-modules` is the one that motivated it: a nested
`module('…', function (hooks) { … })` shadows the outer hooks, QUnit 3
turns that into a hard error, and the suite stays green when you do it.
The suite is already clean on that rule; the other 58 findings were
fixed rather than silenced, except `require-expect`, which is disabled
as noisy for a suite whose assertion counts are obvious.
Notable among those fixes: nine assertions hedged with `||` or `&&`,
which hid what the code actually does. Splitting them exposed four
tests asserting something untrue — the tab-navigation container carries
no `pills` class, element-renderer's style attribute is the raw authored
hex rather than a computed rgb(), the dropdown-button tooltip is not
rendered inside the trigger, and the canvas settings panel has no number
inputs at all (those are element-scoped). All four now assert what the
component does. Two more tests could not fail: the sticky-cell DOM test
and a canvas click test each had a branch that never ran.
A repo-local template-lint rule, no-unguarded-handler-argument, flags
`{{on "evt" @arg}}` and `{{fn @arg …}}` in the handler position. These
throw while rendering when the argument is absent, so a component with
one cannot be rendered without it. It accepts the guarded forms this
codebase settled on — `(or @arg (noop))`, `{{#if @arg}}`-wrapped
bindings, and `this.ownAction`.
It found nine unguarded bindings in the addon that the manual sweep
missed, in modals/bulk-action-model, modals/bulk-delete-model,
modals/import-form, modals/save-report, overlay/header (two),
table/cell/link-list and layout/header/dropdown/item (two). All nine are
now guarded. The rule is off for test fixtures, which supply their own
handlers by construction.
.template-lintrc moves to .mjs: ember-template-lint 5 is ESM-only, so a
CommonJS config cannot require the Rule base class.
4940 tests pass, 0 skips. First-party coverage 93.69% statements /
89.22% branches / 97.18% functions / 94.10% lines.
Fixes the backtracking-rerender assertion recorded as DEFECTS.md #26, which the documented one-line fix (two-way to one-way bindings) did not clear — the binding style was the symptom, not the cause. Two things were wrong. Every handler mutated `this.translations` in place and then reassigned the same reference, writing to a tracked property the render was still consuming. And `{{#each-in}}` keyed the rows on the translation KEY, so typing in a key field destroyed the input and rebuilt it on the next render, mid-edit. The editor now holds `{ language: [{ id, key, value }] }` rows. A row's identity survives renaming its key, and every edit builds a new structure and assigns it once, so nothing writes to state that a render is reading. The fields are one-way and commit on `change`. `setDefaultKeys` no longer writes the defaults into the caller's own `@value` — it returns a new object. That was a side effect on the consumer's data, now pinned by a test. The public surface is unchanged: `@value` in, `@onChange(translations)` out, both in the `{ language: { key: value } }` shape, plus `@defaultKeys` and the label arguments. The internal `loadedTranslations` getter is gone; it was only read by this component's own template. Six tests added, covering what the old design could not express: the same input element survives a rename, a key can be renamed twice in a row, a key edit followed by a value edit reports both, rows edit independently, keys are underscored on commit, and the caller's object is never touched. 4946 tests pass, 0 skips. Coverage 93.71% statements / 89.21% branches / 97.18% functions / 94.11% lines.
Drops the prerelease dependency. `ember-radio-button` was pinned to 3.0.0-beta.1 because its last stable release, 2.0.1, references the removed `Ember` global and throws on Ember 5 — and the package has not been updated in four years. Adds `radio-button` and `radio-button-input` to the addon, keeping the component names, arguments and rendered DOM identical to the upstream addon, including the `ember-radio-button` label class that existing stylesheets target. The two call sites — modal/layouts/option-prompt and custom-field/input — are unchanged, and their tests pass untouched. The full surface is preserved: @value, @groupValue, @Changed, @name, @disabled, @required, @autofocus, @tabindex, @radioClass, @radioId, @classnames (string or array), @checkedClass, @ariaLabelledby and @ariaDescribedby; block form wraps the input in a label, inline form renders the input alone. Selection is compared with `isEqual` and reported through `once`, both as upstream, so equality semantics and callback timing are unchanged. radio-button.hbs carries a scoped template-lint disable for no-autofocus-attribute and no-positive-tabindex. Both rules target an author hardcoding those values; here they are pass-through bindings, and removing them would change the component's argument list. 20 tests, both components at 100% coverage. 4964 tests pass, 0 skips. `pnpm run build` exits 0.
`<ModelSelect>` advertised `@infiniteScroll` but nothing behind it ran. The options
component rendered ember-infinity's `<InfinityLoader>` against `@infiniteModel`, which
was passed as `infiniteModel=this.model` — a `@tracked` property declared on the
component and never assigned anywhere. It was therefore always `undefined`, so the
loader's `{{#if}}` never opened. ember-infinity was not a dependency either, so the
component would have failed to resolve had the branch ever been reached (DEFECTS #105).
Paging is native now, with no new dependency:
- `loadModels` records the term it loaded for and asks the server for page 1, then
decides whether another page exists — preferring the total the server reports and
falling back to "the server filled the page", which is all the custom search
endpoint gives us.
- `loadMoreOptions` is a `dropTask`, so overlapping scrolls collapse into one
request. It repeats the current term at the next page and appends the results.
- The options component watches ember-basic-dropdown's content element (the thing
that actually scrolls) and calls `@onLoadMore` within 32px of the bottom. The
listener is torn down through `registerDestructor`.
- A spinner row marks the page in flight, styled alongside the existing spinner
rules and following the same `ember-model-select__*` naming.
A new search restarts at page 1 and keeps the term, so scrolling a filtered list pages
through the filtered results rather than the unfiltered ones.
Tests: 13 covering a full first page paging into a second, a short page ending paging,
the server's reported total ending it, `@infiniteScroll={{false}}`, a new search
restarting the sequence, overlapping scrolls dropped, a scroll short of the bottom
doing nothing, reaching the bottom of a complete list doing nothing, and the spinner
appearing while a page is in flight and going once it lands. The options test file's
two loader tests asserted the old inert behaviour and now assert the real thing.
Full suite 4975 pass / 0 fail / 0 skip. Coverage 93.73% statements, 89.28% branches,
97.24% functions, 94.13% lines.
Two branches in options.js stay partial: the null guards around `scrollable`. Inside a
rendered dropdown `closest('.ember-basic-dropdown-content')` always resolves, so they
are defensive only — documented in the source rather than suppressed. Two more in
model-select.js are pre-existing and recorded as DEFECTS #163: both task permission
guards are unreachable, because denying permission also sets `disabled` and
power-select refuses to open a disabled trigger.
`table/cell/resource-identity.hbs` rendered its trigger with a literal `class="flex min-w-0 items-start gap-2 text-left py-0.5"`. Every other visual aspect of this component is a column option with a default — `wrapperClass`, `imageSizeClass`, `imageRoundedClass`, the five `statusBadge*` getters — but the trigger's padding was the one thing no argument could reach, so the identity cell could not be compacted for a dense table however its column was configured (DEFECTS #108). The committed test said as much: it asserted `doesNotHaveClass('py-0.5')`, i.e. the template and its own test disagreed about whether the compact variant carries vertical padding. That mismatch shipped. Fixed the way the rest of the file already works — a getter reading the column with a default: get triggerClass() { return this.column.triggerClass ?? (this.compact ? 'py-0' : 'py-0.5'); } `column.compact` is the shorthand, so a caller need not know which padding class to drop; `column.triggerClass` replaces the padding outright and wins over the shorthand. `triggerClass` is the name this addon already uses for a trigger's classes (country-select, visible-column-picker, dropdown-button, content-panel). Default output is unchanged, so no existing table shifts. `py-0` is picked up by Tailwind, which scans `./addon/**/*.{hbs,js}`. Tests: five covering the default, the compact shorthand, an explicit triggerClass, the precedence between them, and the layout classes surviving either way. The pinned assertion is unpinned. Full suite 4980 pass / 0 fail / 0 skip; the component has no uncovered statements.
You confirmed on production that switching tabs reloads the subject every time. Two
separate causes, one already fixed on this stack and one not.
The first is the wiring: `custom-fields-manager.hbs` on main passes
`@onTabChange={{perform this.loadCustomFields}}`, which fetches unconditionally and
never reaches the `onTabChange` action holding the already-loaded guard. That is #97,
fixed earlier in this stack.
The second survives that fix. The guard read:
if (subject && (!subject.groups || subject.groups.length === 0))
`groups.length === 0` cannot distinguish "not fetched yet" from "fetched, and this
subject genuinely has no field groups". A subject with no groups configured is
therefore refetched on every single tab selection, forever — the same symptom, with the
wiring corrected.
Replaced the inference with a record of what has actually been fetched: a private
`#loadedSubjects` set, added to after a successful load and after a cache restore. A
load that fails is not recorded, so the next tab selection retries it.
While in `restoreFromCache`:
- `await loadCompany()` ran once per subject inside the loop. Hoisted; a company that
will not load now reports once and stops, instead of failing identically for every
subject.
- It read `cachedManager.groups`, the raw category array. The load path stores
`customFieldGroups`, the grouped form that carries each group's fields. Reading the
same accessor the load path uses keeps a restored tab identical to a fetched one
rather than relying on the two arrays sharing model instances.
- Dropped the `cachedManager &&` check. `forSubject` builds an empty manager on a
cache miss and never returns nothing, so that half could not fail.
The test double for the registry returned `null` from `forSubject`, which the real
service never does. It now returns an empty manager, as the service does.
Tests: three new — an empty subject fetched once however often its tab is selected, a
failed fetch retried on the next selection, and a company that will not load stopping
the restore before any lookup. The first two fail against the previous guard (verified:
31 pass / 2 fail) and pass against this one.
Full suite 4983 pass / 0 fail / 0 skip.
…nreachable
Working the 77 files whose remaining coverage gap was three sites or fewer. This is the
first pass: 29 of them are now clear.
Tests (13 new, no production behaviour touched):
- docs-panel: close, open-externally and the iframe-failure fallback. All three
actions are thin delegations reachable only through the panel's own controls, and
none were exercised. Three details worth recording for the next person: FontAwesome
renders `times` as `xmark`; the service calls the real global `window.open`, not
ember-window-mock's; and an `error` event dispatched at an element still reaches
`window.onerror`, which QUnit installs, so it has to be suppressed for the duration
or the test fails on the event it is deliberately raising.
- layout/resource/cards-grid: the yielded card. `cardClass` is only read when the
block renders a card, so the hash in the template was never built.
- layout/header/sidebar-toggle: the disabled toggle, both when disabled by argument
and when disabled by the sidebar service, plus the optional onToggle callback.
- file-icon: an upload file with no underlying file, and filenames with no extension
at all — both arms of the extension lookup fall back to null and every existing
fixture carried a well-formed name.
- table/th: a header rendered with no column at all.
Coverage exclusions (25 + 14, each line-specific and justified inline):
- `@tracked` field initializers whose value is assigned before it is ever read. With
Ember's decorators the initializer is lazy, so it is never invoked. Applied only
after checking each field really is assigned elsewhere; `modal/dialog`'s `@ref`
field was correctly skipped by that check and is left alone.
- Helper and modifier parameter defaults. Glimmer always passes both the positional
and named arguments, so those defaults cannot be reached from a template.
Three sites were investigated and deliberately NOT covered, because they cannot run:
- table/foot's `element instanceof HTMLElement` — `offsetElements` is a literal array
of two strings.
- truncate-pages' `if (res.length > 0)` — `res.push(currentPage)` above it is
unconditional, so the array is never empty.
- sidebar-toggle's `if (this.isDisabled) return;` — the template renders
`disabled={{this.isDisabled}}`, so the button cannot be clicked when it is true.
Also noted: `overlay/header`'s `useEllipsis` getter is referenced by no template. The
header gates on `@overlay.isMinimized` instead. Left in place pending a decision.
Full suite 4996 pass / 0 fail / 0 skip.
Coverage 93.99% statements (was 93.68%), 89.32% branches (89.19%), 97.40% functions
(97.25%), 94.42% lines (94.08%).
Batch A remaining: 48 files, 22 statements, 58 branches.
…rded
Continuing the ≤3-site files. Eleven more are clear.
Tests (6 new):
- tabs/tab: an inactive tab, which withholds both the active class and the pane entirely.
- with-record: a rejection carrying no message, which falls back to a generic one. Not
every rejection is an Error.
- table/cell/media-name: an alt-text path with no row to read it from.
- bulk-search-dropdown: clearing and searching with no handlers supplied at all.
`dropdown-fn` closes the dropdown on click, so the assertions reopen it first.
Coverage exclusions (8 more, each justified inline against something checked, not assumed):
- tabs/tab's inactive pane class — the pane it lands on is itself inside
`{{#if this.isActive}}`.
- table/foot's `element instanceof HTMLElement` — `offsetElements` is a literal array of
two strings.
- truncate-pages' `if (res.length > 0)` — `res.push(currentPage)` above is unconditional.
- sidebar-toggle's disabled early return — the template renders
`disabled={{this.isDisabled}}`.
- model-tag-input's non-array fallback in removeTag — a non-array attribute renders no
tags, so there is no remove control to click.
- modal/title-with-buttons' `dd?.actions?.close` guard — the template only calls it as
`(fn this.handler option dd)` inside a DropdownButton.
- unwrap-coordinates' `window.leaflet || window.L` — evaluated at module import.
- extensions-list and smart-nav-menu/item's `router ?? hostRouter` — `router` is a
framework service and always resolves.
Two defects found by writing the tests, both recorded rather than fixed (#165, #166):
- chat-window/attachment throws on any filename without a dot. `getExtension` returns
null and `getIcon` passes it straight to `getWithDefault`, which asserts on a non-string
key — so README, Dockerfile or LICENSE cannot render at all. <FileIcon> guards the
identical case two files away. The test that reaches it is one asserting a crash, which
would pin behaviour that should change, so the branch stays uncovered and the reason is
recorded.
- Two getters no template references: overlay/header's `useEllipsis` (the header gates on
`@overlay.isMinimized`, so its 15-character threshold has no effect) and
report-builder/condition-value's `isBoolean` (the template has no boolean arm, so a
boolean column gets a free-text field). The second may be a missing editor rather than
dead code, which is a product call.
Full suite 5001 pass / 0 fail / 0 skip.
Coverage 94.00% statements, 89.49% branches, 97.40% functions, 94.42% lines.
Batch A remaining: 37 files, 20 statements, 45 branches.
…g clipboard
Nine more of the ≤3-site files are clear.
Tests (5 new, all confirmed against the coverage report rather than trusted because they
passed):
- layout/resource/tabular: cycling a sort back off. The legacy single-column `sortBy`
and `sortDirection` fields fall back to null once nothing is sorted, which only
happens on the third click.
- locale-selector-tray: an explicit `@renderInPlace`, both true and false. The getter
only consults the argument when it is an actual boolean and the viewport is not
mobile; the module-level tests run against the real media service, and the mobile
stub is scoped to one nested module.
- click-to-copy: a failing `document.execCommand` in the fallback path, which is
reported rather than thrown.
- smart-nav-menu/dropdown: an item with no title, matched on its description instead.
Coverage exclusions (10 more, each checked against the code that makes it unreachable):
- report-builder/export-options' `if (!this.args.disabled)` — the Export button is
rendered `@disabled={{@disabled}}`, so it cannot be clicked while that is true. Same
shape as sidebar-toggle.
- table/cell/point's `if (column)` — `isClickable` is only true when the column carries
an onClick or action, and only then does the template render something clickable.
- table/foot's `typeof element === 'string'` — the companion to the instanceof guard;
every entry is a string literal, so this one is always taken.
- table/th's `if (!column)` — `showSortPriority` short-circuits on `isSorted`, so the
getter is never evaluated without a column.
- chat-tray/conversation-row's `participants.length ?? 0` — `participants` always
returns an array.
- widget/count's and vertical-offset-by's parameter defaults — each has exactly one
caller and it always passes both arguments.
- transition-end's `if (backup)` — `done` removes its own listener and nulls `backup`,
so it only ever runs once, with it set.
- sidebar-navigator's `router ?? host-router` — `router` always resolves.
- register-report-widget's lookup failure and missing-service return — the widget
service resolves in every booted app.
Full suite 5006 pass / 0 fail / 0 skip.
Coverage 93.98% statements, 89.57% branches, 97.40% functions, 94.40% lines.
Branches are up from 89.19% at the start of Batch A.
…ers on their own terms
Three more files clear, and the `did-update` family covered properly.
Tests (5 new):
- checkbox, filters-picker/button and full-calendar/draggable each wire a handler as
`{{did-update this.handler @arg}}` and destructure it with a default. That default is
not dead code and is not a framework artefact: it applies when the argument changes
*to* undefined, which is what a caller clearing its state does. One test each —
clearing @value unchecks the box, clearing @buttonComponentArgs drops the
active-filter badge, clearing @disabled makes the draggable draggable again.
- kanban's drop handlers read the same configuration as its drag-start handlers but
from their own code paths, so a custom columnIdPath was covered when a card lifted
and not when it landed. Added the landing case, plus a column drop with no
onColumnMove handler at all.
One exclusion: kanban's `onCardDrop(targetColumnId, targetPosition = null)` — the only
caller is kanban/column.js, which always passes the position.
Full suite 5011 pass / 0 fail / 0 skip.
Coverage 94.01% statements, 89.71% branches, 97.40% functions, 94.43% lines.
Batch A remaining: 22 files, 16 statements, 26 branches.
…d, and the last guards
Tests (3 new):
- layout/resource/panel: two tests supplying a @saveTask. The `authSchema` getter was
never evaluated at all, because Glimmer evaluates arguments lazily and its only
consumer is the header's save button, which does not render without a save task.
Uses the saveTaskHost pattern already established in header-actions-test.js.
- chart: a component torn down while its dataset loader is still in flight. That guard
is genuinely reachable — it is why it exists — so it gets a test rather than an
exclusion: render against a promise the test controls, clearRender, then resolve.
Coverage exclusions (11), each traced to the caller that makes it unreachable:
kanban/column's missing column body; layout/header/dropdown's and unit-input's
dropdown-close checks (the template and PowerSelect always supply the actions);
model-coordinates-input's child registration via @onInit; date-picker's node ref;
modal/dialog's did-insert element and its @ref field; and the owner/service guards in
is-dark-mode, get-universe-components and get-universe-menu-items.
Two corrections to the previous pass, both mine:
- The table/th exclusion had landed on the WRONG getter. A scripted insert matched the
first `if (!column) {` in the file, which is `sortColumn`'s — and that guard is
exercised by the no-column test added earlier, so the exclusion was suppressing a
real result instead of recording an unreachable one. Moved to `sortPriority`, which
is the getter genuinely never evaluated without a column. The report now shows
table/th at 0 uncovered statements and 0 partial branches with only that one
exclusion, which is what confirms the fix.
- Ignoring an `if` does not cover a `return` that follows it. The trailing fallbacks in
is-dark-mode, get-universe-components, get-universe-menu-items and sidebar-navigator
each needed their own comment.
Full suite 5014 pass / 0 fail / 0 skip.
Coverage 94.06% statements, 89.87% branches, 97.45% functions, 94.47% lines.
Batch A remaining: 10 files, 7 statements, 10 branches.
Branches read marginally lower than the previous pass (89.87% against 89.93%) because
removing the misplaced table/th exclusion restored a real partial branch to the report.
That is the correction working, not a regression.
…modal never resolved modal.js was the largest single remaining file (13 statements / 14 branches). Its gap turned out to be three unrelated things, only one of which was a real test gap. Tests (4 new). `<Modal>` declares `keyboard` and `backdropClose` and passes them to <Modal::Dialog>, which reads them only from inside `handleKeyDown` and `handleClick`. Glimmer evaluates arguments lazily, so until one of those handlers fires through a real <Modal>, the modal's own `@arg` defaults are never resolved — which is why they read as never-initialised. The existing dialog tests render <Modal::Dialog> directly with the arguments passed explicitly, so they never exercised the modal's side of it. Added: escape closing the modal by default and being ignored under @keyboard={{false}}, and the same pair for clicking the backdrop area. This is the same lazy-argument shape as layout/resource/panel's `authSchema` in the previous batch. Worth recognising on sight: an argument that is only read inside a handler is not evaluated until that handler runs. Exclusions (6): every `isFastBoot(this)` guard, plus the SimpleDOM branch of `addBodyClass` behind one of them. This suite runs in a browser, where isFastBoot() is always false, so neither arm can be reached. Left uncovered and NOT excluded — six teardown and re-entrancy guards: `_isOpen`, three `isDestroyed` checks sitting after awaits, and two `!modalElement` checks. These are the same shape as chart's teardown guard, which was genuinely reachable and got a test last batch. Reaching these needs the component destroyed mid-transition, and `render()` awaits settled, so the transition has already finished before a test could tear it down. They are reachable in principle, so excluding them would be wrong; recorded here instead. Full suite 5018 pass / 0 fail / 0 skip. Coverage 94.13% statements, 90.06% branches, 97.45% functions, 94.55% lines. Branches are past 90% for the first time. Batch B remaining after this file: 79 files, ~308 statements, ~357 branches.
Delete confirmed dead code; scope the coverage gate to first-party source
Adopt eslint-plugin-qunit and a rule for unguarded handler arguments
Rebuild translations-editor around stable rows
Replace ember-radio-button with a native implementation
Make model-select's infinite scroll actually work
Let the resource-identity cell actually compact
Stop custom-fields-manager refetching a subject on every tab change
Batch A: close the small coverage gaps, record the unreachable ones
Batch B (part 1): modal.js
pending-attachment: a file with no name, url or path (nothing to identify it by), one identified only by its path, a filename with no extension at all, and removing with no handler behind it. utils/dom: a negative timeout, which means "wait indefinitely" — no deadline is armed, so there is none for cleanup to clear either. That one line was the only thing standing between waitForInsertedAndSized and full coverage. header dropdown item: no @item at all, and an ExtensionComponent instance, which takes the component branch rather than the text branch. The assertion is that its text is NOT rendered, because the component itself resolves through a lazy engine that a rendering test has no engine to load. timeline's two paging guards are ignored rather than tested: timeline.hbs disables each arrow at its end of the range, so neither guard has a control to fire it — the same shape as four earlier findings. Full suite: 5330 tests, 0 failures. 98.31 / 98.35 / 99.59 / 97.55.
…o 100% DEFECTS #27: array-input's row bound keyup to `inputDatum`, whose second parameter is the value — so every keystroke stored the KeyboardEvent itself in the array and reported it to @onDataChanged. Confirmed with a probe before fixing: after one keyup the first element had `typeof === 'object'`. keyup now goes through onChange, the same handler the change event uses. The two handlers with no coverage at all were the tell — nothing had ever fired a keyup or a paste on a row. autocomplete-input: a geocoder that answers with an error object rather than a list, focusing before any search has run, focusing again after one, clearing the field (a blank query is not sent anywhere), and selecting with no handler. fetch-select: the selection is resolved during construction, before the first fetch finishes, so the two arms that re-resolve it against freshly loaded options had never run. Reopening the select is what reaches them. notification-tray: the mobile layout — a full-width sheet under the trigger, and @renderInPlace refused outright — plus a delete event carrying one record rather than a list. Full suite: 5345 tests, 0 failures. 98.47 / 98.51 / 99.64 / 97.81.
DEFECTS #28: one unusable record emptied the whole model-select dropdown. The custom-search-endpoint mapper returns null for a record the store refuses to normalise, and those nulls stayed in the list handed to power-select, whose option walker dereferences every entry. Found by writing the test for that catch: it failed inside power-select rather than on its assertion. The catch already meant "drop this one" — now `resolve(records.filter(Boolean))` does. Also covered: clearing a model-select reports to @onclear with the select api, opening and minimising a drawer with no callbacks behind them, and the events service integration in filters-picker. filters-picker carried a `hasFilters` getter referenced by nothing; deleted. Two ignores here rest on the dummy app rather than on the code: `events` and `hostRouter` are looked up by name, and the dummy app ships both, so the absent-service paths cannot be exercised — the resolver re-provides them even after an explicit unregister. The traces say so rather than claiming the guards are dead. Full suite: 5350 tests, 0 failures. 98.67 / 98.69 / 99.68 / 98.12.
Dead state removed rather than ignored: button's `disabled` (isDisabled reads this.args.disabled and the constructor never assigns the field), click-to-reveal's `timeout`, country-select's `id`, and dropdown-button's `_onTriggerInsertFired` / `_onButtonInsertFired`, which were written on every insert and read by nothing. Covered: a chart value that is neither a number, a Date nor a string; a comment whose parent has no uuid, then no public id either; and — in comment-thread/comment specifically, which is a public component in its own right, because through <CommentThread> the contextApi always supplies the reply task and that branch never runs. The rest are the constructor-initializer pattern this codebase uses everywhere: a `@tracked` field with a default that the constructor overwrites from an argument before anything reads it, so the initializer never produces the value that survives. Full suite: 5351 tests, 0 failures. 98.92 / 98.93 / 99.73 / 98.28.
DEFECTS #29: aside-item-scroller crashed on an item with no title. The guard written to skip it sat one line below `title[0]`, so an untitled item threw before it could run — and since the grouped list is read straight from the template, one bad item rendered nothing at all. Same shape as the @item guard in layout/header/dropdown/item.js, fixed earlier for the same reason. The `!title` check moved above the dereference; `!firstLetter` went with it, because a non-empty string always has a first character. Also covered: filter/range with no @filter of its own (every use in range.hbs defends with `or @filter.min 0`, so the component has to stand up without one), an unparseable upper half, and selecting a currency with no callbacks. dropdown-fn's untouchable-`this` Proxy had three traps raising the same assertion through three copies of the same line; they are now one handler, which is behaviour-identical because the assertion never returns. Its statement is ignored with a note naming the three tests that do exercise it — istanbul does not record a statement inside a Proxy trap reached from that module-level IIFE. Full suite: 5356 tests, 0 failures. 98.99 / 99.00 / 99.73 / 98.51.
phone-input's geoIpLookup was reaching json.geoiplookup.io over the real network on every run — lookupUserIp is called with `initialCountry: 'auto'`, and nothing stubbed it. The tests now stub fetch, so the suite stays off the internet. Its two fallback paths are ignored rather than tested: intl-tel-input resolves the auto-country once per page and caches it, so geoIpLookup runs at most once in a whole run and there is no second outcome to produce. Covered: a date-only ISO string (no time part, so it falls through to parseISO), a value shaped like a local date-time but impossible, a time chosen before any date, restarting a countdown whose seconds argument has gone away, a rename field taking a key that is neither Enter nor Escape, an altText callback, a status badge with no configured size, and an image on a row with nothing to label it. Full suite: 5368 tests, 0 failures.
The sort comparators in to-power-select-groups had only ever been asked to move things: every existing test hands them a list that is out of order, so the arms that report "after" and "equal" were never taken. Two lists already in order and two options with identical labels reach them. Groups cannot compare equal — they are keyed by their own name — so that arm is ignored with the reason. docs-panel's seven tracked defaults were invisible because every test calls open() first, which assigns all of them. One test now looks at the service before anything has happened to it. modals-manager: a modal shown with no options hash at all. The rest are ignored — the `events` lookup the dummy app satisfies, and three guards inside the prompt modal's own controls, which cannot run with that modal off the stack. Full suite: 5374 tests, 0 failures.
DEFECTS #30: a hover dropdown asked for `@openDelay={{0}}` got 300ms instead. getDelay picked its answer by truthiness, so 0 fell through both checks to the default — which also meant the immediate-open and immediate-close arms below it could never run, since getDelay could not return a falsy delay. Both checks now compare against undefined, and the two arms are covered by a test that opens and closes with no timer in between. registry-yield: a universe fallback that answers with nothing at all rather than an empty list, on both the menu and the component side. Its two service guards are ignored — menuService and registryService are injections, and the addon ships both services, so they always resolve. isComponent's empty-list arm is ignored too: registry-yield.hbs only reads that getter from inside {{#each this.yieldables}}, so there is always at least one. Also covered: mousedown on a hover dropdown's trigger, which the component swallows so a press cannot toggle a dropdown that is meant to be hover-driven. Full suite: 5381 tests, 0 failures.
DEFECTS #31 logged, not fixed: <Attach::Popover @arrow={{true}}> renders an arrow element that is never positioned, for two independent reasons. floating.js looks the arrow up with `element.closest('[x-arrow]')`, but the arrow is a *descendant* of the floating element, so the middleware is never installed — and computePosition destructures only {x, y} from the result, so middlewareData.arrow would be discarded even if it were. Making it work is implementing the feature, not correcting a slip, and it changes how every popover with an arrow looks. Deleted: dashboard's `setCurrentDashboard`, an @action that delegates to the service and is called by nothing — the same shape as resource-context-panel's setActiveTab. Covered: a custom field whose values have not loaded, a file value that arrives already parsed rather than as JSON, downloading a file value, a date-picker custom field, and a text field typed into with no handler behind it. Full suite: 5386 tests, 0 failures. 99.60 / 99.59 / 99.95 / 99.48.
… guards
canvas: tapping an element is what tells the parent to select it, and no test
had ever produced a tap on the canvas — interact.js listens for real pointer
events, so the new tests dispatch them the way a browser would, with and without
a handler above.
variable-picker: a whitespace-only formula, which is pressable because the
button is disabled on `not this.formulaExpression` and whitespace satisfies
that; inserting with no handler; a schema that declares no variables; and a
variable with no label of its own, matched on its path instead.
query-form's remaining guards are ignored with their reasons — the tracked
string and array fields that are never nullish, and the two row-editing guards
whose index comes from {{#each}} over the very list being indexed.
Full suite: 5391 tests, 0 failures.
`pnpm run coverage:check` exits 0. This batch closed the last nine files. The interesting one was model-multi-file-upload's `failedStates`, a tracked array nothing reads — deleted rather than ignored, like the other dead fields this campaign turned up. Covered: a changelog failure with no message of its own, a response body of null, a language mapping to null in translations-editor, and a queued file whose queue has already let go of it. Everything else was the two shapes this campaign has been ending on: a tracked field whose constructor overwrites it before anything reads it, and a guard whose precondition the caller has already established. Each carries the trace that says which. One mechanical note for whoever reads these next: `istanbul ignore next` does not attach to an object-property value. Both of the last two branches — in report-builder and query-form — needed the expression hoisted into a local before the comment would take. Full suite: 5395 tests, 0 failures.
DEFECTS gains a status section: the gate passes, what survives is the worklist that outlived the campaign — three findings that need a product decision rather than a fix, and #18, which is about the measurement rather than the code. #18 is annotated rather than closed. It has not been observed since the gate reached 100%, and three consecutive runs agree at 5963/5963 branches — but the denominator is far smaller than when it was found, because a large number of unreachable branches now carry traced ignores instead of sitting in it. The mechanism was never located, so a run that comes back at 5962/5963 is this, not a regression. Two conventions are written down where the next person will look: every ignore in the addon names the specific thing that makes the code unreachable, and `istanbul ignore next` will not attach to an object-property value — hoist the expression into a local first.
The full suite has never finished in CI. Every run died in the same place — notification-tray > interacting: the view-all link reports the press — with "Browser timeout exceeded: 120s", which reads like a hung test but is testem losing the browser outright. The view-all control is a real <LinkTo>. The test held a modifier key so Ember would skip its own transition (a rendering test has no application route to service one), but a modifier-held click is precisely the click Ember hands back to the browser, default action intact — so the page followed the href and navigated away. macOS Chrome swallows it; headless Linux Chrome does not, which is why it only ever failed in CI. Hold the modifier AND suppress the default action, rather than trusting either platform's handling of one.
With the suite finally completing in CI, the gate ran for the first time and found two files short there that are 100% locally. Both come down to the same thing: headless Linux Chrome never gives the page focus, and macOS Chrome does. tip-tap-editor's onFocus/onBlur carried an ignore on the method *body*, which covers the statement but not the function — tiptap still has to call the method for that to count, and locally it does. Moved the ignore above the method so the function goes with it; the reason it names was already the right one. layers-panel's re-entrancy guard in commitRename was never covered on purpose. It was covered by the blur the browser fires as the rename field is torn down, which macOS Chrome sends and headless Linux Chrome does not. Replaced that accident with a test that dispatches two blurs in the same tick — the field is still mounted, with its listener attached, until the render that removes it, so this is the same window the guard defends. Removing the guard fails the test on its own, which the incidental coverage could never have told us.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
The gate passing locally and the gate passing in CI turned out to be different claims, and the two conventions added here are the reason. Also corrects the BLOCKERS entry that filed a navigating link under machine contention — same error string, different cause, and the misfiling cost real time.
Consolidates DEFECTS.md, BLOCKERS.md and NEED_INFO.md into a single review document: five decisions that are Ron's, one made without an answer that is reversible, one diagnostic task with the method written down, and the machine fact that reads like a broken suite and is not. Points at the three source files rather than replacing them — they stay authoritative. Also carries what a fresh session needs before touching anything: the hard constraints, the run commands, and the four conventions that cost the most to learn.
…lewareData.arrow applied (DEFECTS #31)
… dead columns getter (DEFECTS #21)
…comes alive (NEED_INFO #3)
Closed
This was referenced Sep 16, 2026
Closed
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.
Brings the addon from a suite that could not complete a run to 5,396 tests passing, 0 skips, with coverage measured on every CI run, gated per-file at 100%, and uploaded to Codecov.
pnpm run coverage:checkexits 0 in CI — run 32938940906. Twenty-plus PRs merged into this branch before the last stretch was done directly on it.DEFECTS.mdis the reference for anything found along the way; it is kept as a worklist, so fixed entries are removed and preserved in git history (archive atc9bc486).Coverage
Opened at 92.02% / 87.50% / 94.81%. These are the numbers CI printed on
a210ac1, not a local run — see the comment thread for why that distinction turned out to matter.Every
istanbul ignorein the addon is line-specific and its comment names the exact thing that makes the site unreachable — the caller that always passes the argument, the template that renders the controldisabledor inside{{#unless}}, the constructor that assigns the field before anything reads it, the library that filters the call before it arrives. An ignore without that trace is a bug waiting to be reintroduced, and reviewing them is the point: if a reason does not hold, the gate is wrong, not the code.What is in here
addon/andapp/, +3,031 / −1,722. Every one needed either to make the suite runnable or to close a defect the tests exposed.codecov.ymlat 100% / 0% threshold; CI running the self-test, the suite, the gate, an LCOV check and the upload.Coverage collection was itself broken, and that mattered more than any single defect
For most of this work, coverage reports were being silently lost. The
/write-coveragePOST carries several megabytes — a per-file 100% gate requiresforceModulesToBeLoaded()to evaluate every module so untested files stay in the denominator — and testem tore the browser down before the upload finished.raw-bodyaborted, nothing was written, and the previous run's artifact stayed on disk looking current.Measured: fast filtered runs produced a report 2 times in 9; full runs always did. That size-and-timing dependence is why it read as flakiness for so long. It is a known upstream bug (#420, #421, testem#1577); our measurements are now on #420.
Two pieces landed:
Testem.afterTestsinstead ofQUnit.done— testem waits for that callback, so the upload completes. Paired withbrowser_disconnect_timeout: 120, because testem's 10s default then becomes the new failure mode.scripts/check-coverage.js. The gate now refuses a stale artifact, a missingcoverage-final.json, or an unstamped run before reading a single percentage.The second is worth keeping regardless of the first: it is what turned a silent wrong answer into a loud one. A gate that reads whichever file happens to be on disk can report a regressed line as covered, and that is worse than a red build.