test(dom-leak): one attribute judge, shared by both gates (#4434) - #4499
Merged
Conversation
The "is this attribute HTML-defined" judge existed in two copies, inline in two test files that cannot import each other, and the copies had already diverged. Extract it once into a new private, never-published `@object-ui/test-support` package, carrying the union of what both copies knew, with the calibration fixtures moved next to it so it proves itself once instead of once per copy. Only the judge unifies: each gate keeps its own canary sets, targets, readiness selectors, ledger and assertions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
github-actions
Bot
added
documentation
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
`scripts/__tests__/quick-reference-commands-4149.test.ts` requires the `packages/*` row to name every private package under `packages/`, so that a published count smaller than the directory count is explained rather than "corrected" back (objectui#4149). Adding `packages/test-support` makes it the second one. The published count is unchanged at 38 — the new package is `private: true`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
Collaborator
Author
|
ACCEPT — step-7 复核 by PM session
Flipping ready + arming auto-merge. The DOM-leak infrastructure is now: one judge, one home, two gates consuming it — #4425's completion (metric rows + contract docs) is the family's last card. Generated by Claude Code Generated by Claude Code |
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
2 participants
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.
Closes #4434
The DOM-leak "is this attribute HTML-defined" judge existed in two copies, inline in two test files that cannot import each other. This extracts it once, carrying the union of what the two copies knew, and moves the calibration fixtures next to it so the judge proves itself once instead of once per copy.
Only the judge unifies. Each gate keeps its own canary sets, targets, readiness selectors, ledger and assertions — the only non-comment edits to either gate file are the import and, in the fields gate, five
leakReportcall sites re-spelled to the unified label parameter (they produce byte-identical messages).The home: measured, then chosen
Measured first — there is no existing convention to follow. Nothing in this repo ships shared test code across a package boundary today:
./testing/./test-supportsubpath in anyexportsmap (grep '"./test'over every manifest: zero hits);packages/components/src/__tests__/test-utils.tsx, is imported only by its own package's tests;grep "from '../../../../"over every*.test.ts(x): zero hits);@object-ui/fieldsdeep subpaths resolve only through the repo vitest alias — itsexportsmap publishes none of them #4325, which ruled out@object-ui/fields/widgets/MarkdownContent: a specifier only this repo's vitest alias could resolve, TS2882 fortsc, unresolvable outside the repo. A package's surface is its index.So this implements the smallest sound convention, against the three constraints:
./test-supportexport on a released packagepackages/fieldsandpackages/app-shelldevDependencyline eachprivate: true, never releasedexportsmap is public API whatever it is named, and it has to ship in the tarball to resolve@object-ui/test-support, resolved through its ownexportsmapConstraint (b) is what decides it, so:
packages/test-support,@object-ui/test-support,private: true.It resolves the same way for both toolchains, which is the #4325 lesson applied rather than restated:
tsc— through the packageexportsmap. Proof that this is not leaning on a repo-only alias:packages/app-shell/tsconfig.test.jsonsets"paths": {}precisely to force resolution through the workspace dependency, andtsc -p tsconfig.test.json --listFileslistspackages/test-support/src/dom-leak-judge.tsand.../src/index.tsas program inputs.tscreads instead of depending on how Vite treats a symlinked dependency.Home wiring, all of it: the package manifest/tsconfig/README, one
devDependenciesline in each consumer, one vitest alias, and one.changeset/config.jsonignoreentry (a never-published package must not be versioned;scripts/check-changeset-fixed.mjsrequires every workspace package to be classified as one or the other). Verified green:check-type-check-coverage(now 46/46 packages, 41 compiling their tests),check-lint-coverage(46/46),check-changeset-fixed,check:phantom-deps, andchangeset status— the last one matters because changesets rejects a released package depending on an ignored one, and it does not fire here since that check excludes devDependencies.The divergence the union absorbed
Diffing the two judge blocks (
fieldslines 134-304 againstapp-shelllines 184-330 on the merge base) gives three real divergences; everything else is comment prose.1. The SVG presentation list — ten entries the sweep had and the fields copy did not. This is the divergence the card was filed for: recharts markup the fields gate never renders.
Union = the sweep's superset. Kept as a labelled second block, because the two halves have different producers (lucide icons vs. recharts) and will drift on different upgrades.
2.
findLeakstruncated its records differently.Neither bound is load-bearing — both gates assert on attribute names, so these only shape the failure message. The union takes the defensive answer on each axis independently: the value is bounded at 80 (a leaked value can be an arbitrarily long serialized object), and the markup keeps the wider of the two bounds at 400 (the markup is what tells you which element in a 46-widget sweep produced the finding). Both are named constants with that reasoning next to them.
3.
leakReporthad two signatures.Unified on one
label. The fields gate passes`field:${type} [${variant}]`, so both gates emit byte-identical failure text to what they emitted before — the message format was not changed, only where the label is assembled.Everything else was identical in content:
OPEN_PREFIXES,GLOBAL_HTML_ATTRIBUTES,ATTRIBUTE_TO_IDL_ALIAS,HAPPY_DOM_IDL_GAPS(all five entries),idlPropertiesForandisKnownAttribute— the last two byte-for-byte.The calibration fixtures were also unioned, and this is where the extraction paid for itself immediately: the fields fixture was the superset of markup, the sweep fixture planted the SDUI injection names, and the ten recharts SVG attributes above had a clean-markup fixture behind them in neither file. The merged CLEAN fixture now carries a
defsblock exercising all ten; the merged PLANTED list is 21 attributes (15 from fields, 13 from the sweep, 7 shared), with the fields copy's exact-count assertion kept.Red-first
The calibration fixtures are the mechanism that catches happy-dom / library drift, so they have to be shown failing. Both directions were run by deleting one entry from the shared judge and restoring it byte-identically afterwards (verified by
diff -qagainst a pre-edit copy; nogit stashinvolved).Direction 1 — delete the
select[size]happy-dom IDL gap:Direction 2 — delete
markerwidth, i.e. one of the ten union-only recharts entries (the half that had no fixture behind it before this PR). Two cases go red, and the second one names the mechanism rather than the symptom:(The two angle brackets above carry a space so GitHub's body sanitizer does not eat them as HTML tags; the real assertion text has none.)
Both gates green through the shared import, full runs, repo-root vitest:
No judge logic left inline. Grepping both gate files for
isKnownAttribute|idlPropertiesFor|HAPPY_DOM_IDL_GAPS|SVG_ATTRIBUTES|OPEN_PREFIXES|GLOBAL_HTML_ATTRIBUTES|ATTRIBUTE_TO_IDL_ALIAS|function findLeaks|function leakReport|interface Leakreturns only docblock prose describing where the judge went — zero declarations. The sweep file keepsleakedAttributeNames, which was never duplicated: it is the ledger's unit, and it stays with the ledger.Verification
pnpm --filter '@object-ui/fields^...' --filter '@object-ui/app-shell^...' buildpnpm exec vitest run packages/test-support/ packages/fields/ packages/app-shell/ --maxWorkers=2pnpm run type-checkintest-support/fields/app-shelltsc -p tsconfig.test.jsonpnpm run lintin the same threeno-explicit-anyset)node scripts/check-control-bytes.mjscheck-type-check-coverage/check-lint-coverage/check-changeset-fixed/check:phantom-deps/check-changeset-no-major/check-doc-linkspnpm exec vitest run scripts/(after theQUICK_REFERENCE.mdfix below)d006a001bOne more piece of home wiring, found by CI
scripts/__tests__/quick-reference-commands-4149.test.tsrequiresQUICK_REFERENCE.md'spackages/*row to name every private package underpackages/, so a published count smaller than the directory count is explained rather than "corrected" back to the directory count — which is how objectui#4149 happened.packages/test-supportmakes it the second such package, so the row now names it too. The published count is unchanged at 38; the new package isprivate: true, which is exactly the property that ratchet is measuring.Second commit, one CI lap: the local run was scoped to the three affected packages, and this gate lives in
scripts/.Surface
Touched: the two gate test files (import swap + the moved calibration block), the new
packages/test-support, its home wiring (.changeset/config.json,vitest.config.mts,QUICK_REFERENCE.md, the two consumers'devDependencies,pnpm-lock.yaml), and one changeset.Not touched: ledger rows, canary sets, target sets,
MetricWidget/schemaHostProps, any renderer source,console/**,plugin-calendarsource, chart files,content/docs/releases/.Note on the changeset
The card expected "test-only, so none owed".
scripts/check-changeset-presence.mjsdisagrees and it is right: it deliberately has no carve-out for test files undersrc/, so editing the two gate files under released packages demands a declaration. The answer it names — and calls "a pass, not a workaround" — is an empty-frontmatter changeset declaring that this releases nothing, which is what.changeset/shared-dom-leak-judge.mdis. The gate now reports: "Every one of them has an EMPTY frontmatter — declared as releasing nothing, which is the explicit exemption and a complete answer to this gate."No
skip-changesetlabel was applied, because in this repo it does not exist: objectui#3724 deleted a doc page that described exactly that mechanism, recording that "neither the workflow nor the label was ever real". The empty-frontmatter changeset is this repo's mechanism.Generated by Claude Code