Add specs for remaining experimental components - #660
Open
fpigeonjr wants to merge 2 commits into
Open
Conversation
Adds specs for the under-covered experimental/* components tracked by issue #636: presentational primitives (box, container, label, layout, list, youtube, title, actions-list, button-next, alert), tabs (tab-group/tab-header/tab-body), sidenav, picker/popover, dollar, search, video-player, listbox, hierarchical (component + model classes), and the date-range-v2 datepicker. Fixes two real bugs surfaced while writing specs: - tabs/tab-header.ts, tabs/tab-group.ts: replaced a legacy RxJS 5 "unbound operator" pattern (auditTime.call(fromEvent(...), 10)) that crashed under RxJS 7 on any render of MdTabGroup, with proper pipe() usage. - video-player.component.ts: guarded ngOnDestroy against a null px-video-aria-announce element instead of throwing. All new specs exercise components through their public API only.
There was a problem hiding this comment.
Pull request overview
This PR significantly increases unit-test coverage across the remaining under-covered src/ui-kit/experimental/* components (per #636), and includes a couple of small runtime fixes discovered while adding specs (notably RxJS operator usage in tabs and a defensive guard in video-player teardown).
Changes:
- Added/expanded Vitest specs for many experimental components (presentational primitives, picker/popover, search, dollar, sidenav, tabs, etc.).
- Updated tabs implementation to replace legacy RxJS “unbound operator” usage with RxJS 7-compatible
.pipe(...)usage. - Hardened
SamVideoPlayerComponent.ngOnDestroyagainst missingpx-video-aria-announceelement.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ui-kit/experimental/youtube/youtube.spec.ts | Adds baseline behavior coverage for YouTube embed URL + iframe rendering. |
| src/ui-kit/experimental/video-player/video-player.spec.ts | Adds initialization, fullscreen, and teardown coverage for video player. |
| src/ui-kit/experimental/video-player/video-player.component.ts | Adds null-guard in ngOnDestroy for aria-announce element removal. |
| src/ui-kit/experimental/title/title.spec.ts | Adds rendering/utility coverage for heading selection + class composition. |
| src/ui-kit/experimental/tabs/tab-header.ts | Fixes RxJS resize stream to use .pipe(auditTime, startWith) rather than legacy operator calls. |
| src/ui-kit/experimental/tabs/tab-header.spec.ts | Adds keyboard/focus behavior coverage for tab header. |
| src/ui-kit/experimental/tabs/tab-group.ts | Updates selectedIndexChange derivation to RxJS .pipe(map(...)). |
| src/ui-kit/experimental/tabs/tab-group.spec.ts | Adds selection/focus/id/boolean-coercion coverage for tabs-next group. |
| src/ui-kit/experimental/tabs/tab-body.spec.ts | Adds position/origin + animation lifecycle coverage for tab body. |
| src/ui-kit/experimental/sideNavigationToolbar/sideNavigationToolbarItem/sideNavigationToolbarItem.component.spec.ts | Expands item-level open/close/click behavior coverage. |
| src/ui-kit/experimental/sideNavigationToolbar/sideNavigationToolbar/sideNavigationToolbar.component.spec.ts | Expands accordion coordination coverage between toolbar items. |
| src/ui-kit/experimental/search/search.spec.ts | Adds debounced search + cfda mode + selection behavior coverage. |
| src/ui-kit/experimental/picker/popover.spec.ts | Adds coverage that popover constructs an AbstractGrid. |
| src/ui-kit/experimental/picker/picker.spec.ts | Adds combobox wiring + click/search/clear behavior coverage (and no-op path). |
| src/ui-kit/experimental/patterns/layout/components/sidenav/sidenav.spec.ts | Adds extensive sidenav/container behavior coverage (toggle promises, escape/backdrop, styles). |
| src/ui-kit/experimental/listbox/listbox.component.spec.ts | Expands CVA/writeValue/disabled filtering + checked state coverage. |
| src/ui-kit/experimental/list/list.spec.ts | Adds baseline class composition coverage for list + list-item bullet rendering. |
| src/ui-kit/experimental/layout/layout.spec.ts | Adds baseline class composition coverage for layout primitives. |
| src/ui-kit/experimental/label/label.spec.ts | Adds baseline class composition coverage for label-next. |
| src/ui-kit/experimental/hierarchical/models/SamHierarchicalTreeGridConfiguration.spec.ts | Adds baseline model default/assignment coverage. |
| src/ui-kit/experimental/hierarchical/models/SamHierarchicalTreeConfiguration.spec.ts | Adds baseline model default/assignment coverage. |
| src/ui-kit/experimental/hierarchical/hierarchical/hierarchical.component.spec.ts | Un-skips and updates modal-related tests. |
| src/ui-kit/experimental/dollar/dollar.spec.ts | Adds extensive conversion/formatting/form-service/destroy coverage for dollar control. |
| src/ui-kit/experimental/date-range-v2/datepicker/picker.spec.ts | Adds extensive rendered + isolated coverage for datepicker behavior/validators. |
| src/ui-kit/experimental/date-range-v2/datepicker/picker.component.ts | Adjusts @ViewChild(calendarpopup) to non-static to match *ngIf usage. |
| src/ui-kit/experimental/container/container.spec.ts | Adds baseline class composition coverage for container. |
| src/ui-kit/experimental/button-next/button.spec.ts | Expands coverage for size/theme/disabled composition and click emitting. |
| src/ui-kit/experimental/box/box.spec.ts | Adds baseline class composition coverage for box. |
| src/ui-kit/experimental/alert/alert.spec.ts | Replaces commented/skip-only alert spec with working coverage for type selection + SR text. |
| src/ui-kit/experimental/actions-list/actions-list.spec.ts | Adds coverage for rendering, click behaviors, showMore collection, and dropdown dispatch. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- listbox.component.spec.ts: fix "reuslts" -> "results" test name typo. - video-player.component.ts: rename pxAnounce -> pxAnnounceEl and guard the parentNode.removeChild fallback against a null parentNode. - tab-header.spec.ts / tab-group.spec.ts: drive keyboard-navigation specs through real DOM keydown events dispatched at the tab list container instead of calling the underscored internal methods (_handleKeydown, _focusChanged, _getTabLabelId, _getTabContentId) directly, matching the public-API-only spec convention. Label/content id coverage now asserts against the rendered DOM attributes instead. The video-player.spec.ts InitPxVideo mocking concern was investigated: globalThis.InitPxVideo is read via a `declare var InitPxVideo: any` ambient reference at the call site, which Vitest/esbuild resolve against the global scope at call time (not the sibling `import *` namespace, which is unused dead code left over from an earlier attempt at typed access). Confirmed by removing the beforeEach mock entirely: the component throws "InitPxVideo is not defined" without it, and all assertions against initPxVideoSpy still pass with it in place, so the existing mock is effective. No change made there.
fpigeonjr
marked this pull request as ready for review
August 28, 2026 19:27
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
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.
Description
Raises unit-test coverage for the remaining under-covered
experimental/*components called out in #636.box,container,label,layout(incl.SamLayoutImgComponent/SamLayoutContentComponent),list(incl.SamListItemComponent),youtube,title,actions-list. All previously had zero or no spec coverage; now 100% lines/statements on each.alert.spec.ts— the pre-existing spec file was fully commented out /it.skip-only. Rewrote it against current infra (SamIconsModule) covering default/known/unknown type selection,typeNotDefined, and screen-reader text rendering.button-next/button.spec.ts— expanded existing coverage to include size/theme/disabled class composition andonClickemit/no-emit branches.sideNavigationToolbar/sideNavigationToolbarItem— expanded existing specs.picker/picker.spec.ts+picker/popover.spec.ts(new) — cover the combobox wiring betweensam-picker, an<input>, andsam-popover, including the "no input/popover present" no-op path.tabs/(new specs fortab-group.ts,tab-header.ts,tab-body.ts) — while writing a rendering spec forMdTabGroup(which always embedsMdTabHeader), discovered a real bug:tab-header.tsused a legacy RxJS 5 "unbound operator" pattern (auditTime.call(fromEvent(window, "resize"), 10)), which throws synchronously under RxJS 7 on any render ofsam-tabs-next— in tests and in a real browser alike. Fixed by switching to proper.pipe()usage intab-header.tsandtab-group.ts, then added specs for keyboard navigation, focus/select events, position/origin state, and the centering animation lifecycle.patterns/layout/components/sidenav/sidenav.ts(new spec) — covers open/close toggle promises via_onTransitionEnd, Escape-to-close (respectingdisableClose), align-change emission, mode class flags, container-level open/close delegation, backdrop click, and the duplicate-alignguard.dollar/dollar.component.ts(new spec) — covers dollar/number string conversion, currency rounding, focus/blur formatting round-trip, max-length key blocking,SamFormServicesubmit/reset wiring, and destroy cleanup.search/search.component.ts(new spec) — covers the debounced autocomplete search,closeAutocomplete,inputFocus, thecfdatab-search mode enter/exit, backspace clearing, and select-change handling.video-player/video-player.component.ts(new spec) — coversInitPxVideoinitialization with the required@ContentChildren present, default title/seek-interval,onFullScreenChangeemission, and missing-content-child error logging. Discovered and fixed a second real bug:ngOnDestroythrewCannot read properties of null (reading 'parentNode')when thepx-video-aria-announceelement didn't exist; added a guard.listbox/listbox.component.spec.ts— expanded with disabled-option filtering inwriteValue, empty-model default, andisChecked. (A pre-existing@ViewChild({static: true})bug reaching into an*ngTemplateOutlet-created view — matching existingit.skiptests in the file — was left as-is; it's a deeper architectural issue out of scope for this PR.)hierarchical/hierarchical.component.spec.ts— un-skipped 3 previouslyit.skipped modal tests that were skipped due to incomplete test setup, not a product bug.hierarchical/models/SamHierarchicalTreeConfiguration.spec.ts,SamHierarchicalTreeGridConfiguration.spec.ts(new) — cover default values and field assignment for these previously-untested model classes.date-range-v2/datepicker/picker.component.ts(new spec) — covers day-name ordering, year validation, date-range validity checks, day filtering, color/hover state helpers, calendar open/close (including outside-click and calendar-button-click handling), month navigation with range clamping, day selection, year submission, input/calendar sync, and the staticdateValidationvalidator. Loosened an overly-strict@ViewChild(..., { static: true })oncalendarpopupto non-static, matching its actual*ngIf-gated template usage (it's only referenced defensively in code, so this has no behavioral effect, only test-observability).All new/expanded specs exercise components through their public API (inputs, outputs, DOM,
ControlValueAccessormethods) — no reaching into private members except through explicit, narrowly-typed test-only interfaces used to assert on documented internal state (e.g. animation position/origin), matching the existing spec conventions in this repo.Motivation and Context
Closes #636
Type of Change (Select One and Apply Label)
bugfixlabelenhancementlabelbreakinglabelmaintenancelabelHow to Test
npm ci && npm ci --prefix test-appnpm --prefix test-app test— runs the full spec suite via Vitest with coverage.npm run coverage:check— confirms the repo-wide ratcheting coverage floor still passes (it improves).npm run lint:baseline— confirms no new ESLint warnings beyond the existing baseline.npm run format:check— confirms Prettier formatting.cd test-app && npm run build— confirms the app still builds.Expected result:
Test Files 125 passed (125)/Tests 1137 passed (1137). Per-component line coverage increases measurably from baseline for every file listed in #636, e.g.box.component.ts/container.component.ts/label.component.ts/youtube.component.ts0% → 100%,sidenav.ts~1% → 85%,tab-group.ts/tab-header.ts/tab-body.tslow-single-digit% → 91–100%,dollar.component.ts~0% → 96.66%,search.component.ts~0% → 94.73%,video-player.component.ts~0% → 96.29%,date-range-v2/datepicker/picker.component.ts36.45% → 95.07%. Repo-wide coverage gate passes well above floor (lines 77.88%vs. floor52.84%). Lint baseline gate passes at1591warnings (baseline1619, i.e. it improved).format:checkpasses cleanly.Screenshots (if appropriate)
N/A — test-only changes plus two defensive bug fixes with no visible UI impact.
Checklist
gh-<number>-<slug>)format:checkpasses (npm run format:check)lintpasses (npm run lint)buildpasses (cd test-app && npm run build)cd test-app && npm test)