mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-26 14:30:13 +00:00
test(charts): cover the event-time context reads; correct two comment claims (TASK-3093 review r2)
Round 2 returned no P1: one P2 and one Nit, both about comment truth, plus one defect I found in my own new test while checking it. P2 — my round-1 "fix" to the absence claim replaced one unverified statement with another. I had written that on a fresh fixture workspace hasData is false so the layers never mount. Both halves are wrong, and I checked each at source this time rather than reasoning: the insights page gates the chart on `report` being non-null and on `noActivity`, never on the chart's own hasData; `zeroFilledBuckets` emits a bucket per day for the whole window regardless of activity, so once `report` exists data.length>0 and hasData is TRUE; and the e2e fixture is one SHARED workspace seeded from the `startup` template, not a fresh one. The sentence now states only what that spec does (asserts URL pathname and document.title, nothing about chart output) plus the thing I actually measured in this unit — serving the unmigrated v11 build in a real browser left the page loading with its title intact, throwing only pageerrors and rendering zero charts, which is why a title assertion cannot catch this class. Nit — theme.ts claimed the package casts through `unknown` "for the same reason". It does not; it gives two of its own (conditional ScaleFor types that don't compare across different S, and an object that starts empty and is filled by defineProperties). Corrected, along with the narrower point that `yScale` alone blocks the direct cast while `xScale` does not, since `bandwidth` is optional there. Found while checking my own work: the new hover leg did not test what its comment claimed. `bandCenter` and `bandSummary` are the only context reads outside a tracking context, and round 1 judged them correct by design — so this leg exists to check that rather than argue it. But BarChart clamps the tooltip's left edge into [w/2, canvasWidth - w/2], and the shim reported the same 600 for both, collapsing that interval to a point: a mutant dropping `k.padding.left` from bandCenter SURVIVED. The shim now reports a narrow width for the tooltip specifically, and the assertion derives the expected centre from the hit-rect's own x and width instead of a magic pixel. That mutant now dies by exactly 36px, which is padding.left. Mutation coverage for the unit now stands at 5 of 5: setup-time destructure in each of the three layers (killed only by the reactivity leg, with the static legs passing in all three), bandSummary's accessor, and bandCenter's padding term. Adds theme.ts's note that requiring yScale.ticks is only safe while BarChart hardcodes scaleLinear() and never exposes the scale as a prop. svelte-check 0 errors; vitest 229 files / 3201 tests passed. Claude-Session: https://claude.ai/code/session_01BLg7wiBeSgXvAhWi23mT4Z
This commit is contained in:
@@ -4,11 +4,15 @@
|
||||
// Nothing in the suite asserted anything about `BarChart`'s output. Its only
|
||||
// consumers are the insights page and its print route; no vitest suite rendered
|
||||
// either, and the one e2e spec that visits `/insights`
|
||||
// (page-title-survives-pane-and-nav.spec.ts:88) SPA-navigates there and checks
|
||||
// `document.title` alone — on a fresh fixture workspace the chart's `hasData`
|
||||
// is false, so the layers never even mount. So `Web` went green on a major bump
|
||||
// that rewrote the library's context API out from under the three chart layers.
|
||||
// The green was the enumeration (nothing looked), not the property (the charts
|
||||
// (page-title-survives-pane-and-nav.spec.ts:88) asserts the URL pathname and
|
||||
// `document.title` — nothing about chart output.
|
||||
//
|
||||
// A title assertion cannot catch this class of break, which was measured rather
|
||||
// than reasoned: serving the unmigrated v11 build in a real browser, the
|
||||
// insights page still loaded with its title intact and threw only `pageerror`s
|
||||
// while rendering zero chart elements. So `Web` went green on a major bump that
|
||||
// rewrote the library's context API out from under the three chart layers. The
|
||||
// green was the enumeration (nothing looked), not the property (the charts
|
||||
// work).
|
||||
//
|
||||
// This test renders the chart end-to-end through the real `<LayerCake>` — no
|
||||
@@ -22,11 +26,15 @@
|
||||
// under test.
|
||||
import { afterEach, beforeAll, describe, expect, it } from 'vitest';
|
||||
import { render, cleanup } from '@testing-library/svelte';
|
||||
import { tick } from 'svelte';
|
||||
|
||||
import BarChart from './BarChart.svelte';
|
||||
|
||||
const BOX_WIDTH = 600;
|
||||
const BOX_HEIGHT = 240;
|
||||
const TOOLTIP_WIDTH = 80;
|
||||
// BarChart's own padding literal; bandCenter offsets by its left edge.
|
||||
const PADDING_LEFT = 36;
|
||||
|
||||
beforeAll(() => {
|
||||
// LayerCake measures its container through `bind:clientWidth` /
|
||||
@@ -40,8 +48,15 @@ beforeAll(() => {
|
||||
// binding not to throw.
|
||||
Object.defineProperty(HTMLElement.prototype, 'clientWidth', {
|
||||
configurable: true,
|
||||
get() {
|
||||
return BOX_WIDTH;
|
||||
get(this: HTMLElement) {
|
||||
// The tooltip reports a NARROW width on purpose. BarChart clamps the
|
||||
// tooltip's left edge into [w/2, canvasWidth - w/2]; if the tooltip
|
||||
// measured the full canvas width that interval would collapse to a
|
||||
// single point and every position assertion below would hold no matter
|
||||
// what the layer computed. Measured: with both at 600 the hover leg
|
||||
// could not tell `bandCenter` apart from one with `k.padding.left`
|
||||
// dropped.
|
||||
return this.classList?.contains('tooltip') ? TOOLTIP_WIDTH : BOX_WIDTH;
|
||||
},
|
||||
});
|
||||
Object.defineProperty(HTMLElement.prototype, 'clientHeight', {
|
||||
@@ -174,4 +189,40 @@ describe('BarChart renders through LayerCake', () => {
|
||||
);
|
||||
expect(Math.max(...ticks)).toBeGreaterThanOrEqual(12);
|
||||
});
|
||||
|
||||
it('drives the hover tooltip from context read at event time', async () => {
|
||||
// `bandCenter` and `bandSummary` (Bars.svelte) are the only context reads
|
||||
// in the unit that happen OUTSIDE a tracking context — they run in a
|
||||
// pointer handler and want the value at event time rather than a
|
||||
// dependency. Review round 1 judged that correct by design; this leg is
|
||||
// what makes it checked rather than argued. It is also the only coverage
|
||||
// of `k.padding` anywhere in this file.
|
||||
const { container } = renderChart();
|
||||
|
||||
const hits = container.querySelectorAll('g.bars rect.hit');
|
||||
expect(hits.length).toBe(DATA.length);
|
||||
|
||||
// The accessible per-band summary is built from k.x plus the series values.
|
||||
expect(hits[1].querySelector('title')?.textContent).toBe(
|
||||
'Tue — Created 3, Completed 7'
|
||||
);
|
||||
|
||||
hits[1].dispatchEvent(new PointerEvent('pointerenter', { bubbles: true }));
|
||||
await tick();
|
||||
|
||||
const tooltip = container.querySelector('.tooltip') as HTMLElement | null;
|
||||
expect(tooltip).not.toBeNull();
|
||||
expect(tooltip?.textContent).toContain('Tue');
|
||||
expect(tooltip?.textContent).toContain('7');
|
||||
|
||||
// bandCenter = k.padding.left + k.xGet(d) + bandwidth/2. Both operands are
|
||||
// readable off the hit-rect itself (its x IS k.xGet(d), its width IS the
|
||||
// bandwidth), so this asserts the actual relation rather than a magic
|
||||
// pixel — and it fails if the k.padding.left term is dropped.
|
||||
const hitX = Number(hits[1].getAttribute('x'));
|
||||
const hitW = Number(hits[1].getAttribute('width'));
|
||||
const left = Number.parseFloat(tooltip!.style.left);
|
||||
expect(Number.isFinite(left)).toBe(true);
|
||||
expect(left).toBeCloseTo(PADDING_LEFT + hitX + hitW / 2, 5);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -58,15 +58,17 @@ export interface ResolvedSeries {
|
||||
* started reading it.
|
||||
*
|
||||
* Layers reach it as `getLayerCakeContext() as unknown as LayerCakeContext`.
|
||||
* The hop through `unknown` is required, not laziness. The package types every
|
||||
* scale as the loose `Scale` (`{ (value: any): any, [key: string]: any }`),
|
||||
* which DECLARES none of the methods the shapes above require, and an index
|
||||
* signature does not satisfy a required named member. So TS refuses the direct
|
||||
* cast — verbatim: "Conversion of type 'LayerCakeContext<any, ChartDatum[]>' to
|
||||
* type 'LayerCakeContext' may be a mistake because neither type sufficiently
|
||||
* overlaps with the other ... Property 'ticks' is missing in type 'Scale' but
|
||||
* required in type '{ (value: unknown): number; ticks: ... }'". The package
|
||||
* casts its own context through `unknown` for the same reason.
|
||||
* The hop through `unknown` is required, not laziness: the package types every
|
||||
* scale as `{ (value: any): any, [key: string]: any }`, and an index signature
|
||||
* does not satisfy a required named property — so `yScale` below, which
|
||||
* requires `ticks`, is judged non-comparable and TS2352 rejects the direct cast.
|
||||
* `xScale` is NOT what blocks it: `bandwidth` is optional there, so nothing it
|
||||
* requires is missing. Measured while narrowing this interface — when both
|
||||
* scales still required `domain`, the compiler named `xScale` first; dropping it
|
||||
* moved the error to `yScale`. The package also casts its own context through
|
||||
* `unknown`, but for its own reasons (it gives two: conditional `ScaleFor` types
|
||||
* that don't compare across different `S`, and an object that starts empty and
|
||||
* is filled by `defineProperties`) — not this one.
|
||||
*
|
||||
* What the hop gives up is only that one comparison, against a type that
|
||||
* asserts nothing. What this interface exists to check is the layer bodies, and
|
||||
@@ -83,7 +85,14 @@ export interface LayerCakeContext {
|
||||
(value: unknown): number;
|
||||
bandwidth?: () => number;
|
||||
};
|
||||
/** Linear scale: called for a position, and asked for `ticks()` by AxisY. */
|
||||
/**
|
||||
* Linear scale: called for a position, and asked for `ticks()` by AxisY.
|
||||
* `ticks` is REQUIRED, which is only safe because `BarChart` hardcodes
|
||||
* `scaleLinear()` and never exposes the y scale as a prop — a band scale has
|
||||
* no `ticks`. If the y scale ever becomes caller-supplied, this narrowing
|
||||
* turns into a lie the cast would hide, and AxisY needs the same `typeof`
|
||||
* probe the bandwidth reads use.
|
||||
*/
|
||||
yScale: {
|
||||
(value: unknown): number;
|
||||
ticks: (count?: number) => number[];
|
||||
|
||||
Reference in New Issue
Block a user