test(core): add T8 override-set merge semantics suite for getVariables

[vanceingalls]

What

Extends getVariables.test.ts with T8 from the SDK migration test plan: override-set merge semantics.

Tests (4 new)

• last-write-wins — calling setOverrides twice; second value wins
• sparse override — override one key, unmentioned declared defaults survive intact
• batch override (brand kit) — setting all keys at once via a single override object
• manual override after batch — replacing one key from a kit batch; others untouched

Scope note

Tests are labeled "flat-merge, current behaviour" — getVariables does {...defaults, ...overrides}. Dotted-key path resolution ("headline.color" as id.prop) is a future SDK concern; these tests validate the flat-merge contract that exists today, not the future path-resolution semantics.

Stack

Stacked on T1 (#1240). Prerequisite for T11 (#1242), T4 (#1243).

[vanceingalls]

• test(core): add T9 CompositionVariable font/image parse spec (before R1) #1246
• test(core): add T2 stable id spec for parse-to-hf id contract (before R1) #1245
• test(studio): add T4 op-contract stubs for editor dispatch boundary #1243
• test(studio): add T11 history coalescing and cross-prop separation suite #1242
• test(core): add T8 override-set merge semantics suite for getVariables #1241 👈 (View in Graphite)
• test(core): add T1 round-trip idempotence suite for parse/serialize #1240
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[miguel-heygen]

T8 suite cleanly documents current flat-merge semantics before R1 touches getVariables. The beforeEach/afterEach reset pattern matches the existing file convention.

One implicit contract worth calling out in a comment (not a blocker): the 4th test — "override covering only one key reverts uncovered keys to declared defaults" — is documenting that each setOverrides call replaces the overrides object wholesale (flat-merge), not deep-merges. If anyone reads "merge" and expects accumulation, they'll be confused. The test is correct; a one-liner inline comment like // setOverrides replaces entirely, not patches would help future readers.

✅ Approve.

[james-russo-rames-d-jusso]

Hey Vance — T8 lands four targeted assertions on the public setOverrides / getVariables contract. No blockers. The flat-merge / path-resolution scope split is exactly right; comments below are about naming + a small symmetry gap.

What I verified

Walked each test against "what R1 (or the SDK migration broadly) could silently break":

• Last-write-wins on setOverrides × 2 — catches "refactor accidentally merges successive override-sets instead of replacing." Load-bearing.
• Sparse override leaves declared defaults intact — catches "refactor stops falling back to declared defaults when override is partial." Load-bearing.
• Batch override (brand-kit) — basically a re-statement of last-write-wins with all keys at once. Slightly redundant with test 1 but cheap; useful as the brand-kit code-path is named explicitly.
• "Override covering only one key reverts uncovered keys to declared defaults" — see Concerns below; the test name doesn't quite match the assertion.

beforeEach + afterEach clearing both setDeclared + setOverrides is the right hygiene — each test is self-contained and the module-level state can't leak.

Concerns

• Test 4's name is mildly misleading. The name says "override covering only one key reverts uncovered keys to declared defaults" — implying there was a prior batch override that's being reverted. But the test calls setOverrides({ primary: "#manual" }) from a clean state (the beforeEach reset), so it's really just a sparse-override test that's structurally identical to test 2. The PR body framing ("manual override after batch — replacing one key from a kit batch; others untouched") is a different scenario: that would require setOverrides(batch) followed by setOverrides({primary: "#manual"}), and then asserting secondary falls back to declared default (NOT to the prior batch's secondary). Two cheap fixes:
  • (a) Rename test 4 to "sparse override falls back to declared defaults (no carry-over from prior override-set)" to match what's actually asserted, OR
  • (b) Add the missing prior setOverrides(batch) call so the test actually exercises the "after batch" scenario from the PR body — that's the more interesting contract because it pins "successive override-sets are replacements, not merges into the prior override-set."
  • Option (b) is the load-bearing one. Today, test 1 (last-write-wins on whole override-sets) and test 4 (sparse override) don't combine into a clear assertion that "sparse override after batch override doesn't carry over uncovered batch keys." That gap is the one the SDK migration will hit if anyone refactors getVariables into a "merge cascade" shape.

Nits

• Test 1 uses setOverrides({ color: "blue" }) then setOverrides({ color: "green" }) — both single-key. A stronger last-write-wins test would have the second call drop a key that was in the first call: setOverrides({ color: "blue", size: "large" }) then setOverrides({ color: "green" }), then assert size falls back to declared default. That'd lock both halves of "last write replaces, not merges." Cheap. (nit)
• No test asserts setOverrides(undefined) clears. The afterEach does this, but the documented behavior of the cleared state isn't pinned. Worth one assertion. (nit)
• brand kit test asserts override matches input verbatim, but doesn't assert only those keys are returned. If R1 adds extra keys in the merge (e.g. system defaults), toEqual would catch it — but a comment "exhaustive — no extra keys allowed" would make intent explicit. (nit)
• The [T8 — override-set merge semantics (flat-merge, current behaviour)] describe-block label is great — the "flat-merge, current behaviour" qualifier explicitly tells the next reader these tests will need rewriting if the SDK migration adds dotted-path semantics. Same convention worth carrying into the other 5 PRs. (nit, applies stack-wide)

Questions

• Once dotted-path overrides ("headline.color") land, what's the expected interaction with this flat-merge layer — does flat-merge stay as a fast path for non-dotted keys, or is it fully replaced by path-resolution? If the former, these tests survive unchanged. If the latter, they'll all need a .skip annotation in R1's first PR. Knowing now would inform whether to mark them with a // flat-merge — slated for replacement at <Tn> comment now.
• Is "declared defaults" the right phrase for what setDeclared(JSON.stringify([...])) accepts? The shape passed is the schema-shape {id, type, label, default}[] — the "default" inside each entry. A reader unfamiliar with the schema might read "declared defaults" and expect a simpler {id: value} map. A one-line comment near the setDeclared call would help.

What I didn't verify

• The HeyGenverse plan doc (HTTP 403 from my unauthenticated fetch). Trusted the PR body's T8 framing and the "future SDK path-resolution" scope split.
• Whether getVariables is read by code outside the studio editor — if it's used in render-time, the same regression net is needed at render-time too. Out of scope here; flagging.
• The setDeclared / setOverrides module-level state model (assumed it's process-local from the beforeEach / afterEach shape — if there's any worker-state, the tests pass in isolation but mask a real bug).

— Review by Rames D Jusso

[miguel-heygen]

Building on Rames's review.

Rames's test 4 concern is the right one: the name implies "after batch override, one key reverts" but the setup starts from a clean beforeEach state, so it's really just a sparse-override assertion structurally identical to test 2. The missing scenario — setOverrides(batch) → setOverrides({primary: "#manual"}) → assert secondary falls back to declared default, not the prior batch's value — is the contract that pins flat-replacement semantics against a future "merge cascade" refactor. That's the test worth adding.

setOverrides(undefined) behavior: afterEach relies on it to clear state, but there's no test asserting what getVariables() returns after a clear. If R1's refactor introduces conditional logic on undefined overrides, that behavior could silently change and the suite would still pass. One assertion: setDeclared(schema); setOverrides(undefined); expect(getVariables()).toEqual({ defaults... }).

Rames's question about whether getVariables is read at render-time (not just in the editor) is worth resolving now — if yes, the same regression net is needed at the renderer boundary.

[vanceingalls]

Addressed in latest push:

• Test 4 redesigned (miguel + Rames): Now calls setOverrides({ primary: "#brand-primary", secondary: "#brand-secondary" }) first (full batch), then setOverrides({ primary: "#manual" }), and asserts secondary drops to the declared default "#000" — not the batch value. This directly pins "second call replaces, not merges into, the first override-set." Renamed accordingly.
• setOverrides(undefined) clear test (miguel): Added — asserts that after setOverrides(undefined), getVariables() returns declared defaults.
• Rames Q — render-time vs editor: getVariables is a runtime module imported by @hyperframes/player for composition execution. Same regression net applies there; out of scope for this PR but tracked.
• setOverrides(undefined) in afterEach: the afterEach already relied on this to clear state — the new test just makes the behavior explicit.

[miguel-heygen]

Follow-up: both concerns addressed.

• ✅ Test 4 rewritten correctly: setOverrides(batch) → setOverrides({partial}) → asserts uncovered key falls back to declared default, not prior batch value. Inline comment makes the flat-replace semantics explicit. This is the load-bearing case.
• ✅ New test: setOverrides(undefined) clears overrides and returns declared defaults.

✅ Re-approve.

The base branch was changed.

[vanceingalls]

Merge activity

• Jun 7, 1:04 AM UTC: @vanceingalls merged this pull request with Graphite.