Raise test coverage to the 90% QASP target on all four metrics by fpigeonjr · Pull Request #680 · GSA/sam-ui-elements

fpigeonjr · 2026-09-02T19:22:25Z

Description

Closes the remaining coverage gap to the 90% QASP target on all four metrics. Branches was the laggard — the metric flagged in #669 as furthest behind — and this PR is the first time it clears 90%.

metric	before	after	target
statements	88.76%	95.60%	90% ✅
branches	78.65%	90.09%	90% ✅
functions	86.00%	92.78%	90% ✅
lines	88.74%	95.67%	90% ✅

Test count: 1,380 → 1,881 across 151 → 173 spec files.

No library source changed. The only non-spec file in the diff is coverage-floor.json.

Approach

Ranked every file under 90% branch coverage by absolute uncovered-branch count (parsing coverage-summary.json), then used lcov.info BRDA: records with a zero hit count to pinpoint the exact line and branch index needing a test. Worked the ranked list top-down until all four metrics cleared the target, deliberately stopping short of the jsdom-hostile files (see below).

Commits are grouped by area so they can be reviewed — or reverted — independently:

commit	area
`55bddf17`	experimental layout-pattern internals — overlay stack, expansion module, `UniqueSelectionDispatcher`, architecture layer, Main/Page/Actionbar, sidenav
`fab1271c`	the four autocomplete implementations (largest absolute branch gaps in the library)
`c07a68fc`	pipes, dom helpers, fa-icon shared utils
`6bc0fbf0`	phone-entry and international-phone form templates
`ab81bfc4`	components, pagination, aria primitives
`ccd76bb2`	date-range validators, upload-v2 edge paths
`403d3196`	label/fieldset wrappers, sam-telephone
`0bf5ea34`	image, textarea, page service, fa-icon error helpers — crosses 90% branches here
`15907371`	selected-item model, results-message clamp
`2f9f7478`	lint-debt cleanup (see below)
`3853874a`	the floor bump, standalone

Twelve files that had zero branch coverage and no spec at all now have one — most of the overlay/expansion internals, plus Paginator, SamSortHeader, SamSidenavService, HierarchicalTreeSelectedItemModel, and the fa-icon error helpers.

Two things worth a reviewer's attention

1. 2f9f7478 is lint debt I created and then paid off. The new specs initially pushed the root ESLint baseline from 1619 → 1800 warnings, ~95% of it @typescript-eslint/no-explicit-any from test doubles and private-member access. Rather than bump the ceiling, I rewrote them:

(component as any).method()  →  component["method"]()
{ ...stub } as any           →  { ...stub } as never
(cb: any) => ...             →  (cb: unknown) => ...

Bracket access reaches private/protected members without disabling type checking; never is assignable to any parameter slot, so a stub literal still reads as "deliberately unchecked test double". The branch now sits at 1581 warnings — 38 below the recorded baseline of 1619.

eslint-baseline.json is deliberately left unbumped. Lowering it is a separate npm run lint:baseline:bump change under the same ratchet rule that governs coverage-floor.json. Worth a follow-up.

2. Files I intentionally did not chase. ~200 uncovered branches remain, concentrated in picker.component.ts (25), sidenav.ts (21), connected-position-strategy.ts (16), tab-header.ts (15), and tabs.component.ts (14). These depend on real layout, the CSS cascade, or animation/detach timing — exactly the two classes of defect AGENTS.md documents jsdom as structurally incapable of catching. Adding Vitest coverage there would mostly assert stubs. If those need real coverage, Playwright is the honest tool, and that's a separate piece of work.

Where jsdom limits were unavoidable, the specs stub at the seam between layout and the decision under test and say so in a comment — e.g. calcToggle tests stub calculateNumberOfLines because jsdom reports offsetHeight as 0, and sidenav.spec.ts overrides _width via Object.defineProperty.

Several assertions also encode genuine quirks discovered while writing them, with comments explaining why: initilizeFileCtrl exposes its postedDate argument as date; Paginator.nextPage() will overshoot when the total is an exact multiple of the page size; process() in phone-entry calls updateModel() even on caret-only keys; KeyHelper compares event.key against numeric literals, so "1" doesn't match but code: "Digit1" does.

Previously-skipped tests now enabled

Three it.skip tests were reinstated rather than left as decoration — two in short-date.pipe.spec.ts and one in date-time-display.pipe.spec.ts, all three originally skipped with a comment saying the intended business rules weren't clear. Each now runs against a fixed clock. One assertion uses a regex rather than an exact length because the pipe's h format is unpadded and would otherwise be flaky by hour.

Motivation and Context

Closes #669

Final item in the coverage epic tracked under #576, following the measurement in #637 and the floor lock in #670/#673.

One AC on #669 is not addressed here and needs a human: "Parent epic (#576) re-reviewed and closed if this closes out its remaining AC." Whether #576 has anything left is a judgment call for a maintainer after this merges.

Type of Change (Select One and Apply Label)

Bug fix (non-breaking change which fixes an issue) → Apply bugfix label
New feature (non-breaking change which adds functionality) → Apply enhancement label
Breaking change (fix or feature that would cause existing functionality to change) → Apply breaking label
Documentation / configuration update → Apply maintenance label

How to Test

npm ci && npm ci --prefix test-app
npm --prefix test-app test — full Vitest suite with coverage; writes test-app/coverage/coverage-summary.json.
npm run coverage:check — the gate, now running against the raised floor.
npm run lint:baseline and npm --prefix test-app run lint:baseline — confirm neither baseline is exceeded.
node scripts/check-coverage-floor-not-decreased.mjs <(git show origin/master:coverage-floor.json) coverage-floor.json — confirm the floor only moved up.
npm run format:check
npm run validate:publish — confirm the frozen consumer deep-import list is intact.

Expected result: 173 spec files / 1,881 tests pass. All four coverage metrics report ≥90% and satisfy the raised floor. Both lint baselines pass (root reports 1581, under its 1619 baseline). Prettier and the publish contract are clean.

Full local run:

npm --prefix test-app test           173 files, 1881 tests pass
npm run coverage:check               ✓ statements 95.60  branches 90.09  functions 92.78  lines 95.67
check-coverage-floor-not-decreased   ✓ no metric's floor decreased vs. base
npm run lint:baseline                ✓ root 1581 warnings (baseline 1619), 0 errors
npm --prefix test-app lint:baseline  ✓ test-app 4 warnings (baseline 4), 0 errors
check-baseline-not-increased         ✓ no workspace's ceiling increased
npm run format:check                 ✓ Prettier clean
cd test-app && npm run build         ✓ builds (pre-existing Sass @import deprecation warnings only)
npm run validate:publish             ✓ 806 packed files, 67 consumer deep imports intact
node --test scripts/*.test.mjs       ✓ 37/37 (the gate tests CI doesn't run)

Screenshots (if appropriate)

N/A — test-only change, no UI or library source modified.

Checklist

Branch name follows convention (e.g. gh-<number>-<slug>)
PR title starts with a verb in the imperative mood
I have self-reviewed my own code
format:check passes (npm run format:check)
lint passes (npm run lint)
build passes (cd test-app && npm run build)
Tests pass and coverage is reported (cd test-app && npm test)
If this change requires a documentation update, I have updated it accordingly
If there are dependent changes, they have been merged and published in downstream modules

Copilot

🟡 Changes recommended

A critical teardown defect and multiple moderate correctness gaps remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Raises all four coverage metrics above the 90% QASP target through expanded tests and an updated coverage floor, without library-source changes.

Changes:

Adds 501 tests across 22 additional spec files.
Re-enables previously skipped date-format tests.
Raises all four coverage ratchets above 90%.

File summaries

File	Review
`src/ui-kit/wrappers/label-wrapper/label-wrapper.spec.ts`	Covers errors and hint behavior.
`src/ui-kit/wrappers/fieldset-wrapper/fieldset-wrapper.spec.ts`	Covers errors and hint overflow.
`src/ui-kit/pipes/short-date/short-date.pipe.spec.ts`	Re-enables date-format cases. Nit (1 vote): Freeze time in the fallback test to prevent date-rollover flakiness.
`src/ui-kit/pipes/filesize/filesize.pipe.spec.ts`	Covers invalid and oversized inputs.
`src/ui-kit/pipes/date-time-display/date-time-display.pipe.spec.ts`	Covers older dates and invalid input.
`src/ui-kit/layout/pagination/paginator.spec.ts`	Adds paginator coverage. Moderate (1 vote): Add and correctly handle the exact-multiple boundary case.
`src/ui-kit/layout/pagination/pagination.component.spec.ts`	Extends lifecycle and event coverage.
`src/ui-kit/layout-deprecated/page.service.spec.ts`	Covers sidebar sizing branches.
`src/ui-kit/layout-deprecated/list-results-message.spec.ts`	Covers final-page clamping.
`src/ui-kit/form-templates/phone-entry/phone-entry.spec.ts`	Covers formatting, validation, and key paths.
`src/ui-kit/form-templates/international-phone/sam-telephone/telephone.spec.ts`	Covers validation and input handling.
`src/ui-kit/form-templates/international-phone/international.spec.ts`	Covers configuration and form events.
`src/ui-kit/form-controls/upload-v2/upload-v2.spec.ts`	Covers upload edge paths.
`src/ui-kit/form-controls/textarea/textarea.spec.ts`	Covers initialization, validation, and counters.
`src/ui-kit/form-controls/sam-sds-autocomplete/autocomplete-search/autocomplete-search.component.spec.ts`	Broadens autocomplete branch coverage.
`src/ui-kit/form-controls/date-range/date-range.spec.ts`	Extends validator and lifecycle coverage.
`src/ui-kit/form-controls/autocomplete/autocomplete.spec.ts`	Expands autocomplete coverage. Moderate (2 votes): The test currently codifies duplicate results instead of deduplication.
`src/ui-kit/form-controls/autocomplete-multiselect/autocomplete-multiselect.spec.ts`	Expands multiselect coverage. Moderate (1 vote): Exact matches must suppress duplicate free-text options. Nit (1 vote): Use a reachable categorized-results shape.
`src/ui-kit/experimental/tabs/tab-nav-bar/tab-nav-bar.spec.ts`	Adds tab-navigation tests. Critical (1 vote): Setup masks destruction failure caused by an uninitialized resize subscription.
`src/ui-kit/experimental/patterns/layout/components/sidenav/sidenav.spec.ts`	Covers modes, directionality, and transitions.
`src/ui-kit/experimental/patterns/layout/components/page/page.component.spec.ts`	Adds responsive page tests.
`src/ui-kit/experimental/patterns/layout/components/main.component.spec.ts`	Covers filter-drawer integration.
`src/ui-kit/experimental/patterns/layout/components/expansion/expansion-panel.spec.ts`	Adds expansion-panel state tests.
`src/ui-kit/experimental/patterns/layout/components/expansion/expansion-panel-header.spec.ts`	Covers keyboard interaction.
`src/ui-kit/experimental/patterns/layout/components/expansion/accordion-item.spec.ts`	Covers accordion lifecycle. Moderate (2 votes): The current test does not actually verify listener deregistration.
`src/ui-kit/experimental/patterns/layout/components/core/overlay/scroll/scrollable.spec.ts`	Covers scroll registration and cleanup.
`src/ui-kit/experimental/patterns/layout/components/core/overlay/position/viewport-ruler.spec.ts`	Covers viewport calculations.
`src/ui-kit/experimental/patterns/layout/components/core/overlay/overlay.spec.ts`	Covers overlay creation. Nit (1 vote): Cleanup leaves empty container elements in `document.body`.
`src/ui-kit/experimental/patterns/layout/components/core/overlay/overlay-container.spec.ts`	Covers container themes and reuse.
`src/ui-kit/experimental/patterns/layout/components/core/coordination/unique-selection-dispatcher.spec.ts`	Covers listener dispatch and removal.
`src/ui-kit/experimental/patterns/layout/components/actionbar.component.spec.ts`	Covers pagination synchronization.
`src/ui-kit/experimental/patterns/layout/architecture/update/reducer.spec.ts`	Covers reducer actions.
`src/ui-kit/experimental/patterns/layout/architecture/service/service-property.spec.ts`	Covers service model updates.
`src/ui-kit/experimental/listbox/listbox.component.spec.ts`	Expands listbox coverage. Moderate (2 votes): The expected order contradicts the claim and mixes strings with option objects.
`src/ui-kit/experimental/icon/fa-icon/shared/utils/object-with-keys.util.spec.ts`	Covers keyed-object utilities.
`src/ui-kit/experimental/icon/fa-icon/shared/utils/normalize-icon-spec.util.spec.ts`	Covers icon normalization.
`src/ui-kit/experimental/icon/fa-icon/shared/utils/classlist.util.spec.ts`	Covers generated icon classes.
`src/ui-kit/experimental/icon/fa-icon/shared/errors/warn-if-icon-missing.spec.ts`	Covers missing-icon diagnostics.
`src/ui-kit/experimental/hierarchical/hierarchical-tree-selectedItem.model.spec.ts`	Covers selection-model operations.
`src/ui-kit/experimental/hierarchical/autocomplete/autocomplete.component.spec.ts`	Expands hierarchical autocomplete coverage.
`src/ui-kit/experimental/aria/abstract-combobox/abstract-combobox.spec.ts`	Covers keyboard and focus accessibility.
`src/ui-kit/dom-helpers.spec.ts`	Covers legacy scroll fallbacks.
`src/ui-kit/components/sidenav/services/sidenav.service.spec.ts`	Adds sidenav service coverage.
`src/ui-kit/components/multiselect-dropdown/multiselect-dropdown.spec.ts`	Covers labels and visibility.
`src/ui-kit/components/image/image.spec.ts`	Covers editing and staged images.
`src/ui-kit/components/header-next/header.spec.ts`	Covers navigation focus and resize.
`src/ui-kit/components/data-table/sort-header.component.spec.ts`	Adds sort-header coverage.
`src/ui-kit/components/banner/banner.spec.ts`	Covers banner closing behavior.
`coverage-floor.json`	Raises all four coverage floors.

Review details

Suppressed comments (3)

src/ui-kit/experimental/patterns/layout/components/core/overlay/overlay.spec.ts:37

Each test appends a container div, but cleanup removes only its pane descendants, so every case permanently leaves an empty element in document.body. Remove the container itself to keep DOM state isolated across this suite and later specs.

  afterEach(() => {
    document.body
      .querySelectorAll(".cdk-overlay-pane")
      .forEach((el) => el.remove());

src/ui-kit/form-controls/autocomplete-multiselect/autocomplete-multiselect.spec.ts:520

This manually assigned [nested] is not the categorized-results shape produced by sortByCategory() (which returns an object with numeric category properties). The test asserts a known wrong result solely to hit item[0]; use a reachable state with the expected no-duplicate behavior, or leave an unreachable branch uncovered rather than adding a misleading regression test.
src/ui-kit/pipes/short-date/short-date.pipe.spec.ts:39
This uses two separate real-time reads: the argument's moment() and the pipe's internal moment(). If the date rolls over between them, the pipe selects a date-bearing format and the regex fails; freeze time while still omitting fakeNow so this fallback test is deterministic.

Files reviewed: 49/49 changed files
Comments generated: 6
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

fpigeonjr · 2026-09-02T20:31:45Z

+    // clone.splice() inserts the raw option object at the computed index,
+    // not its .value -- this asserts the actual (if surprising) behavior.
+    expect(component.model).toEqual([options[6].value, options[2]]);


Agreed on the diagnosis, declined the fix for now — renamed and documented instead in 81916e85.

You've identified a real inconsistency: clone.splice(i, 0, option) inserts the option object, while _ordering, isChecked() and setSelectedItem() all key off option.value. So the model genuinely can end up heterogeneous depending on how a selection was made.

I didn't change onChecked() because model is both an @Input and an @Output (modelChange) and flows through onChange to ControlValueAccessor consumers, and two pre-existing tests on master already push option objects through it — onChecked checked/unchecked and Should remove item from selected results (which relies on filter(val => val !== option) matching by object identity). Making the insert use option.value would break that removal path unless it changes too, and would alter what downstream forms receive. That's a behaviour fix, not a coverage one.

The test name no longer claims an ordering it doesn't produce, and the comment now says plainly that this is a latent inconsistency rather than intended behaviour, with the reason it's left alone. Happy to open a follow-up issue to normalise the model on option.value across onChecked/filter/isChecked together — say the word.

fpigeonjr · 2026-09-02T20:31:32Z

+    it("should append new results without duplicating when they already exist and differ", () => {
+      component.results = ["aaa"];
+      component.lastReturnedResults = ["zzz"];
+      component.requestSuccess(["aaa", "bbb"]);
+      expect(component.results).toEqual(["aaa", "aaa", "bbb"]);


Half agreed — took the rename option in 81916e85 rather than changing the behaviour, and I want to be explicit about why.

You're right that the test name was wrong and that it locks in duplicates. But requestSuccess() is the httpRequest paging path (subscribed in ngOnChanges), and appending is the point: each emission is the next page pushed onto what's already displayed. The areEqual(data, this.lastReturnedResults) guard exists to skip a re-emission of the same page, not to de-duplicate item-by-item — so overlapping pages do repeat by design of the current contract.

Making it append only unseen items would change published paging behaviour for consumers relying on it, which is beyond a coverage PR. So the tests now describe the real contract, and I strengthened the companion test — it previously asserted only lastReturnedResults, so it never actually verified the guard suppressed the append; it now asserts results too.

If per-item de-duplication is the desired behaviour, that's worth its own issue and I'm happy to open one — flag it and I will.

Addresses PR #680 review. Each of these was a real bug that a new spec had documented rather than caught. MdTabNav.ngOnDestroy() unconditionally called _resizeSubscription.unsubscribe(), but nothing in MdTabNav ever assigns that field — only a subclass that subscribes to window.resize would. Confirmed by instantiating a plain MdTabNav and destroying it: 'Cannot read properties of undefined (reading unsubscribe)'. The existing spec passed only because it injected a Subscription first. Teardown is now optional-chained, with a test that creates and destroys a fresh nav. Paginator._exceedsTotal() tested 'r > 0 && r > unitsPerPage', which let the first empty page through whenever the total was an exact multiple of the page size — 100 units at 10 per page accepted page 11. A page is out of range once its first unit is past the end of the data, i.e. remainder >= unitsPerPage, with page 1 still valid for an empty data set. Verified the new predicate against the old one across exact-multiple, partial-final-page, empty and single-page totals. showResultsFreeText() passed item[0] — one sub-item — to findItemExistInList(), which iterates a list. A nested category sub-list containing an exact match therefore reported no match, and the component offered a duplicate free-text option alongside the real one. Now passes the sub-list. Behaviour is unchanged for every other list shape (flat item objects skip the guard entirely; strings and non-matching sub-lists return false either way). Also strengthened three assertions that were weaker than their names: - accordion-item's deregistration test destroyed an already-collapsed, parentless item, so no notification could have changed it even with the listener still attached. It now keeps the item expanded in a non-multi accordion and notifies a different id in that same accordion, plus a control case proving that notification does collapse a live item. - listbox's onChecked() ordering test now says explicitly that inserting the option object while isChecked()/setSelectedItem() compare against option.value is a latent inconsistency, not intended behaviour. Not changed here: the model is @Input/@Output-visible and two pre-existing tests already pass option objects through it. - autocomplete's requestSuccess() tests are renamed to describe the real contract. It is the httpRequest paging path: each emission is a page appended to what is displayed, de-duplicated per-payload against lastReturnedResults but not per-item, so overlapping pages do repeat. The 'same data twice' test now also asserts results, not just lastReturnedResults, so the guard is actually verified. Branches 90.14% -> 90.17%; 1891 tests pass. Refs #669

Adds specs for the layout pattern's previously-untested internals: the overlay stack (Overlay, OverlayContainer, Scrollable, ViewportRuler), the expansion module (ExpansionPanel, ExpansionPanelHeader, AccordionItem), UniqueSelectionDispatcher, the architecture layer (reducer, ServiceProperty), and the Main/Page/Actionbar components. Extends the existing sidenav spec to cover margin/position computation, Escape handling, transitionend filtering, RTL directionality, and push mode. Several of these files had 0% branch coverage and no spec at all. Refs #669

The four autocomplete implementations carried the largest absolute branch gaps in the library. Adds isolated unit tests for their keyboard navigation (arrow wrap-around, Enter, Escape, Tab, backspace), free-text handling, category resolution and sorting, scroll/highlight bookkeeping, result merging, and ControlValueAccessor guards. Tests exercise methods directly rather than driving a full TestBed render where the component's async chains hang under fake timers. Refs #669

Adds specs for the fa-icon classlist and normalize-icon-spec utils (both now at 100% branch coverage) and extends the filesize, date-time-display, and dom-helpers specs to cover their warn/fallback paths. Un-skips two short-date pipe tests that were disabled because the expected business rules weren't obvious; the same-year vs different-year formats are now pinned against a fixed clock. One assertion uses a regex rather than an exact length because the pipe's 'h' format is unpadded. Refs #669

Extends both form-template specs to cover their untested branches: phone-entry's writeValue() empty-value fallbacks, validator composition (preserving a caller-supplied validator, and skipping the default one), the caret-move and unrecognized-key paths through process(), and the position increment/decrement wrap cases. international-phone gains coverage of the extension-required throw, the prefix reset, and SamFormService submit/reset formatting. Refs #669

New specs for SamSortHeader (register/deregister, sort click handling, disableClear coercion), SamSidenavService (model truncation, selected model, getPath including the missing-route warning), Paginator (page validity, remainder math, display strings), and TabNavBar. Extends banner, header-next, multiselect-dropdown, listbox, abstract-combobox, and pagination specs to reach their remaining branches — closeDetail(), mobile nav toggle/resize, updateLabel's throw path, ArrowUp/ArrowDown and focus/blur tab-order wiring, and the pageSize setter's numeric-string coercion. Refs #669

date-range: adds the valid start-only/end-only validator cases, the partially-invalid required combinations, fromRequired/toRequired driven independently of required, non-submit/reset form-service events, the defaultValidations-off path, empty-model emission, and writeValue's non-object and no-date guards. upload-v2: covers writeValue both ways, initilizeFileCtrl with explicit isSecure/postedDate, doUpload skipping non-Initial files, the name-edit toggle-off and default-overwrite paths, removal with no matching model entry, removeFileFromList retaining the input value, a plain HttpRequest deleteRequest, and setElementId's unknown-property guard. Refs #669

Wrappers: covers the hint clamp/overflow matrix (setOverflow/setHeight across showToggle, showFullHint, and toggleOpen), calcToggle with and without a hint container, onResize's recalc reset, setInputLabelElement's no-input and remove-attribute paths, and the non-string error message fall-through. calcToggle tests stub calculateNumberOfLines because jsdom reports offsetHeight as 0 — that method is the seam between layout and the toggle decision. sam-telephone: covers ngOnChanges ignoring non-countryCode changes and treating a missing code as North American, validate() for valid/invalid USA and international numbers and for an empty control, onKeyInput's allow and block paths, and the no-target guards on the input handlers. Refs #669

Crosses the 90% branch threshold. image: covers the not-editable edit toggle, save with nothing staged, long-name truncation and the label fallback, done-text and src precedence, and hideEditButton. textarea: covers the one-shot IE placeholder pristine workaround (reaching the private UA gate directly), non-submit/reset form-service events, the singular 'character' wording, and the hidden-counter guard. page.service: covers wideSidebar set before and without a sidebar. New specs for faWarnIfIconHtmlMissing, faWarnIfIconSpecMissing, and objectWithKey. Refs #669

New spec for HierarchicalTreeSelectedItemModel covering single vs multiple selection modes, duplicate-key rejection, bulk add, removal of present and absent items, membership checks, clear, and replaceItems. Extends the list-results-message spec with the partial-last-page clamp. Refs #669

The new specs pushed the root ESLint warning baseline from 1619 to 1800, almost entirely @typescript-eslint/no-explicit-any from test doubles and private-member access. Rewrites those: (component as any).method() -> component["method"]() { ... } as any -> { ... } as never (cb: any) => ... -> (cb: unknown) => ... Bracket access reaches private/protected members without disabling type checking, and 'never' is assignable to any parameter slot, so a stub literal still says "deliberately unchecked test double" without the rule violation. Also tightens the let/const usage on new lines. Root baseline is now 1581 warnings, below the recorded 1619 — the baseline file is deliberately left unbumped, since lowering it is a separate 'npm run lint:baseline:bump' change per the repo's ratchet rule. Refs #669

Raises the floor to the coverage now measured on master: statements 88.73% -> 95.60% branches 78.65% -> 90.09% functions 85.90% -> 92.78% lines 88.72% -> 95.67% All four metrics now clear the 90% QASP target, with branches — the metric that lagged furthest behind — crossing it for the first time. Generated by 'npm run coverage:bump' and committed on its own, per the ratchet rule in AGENTS.md. Closes #669

warn-if-icon-missing.spec.ts and object-with-keys.util.spec.ts were written and passing locally but never tracked: a machine-level ~/.gitignore_global has a bare 'Icon' rule (the classic macOS 'Icon\r' entry) which, with core.ignorecase=true, matches the whole src/ui-kit/experimental/icon/ directory. CI therefore ran 171 spec files instead of 173 and measured statements 95.53 / branches 89.99 / lines 95.60, just under the floor this branch committed from a local run that did include them. Forced in with 'git add -f'. Refs #669

CI measured branches at 90.07% against a floor of 90.09% while the same commit measured 90.09% locally, with 173/173 files and 1881 tests passing in both. Diffing the CI coverage artifact against the local one isolated it to a single branch: autocomplete-cache.ts:160, the arraysEqual() short-circuit in updateDefault(). No spec targeted that branch. It was only ever reached incidentally, via autocomplete-multiselect.spec.ts's debounced service fetch, which inserts into the default cache twice under fake timers. Whether the second insert lands before the coverage snapshot depends on worker scheduling, so the branch was covered on this machine and uncovered on CI's. Asserts the no-op-duplicate-insert behavior in autocomplete-cache.spec.ts where it belongs, making the branch's coverage independent of cross-file execution order. Refs #669

Two more branches were only covered incidentally by whichever spec file ran first, so total coverage moved with worker scheduling. Verified by re-running the suite under --sequence.shuffle, which reproduced CI's 90.07% locally and pointed at these two files. key-helper.spec.ts's getKeyCode() tests shared one mutated 'mock' object and asserted against the same "asdf" string in every case. Once the first test had set 'key', the code/keyIdentifier cases still returned 'key' and their assertions passed anyway — the fallback branches they name were never actually exercised. Each case now builds its own event stub with distinguishable values, and a case for all-three-missing was added. overlay-ref.ts's _attachBackdrop() defers its fade-in class to requestAnimationFrame. Nothing stubbed rAF, so whether jsdom flushed the frame before the coverage snapshot depended on how busy the event loop was. rAF is now captured and invoked explicitly, covering both the happy path and the guard for a backdrop detached within the same frame. Coverage is now stable at branches 90.14% across three consecutive shuffled runs, up from 90.09% and above the committed floor. Refs #669

Addresses PR #680 review. Each of these was a real bug that a new spec had documented rather than caught. MdTabNav.ngOnDestroy() unconditionally called _resizeSubscription.unsubscribe(), but nothing in MdTabNav ever assigns that field — only a subclass that subscribes to window.resize would. Confirmed by instantiating a plain MdTabNav and destroying it: 'Cannot read properties of undefined (reading unsubscribe)'. The existing spec passed only because it injected a Subscription first. Teardown is now optional-chained, with a test that creates and destroys a fresh nav. Paginator._exceedsTotal() tested 'r > 0 && r > unitsPerPage', which let the first empty page through whenever the total was an exact multiple of the page size — 100 units at 10 per page accepted page 11. A page is out of range once its first unit is past the end of the data, i.e. remainder >= unitsPerPage, with page 1 still valid for an empty data set. Verified the new predicate against the old one across exact-multiple, partial-final-page, empty and single-page totals. showResultsFreeText() passed item[0] — one sub-item — to findItemExistInList(), which iterates a list. A nested category sub-list containing an exact match therefore reported no match, and the component offered a duplicate free-text option alongside the real one. Now passes the sub-list. Behaviour is unchanged for every other list shape (flat item objects skip the guard entirely; strings and non-matching sub-lists return false either way). Also strengthened three assertions that were weaker than their names: - accordion-item's deregistration test destroyed an already-collapsed, parentless item, so no notification could have changed it even with the listener still attached. It now keeps the item expanded in a non-multi accordion and notifies a different id in that same accordion, plus a control case proving that notification does collapse a live item. - listbox's onChecked() ordering test now says explicitly that inserting the option object while isChecked()/setSelectedItem() compare against option.value is a latent inconsistency, not intended behaviour. Not changed here: the model is @Input/@Output-visible and two pre-existing tests already pass option objects through it. - autocomplete's requestSuccess() tests are renamed to describe the real contract. It is the httpRequest paging path: each emission is a page appended to what is displayed, de-duplicated per-payload against lastReturnedResults but not per-item, so overlapping pages do repeat. The 'same data twice' test now also asserts results, not just lastReturnedResults, so the guard is actually verified. Branches 90.14% -> 90.17%; 1891 tests pass. Refs #669

…qasp-target-bra Resolves conflicts in: - src/ui-kit/experimental/hierarchical/autocomplete/autocomplete.component.spec.ts - src/ui-kit/experimental/listbox/listbox.component.spec.ts - src/ui-kit/experimental/patterns/layout/components/page/page.component.spec.ts - src/ui-kit/form-controls/sam-sds-autocomplete/autocomplete-search/autocomplete-search.component.spec.ts Also fixes a resulting test failure in overlay.spec.ts: master's Overlay constructor dropped the ComponentFactoryResolver param (Angular DomPortalOutlet migration), so the mock construction args were updated to match the new 6-arg signature.

…ing-coverage-gap-to-90-qasp-target-bra # Conflicts: # src/ui-kit/experimental/hierarchical/autocomplete/autocomplete.component.spec.ts # src/ui-kit/experimental/listbox/listbox.component.spec.ts # src/ui-kit/experimental/patterns/layout/components/page/page.component.spec.ts # src/ui-kit/form-controls/sam-sds-autocomplete/autocomplete-search/autocomplete-search.component.spec.ts

fpigeonjr added the maintenance Repo maintenance / tooling label Sep 2, 2026

fpigeonjr self-assigned this Sep 2, 2026

fpigeonjr requested a balanced review from Copilot September 2, 2026 19:50

Copilot started reviewing on behalf of fpigeonjr September 2, 2026 19:50 View session

Copilot AI reviewed Sep 2, 2026

View reviewed changes

fpigeonjr marked this pull request as ready for review September 2, 2026 20:39

fpigeonjr requested a review from a team as a code owner September 2, 2026 20:39

fpigeonjr added 15 commits September 2, 2026 17:16

fpigeonjr force-pushed the gh-669-close-remaining-coverage-gap-to-90-qasp-target-bra branch from 81916e8 to 445276c Compare September 2, 2026 22:19

fpigeonjr requested review from a team and christyhermansen as code owners September 10, 2026 15:54

Navigation Menu

Uh oh!

Raise test coverage to the 90% QASP target on all four metrics - #680

Raise test coverage to the 90% QASP target on all four metrics#680
fpigeonjr wants to merge 17 commits into
masterfrom
gh-669-close-remaining-coverage-gap-to-90-qasp-target-bra

fpigeonjr commented Sep 2, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

fpigeonjr Sep 2, 2026

Uh oh!

Uh oh!

Uh oh!

fpigeonjr Sep 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

fpigeonjr commented Sep 2, 2026

Description

Approach

Two things worth a reviewer's attention

Previously-skipped tests now enabled

Motivation and Context

Type of Change (Select One and Apply Label)

How to Test

Screenshots (if appropriate)

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (3)

Uh oh!

Uh oh!

fpigeonjr Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fpigeonjr Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants