Repository navigation
Test: Add the expectCurrentOrBetter comparator to the docgen harness - #35561
valentinpalkovic merged 22 commits into
Conversation
The harness now enforces "current or better" mechanically. src/compare/ parses the committed argtypes snapshots (pretty-format text, not JSON), compares argTypes per key - nothing recorded may be lost, and a type may change only by normalized deep equality or an enumerated improvement - and compares snippets by represented-name sets, so formatting can never fail a snippet. Both recorders read each committed baseline before its snapshot call and self-compare every fixture; a flip re-record therefore surfaces regressions while improvements pass. Acceptance stays the reviewed snapshot update - the committed baseline is the allowlist.
Three fixes from a fresh-context review round: the Vue attribute and root-element scanners now skip single-quoted values like double-quoted ones (value content could fabricate representations and mask a dropped binding); the Angular matchers parse the element structurally instead of regexing the whole string (binding-shaped text inside a value no longer counts, and quote style or spacing around = no longer fails); tuples compare positionally; and a failed member-set superset falls through to the structural rules so a catch-all union member can still improve. Also notes the first-record double-run in the README: the snippet inventory guard reads the fixture dir before vitest flushes file snapshots, so a brand-new fixture fails once and passes on the second run.
The value-close rule (quote, comma, newline) can be satisfied by multi-line prose - a JSDoc description whose line ends with a quoted term and a comma reads as an entry boundary and fabricates keys with no error, reproduced through the real vue-docgen-api pipeline. Local lookahead cannot disambiguate prose from structure, so the parser now re-serializes every parse in the corpus grammar and throws unless the bytes match the input. All 45 committed baselines round-trip byte-identically; hand-edits that survive as a canonical reading are rejected by the recorders' parsed-vs-live check instead.
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe docgen harness now parses committed argTypes snapshots, compares argTypes and Angular/Vue snippet representations using current-or-better rules, aggregates violations, and applies these checks to baseline tests before snapshot updates. ChangesDocgen comparator
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BaselineTests
participant SnapshotParser
participant expectCurrentOrBetter
participant compareArgTypes
participant compareSnippet
BaselineTests->>SnapshotParser: Parse committed argTypes baseline
BaselineTests->>expectCurrentOrBetter: Submit baseline and candidate
expectCurrentOrBetter->>compareArgTypes: Compare argTypes
expectCurrentOrBetter->>compareSnippet: Compare snippet representations
compareArgTypes-->>expectCurrentOrBetter: Return argTypes violations
compareSnippet-->>expectCurrentOrBetter: Return snippet violations
expectCurrentOrBetter-->>BaselineTests: Throw aggregated violations
Possibly related PRs
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
code/lib/docgen-harness/src/compare/argtypes.ts (1)
100-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop acceptance-criteria/ticket codes from comments. These comments encode requirement/AC codes and issue references that the guideline disallows; keep the maintenance rationale, drop the codes.
code/lib/docgen-harness/src/compare/argtypes.ts#L100-L100: remove theAC1reference, keep the "one wrapper level deep" rationale.code/lib/docgen-harness/src/compare/parse-snapshot.ts#L192-L192: removeD1, phrase as "turns every such misparse into a loud failure".code/lib/docgen-harness/src/compare/argtypes.ts#L9-L9: drop the#28706ticket ref, keep the "legacy Angular hardcodesrequired: true" rationale.code/lib/docgen-harness/src/compare/argtypes.test.ts#L500-L500: drop the#28706ticket ref, keep the rationale.As per coding guidelines: "Comments should explain maintenance rationale, not investigation history; do not include internal ticket or acceptance-criteria codes, provenance claims, or cross-file line references."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/lib/docgen-harness/src/compare/argtypes.ts` at line 100, Remove acceptance-criteria and ticket references from the comments while preserving their maintenance rationale: in code/lib/docgen-harness/src/compare/argtypes.ts lines 100-100, remove “AC1” but retain the one-wrapper-level explanation; in code/lib/docgen-harness/src/compare/parse-snapshot.ts lines 192-192, remove “D1” and describe the behavior as turning each misparse into a loud failure; in code/lib/docgen-harness/src/compare/argtypes.ts lines 9-9 and code/lib/docgen-harness/src/compare/argtypes.test.ts lines 500-500, remove “#28706” while retaining the rationale about legacy Angular hardcoding required true. Do not add cross-file references or investigation-history details.Source: Coding guidelines
code/lib/docgen-harness/src/angular/angular-baselines.test.ts (2)
45-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
snapshotsRewriteOnMismatchhelper (and its rationale comment) across both baseline test files. Both files define the byte-identical function that reaches into the undocumented__vitest_worker__global to detect-umode; Vitest's public API (vitest.state,snapshotOptionsconfig) does not expose this as a supported surface, so both copies share the same upgrade-fragility risk, and any future tweak must be kept in sync manually.
code/lib/docgen-harness/src/angular/angular-baselines.test.ts#L45-L59: extract this helper (and its comment) into a shared module (e.g. undersrc/compare/or a small test-support file) and import it here.code/lib/docgen-harness/src/vue3/vue3-baselines.test.ts#L28-L41: import the same shared helper instead of redefining it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/lib/docgen-harness/src/angular/angular-baselines.test.ts` around lines 45 - 59, Extract the duplicated snapshotsRewriteOnMismatch helper and its rationale comment into one shared test-support module, preserving its existing behavior and symbol. In code/lib/docgen-harness/src/angular/angular-baselines.test.ts lines 45-59, remove the local definition and import the shared helper; make the same replacement in code/lib/docgen-harness/src/vue3/vue3-baselines.test.ts lines 28-41 so both files use the single implementation.
103-140: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNon-null assertions on possibly-null
extractArgTypesresults mask extraction failures with an unhelpful crash instead of a clear violation. In both files,extractArgTypescan return a falsy value, but the candidate is force-unwrapped with!right before being handed toexpectCurrentOrBetter; if extraction were ever null while a committed baseline exists,compareArgTypeswill throw a raw TypeError from indexing intonull/undefinedrather than a named comparator violation.
code/lib/docgen-harness/src/angular/angular-baselines.test.ts#L103-L121: replacecandidate: argTypes!with an explicit null check (e.g. skip/fail with a clear message) before callingexpectCurrentOrBetter.code/lib/docgen-harness/src/angular/angular-baselines.test.ts#L123-L140: same forcandidate: filteredArgTypes!.code/lib/docgen-harness/src/vue3/vue3-baselines.test.ts#L65-L84: same forcandidate: argTypes!.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/lib/docgen-harness/src/angular/angular-baselines.test.ts` around lines 103 - 140, Replace the non-null assertions passed to expectCurrentOrBetter in code/lib/docgen-harness/src/angular/angular-baselines.test.ts lines 103-140 for both argTypes and filteredArgTypes, and in code/lib/docgen-harness/src/vue3/vue3-baselines.test.ts lines 65-84, with explicit null checks that fail clearly or skip comparison before invoking the comparator; preserve normal comparisons for valid extraction results.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/lib/docgen-harness/src/angular/angular-baselines.test.ts`:
- Around line 156-170: Update the snippet comparison block around
computesTemplateSourceFromComponent so expectCurrentOrBetter is called only when
snippet is not null, while preserving the existing committedSnippet guard and
snapshot assertion.
---
Nitpick comments:
In `@code/lib/docgen-harness/src/angular/angular-baselines.test.ts`:
- Around line 45-59: Extract the duplicated snapshotsRewriteOnMismatch helper
and its rationale comment into one shared test-support module, preserving its
existing behavior and symbol. In
code/lib/docgen-harness/src/angular/angular-baselines.test.ts lines 45-59,
remove the local definition and import the shared helper; make the same
replacement in code/lib/docgen-harness/src/vue3/vue3-baselines.test.ts lines
28-41 so both files use the single implementation.
- Around line 103-140: Replace the non-null assertions passed to
expectCurrentOrBetter in
code/lib/docgen-harness/src/angular/angular-baselines.test.ts lines 103-140 for
both argTypes and filteredArgTypes, and in
code/lib/docgen-harness/src/vue3/vue3-baselines.test.ts lines 65-84, with
explicit null checks that fail clearly or skip comparison before invoking the
comparator; preserve normal comparisons for valid extraction results.
In `@code/lib/docgen-harness/src/compare/argtypes.ts`:
- Line 100: Remove acceptance-criteria and ticket references from the comments
while preserving their maintenance rationale: in
code/lib/docgen-harness/src/compare/argtypes.ts lines 100-100, remove “AC1” but
retain the one-wrapper-level explanation; in
code/lib/docgen-harness/src/compare/parse-snapshot.ts lines 192-192, remove “D1”
and describe the behavior as turning each misparse into a loud failure; in
code/lib/docgen-harness/src/compare/argtypes.ts lines 9-9 and
code/lib/docgen-harness/src/compare/argtypes.test.ts lines 500-500, remove
“#28706” while retaining the rationale about legacy Angular hardcoding required
true. Do not add cross-file references or investigation-history details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb4eea71-2514-48bf-901b-afba9a53fe67
📒 Files selected for processing (13)
code/lib/docgen-harness/README.mdcode/lib/docgen-harness/src/angular/angular-baselines.test.tscode/lib/docgen-harness/src/compare/argtypes.test.tscode/lib/docgen-harness/src/compare/argtypes.tscode/lib/docgen-harness/src/compare/expect-current-or-better.test.tscode/lib/docgen-harness/src/compare/expect-current-or-better.tscode/lib/docgen-harness/src/compare/parse-snapshot.test.tscode/lib/docgen-harness/src/compare/parse-snapshot.tscode/lib/docgen-harness/src/compare/snippets.test.tscode/lib/docgen-harness/src/compare/snippets.tscode/lib/docgen-harness/src/compare/types.tscode/lib/docgen-harness/src/index.tscode/lib/docgen-harness/src/vue3/vue3-baselines.test.ts
isSnapshotUpdateRun says what the check answers; the old name described vitest's behavior in that mode instead. Comments tightened to the two load-bearing facts: why the round-trip proof skips -u runs, and why the worker global is the signal.
…patch Each framework's represented-names matcher now lives in its own snippets-<framework>.ts, with the shared root-element and attribute scanning in parse-element.ts. The dispatch in snippets.ts is an exhaustive switch over the Framework union: adding a framework fails compilation at the switch until its matcher exists (verified: extending the union to 'svelte' fails vue-tsc with TS2322 at that line).
The unfiltered and filtered argTypes blocks repeated the same read-record-parse-prove-compare sequence; a local recordArgTypes helper now carries it once, with a uniform flag reset. The read-if-exists ternary folds into readCommitted, the per-story action-arg scan hoists out of the loop, the one-file directory listing becomes an existence check, and isSnapshotUpdateRun moves to compare/ instead of living as a byte-identical copy in both recorders.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
code/lib/docgen-harness/src/compare/is-snapshot-update-run.ts (1)
1-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove environment-history claims from this comment.
Keep the durable rationale for bypassing the round-trip check during snapshot updates, but remove claims about Storybook replacing
expectand worker-global fallback behavior unless they are verified and intentionally documented as a supported contract.As per coding guidelines, comments should explain maintenance-relevant rationale, not investigation transcripts or provenance claims; verify environment and bundler assumptions empirically before encoding them in comments.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/lib/docgen-harness/src/compare/is-snapshot-update-run.ts` around lines 1 - 6, Rewrite the comment above the snapshot-update comparison logic to retain only the durable rationale: snapshot updates can intentionally make parsed and live output differ, so recorders skip the round-trip proof and re-enable it on the next normal run. Remove the claims about Storybook replacing the expect instance, reading worker configuration, and fallback behavior when the worker global is absent.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/lib/docgen-harness/src/compare/snippets-angular.ts`:
- Around line 16-20: Update the binding detection in the parseAttributeNames
loop to recognize Angular two-way binding syntax `[(name)]` and add its
normalized name to names, alongside the existing input and output binding
handling. Preserve the current normalization and behavior for the existing bound
formats.
In `@code/lib/docgen-harness/src/compare/snippets-vue3.ts`:
- Around line 40-44: Update the rawName normalization logic to preserve Vue
event bindings instead of returning undefined for names starting with '@'. Map
event bindings to the Vue docgen arg-name convention, while retaining the
existing handling for ':' bindings and excluding the other unsupported prefixes;
add coverage for the event-name mapping and lost-representation behavior.
---
Nitpick comments:
In `@code/lib/docgen-harness/src/compare/is-snapshot-update-run.ts`:
- Around line 1-6: Rewrite the comment above the snapshot-update comparison
logic to retain only the durable rationale: snapshot updates can intentionally
make parsed and live output differ, so recorders skip the round-trip proof and
re-enable it on the next normal run. Remove the claims about Storybook replacing
the expect instance, reading worker configuration, and fallback behavior when
the worker global is absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a8ea8706-1a47-447b-96fa-3bb4f5957d57
📒 Files selected for processing (10)
code/lib/docgen-harness/README.mdcode/lib/docgen-harness/src/angular/angular-baselines.test.tscode/lib/docgen-harness/src/compare/argtypes.tscode/lib/docgen-harness/src/compare/is-snapshot-update-run.tscode/lib/docgen-harness/src/compare/parse-element.tscode/lib/docgen-harness/src/compare/parse-snapshot.tscode/lib/docgen-harness/src/compare/snippets-angular.tscode/lib/docgen-harness/src/compare/snippets-vue3.tscode/lib/docgen-harness/src/compare/snippets.tscode/lib/docgen-harness/src/vue3/vue3-baselines.test.ts
💤 Files with no reviewable changes (1)
- code/lib/docgen-harness/src/compare/argtypes.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- code/lib/docgen-harness/README.md
- code/lib/docgen-harness/src/vue3/vue3-baselines.test.ts
- code/lib/docgen-harness/src/angular/angular-baselines.test.ts
- code/lib/docgen-harness/src/compare/parse-snapshot.ts
The element scanner's quote-aware loops are the two classic regex shapes (quote-alternation open tag, attribute name with optional quoted value), and the snapshot parser's string close rule reads directly as a lazy sticky pattern with a lookahead. Same behavior, pinned by the existing tests, the corpus sweep, and the byte round-trip verifier; about 90 lines shorter.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/lib/docgen-harness/src/compare/parse-snapshot.ts`:
- Around line 5-8: Update parseString to scan quoted tokens with escape
awareness rather than relying on the lazy KEY_STRING and VALUE_STRING regex
bodies, then decode escaped quotes, backslashes, and newlines before returning
the parsed string. Preserve key/value delimiter handling and add fixtures
covering each escaped-string case, including byte-round-trip validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 04069676-97ce-4f44-83c4-393460c0e490
📒 Files selected for processing (2)
code/lib/docgen-harness/src/compare/parse-element.tscode/lib/docgen-harness/src/compare/parse-snapshot.ts
Leads with how to run and how to add a fixture instead of conventions prose, compresses the comparator section to its rules, and keeps the gap and issue lists as tightened one-liners. Same facts, half the words.
Angular two-way bindings now read as both their input and change-output names ([(x)] is sugar for [x] + (xChange)); Vue event bindings map to their bare event names instead of being discarded, so a re-recorded baseline showing @save keeps its floor; and the Angular recorder asserts the generated snippet is not null before comparing it.
An `other`-typed baseline with an unquoted value passed against any
candidate at all. Half the corpus sits in that bucket, so the comparator
checked no type at all for `TreeNode`, `Array([object Object])`, or
`{ theme: string; dense: boolean }`. It only looked correct because both
sides come from the same engine today; the first differing engine would
have walked straight through. An unquoted `other` now accepts a
candidate that adds structure or resolves to the scalar it already
named. Three markers record nothing and still accept anything:
empty-enum, undefined, and the empty string.
The Vue matcher dropped directive modifiers and did not know the
`v-bind:` or `v-slot:` long forms, so a better engine spelling a binding
differently would have been read as losing it. Nested named slots left a
stray closing tag that counted as default content; the scan now finds a
block's end by depth.
Also: `CompareSnippetInput.args` was required everywhere and read
nowhere; an unparsable candidate reported one violation per baseline
binding instead of one broken snippet; a default value carrying a
newline split a violation across lines. Two comments claimed things that
are not true - `storybook/test` never reaches the recorders, and the
round-trip note ended mid-sentence.
Every fix is pinned by a test that fails when the fix is reverted.
What I did
This adds the
expectCurrentOrBettercomparator: the mechanical check that holds the upcoming OSA docgen engines to "current or better" against the recorded legacy baselines, instead of guessing. For argTypes, it enforces a per-key floor: nothing a committed baseline records may be lost, and a type may only change by normalized deep equality or an enumerated improvement (a catch-all becoming structured, a literal union gaining members). For snippets, it compares the represented binding names, so formatting can never fail the check, but a lost binding does.Both recorders now read each committed baseline before its snapshot call and self-compare every fixture on every run. Hence, a future flip re-record surfaces regressions with named violations while improvements sail through, and acceptance stays the reviewed snapshot update. The committed baseline is the allowlist; there is no extra allowlist file. The committed argtypes snapshots are pretty-format text, not JSON, so a small tolerant parser reads them back and verifies itself by re-serializing every parse byte-for-byte. Any misparse throws instead of silently loosening the floor.
How to review this
Start with
src/compare/argtypes.ts(the per-key floor and the type pass-list), thensrc/compare/snippets.tsplus its two framework matchers, then the wiring in the two*-baselines.test.tsfiles.parse-snapshot.tsis self-contained and best read last. Three examples show the intended behavior:1. Losing precision fails. A component author documents a prop like this:
The legacy pipeline records the three options, so the docs Controls panel renders a dropdown. If a new engine only reports
string, that dropdown would silently become a free-text field. The comparator catches exactly that:This also holds one wrapper level deep — a union whose members individually degrade from literals to
stringfails the same way.2. Gaining precision passes. Compodoc gives up on many Angular types today and records the
empty-enumcatch-all:If the new engine extracts a structured type instead, the comparator sees a catch-all becoming structured and reports zero violations. No baseline update, nothing to approve — better just passes.
3. Snippets: content over formatting. The "Show code" block is code users copy, so every binding the baseline shows must stay represented, while formatting is free:
The identity gate proves all of this against reality: every committed baseline (45 argtypes files, 41 snippets) self-compares clean on every run, and the parser round-trips each file byte-for-byte. Deliberately NOT compared:
required(legacy Angular recordsrequired: truefor everything, #28706),table.category,jsDocTags,control/action, and description/default contents — each would entrench a recorded lie or lateral engine vocabulary.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
yarn && yarn nx run-many -t compileyarn test code/lib/docgen-harness(from the repo root) — expect 176 tests: 152 passed, 24 expected fail (the red gap markers from Test: Scaffold docgen-harness and record Vue 3 legacy baselines #35545/Test: Record Angular legacy docgen and snippet baselines in the harness #35548).argtypes.snapshot(e.g.code/lib/docgen-harness/src/vue3/__testfixtures__/props-union-enum/argtypes.snapshot), runyarn test code/lib/docgen-harness -u— the fixture's test fails with a named violation and the file heals itself back. 🙂Documentation
code/lib/docgen-harness/README.md)MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsDeclare whether manual QA will be needed for this PR during the next release, through
qa:neededorqa:skipMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.Summary by CodeRabbit
Release Notes
Documentation
New Features
argTypesand framework-specific snippet baselines with aggregated, readable violation reporting.Tests
expectCurrentOrBettermatcher.