Skip to content

test(studio): add T5a pathOffset+boxSize build-patches characterization#1257

Merged
vanceingalls merged 1 commit into
mainfrom
06-07-test_studio_add_t5a_pathoffset_boxsize_build-patches_characterization
Jun 8, 2026
Merged

test(studio): add T5a pathOffset+boxSize build-patches characterization#1257
vanceingalls merged 1 commit into
mainfrom
06-07-test_studio_add_t5a_pathoffset_boxsize_build-patches_characterization

Conversation

@vanceingalls
Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls commented Jun 7, 2026

Summary

T5 (part 1 of 3) — characterization test suite for manualEditsDomPatches.ts.

The source module exports 8 functions (4 build/clear pairs) that write and restore draft-marker attributes and inline styles onto iframe elements before source-patch operations. It had zero tests.

This PR covers the pathOffset and boxSize pairs with 4 patterns each:

  • populated — fully-configured element produces exact expected PatchOperation[] in declaration order
  • empty — bare element yields only the mandatory marker attribute op
  • clear restores originalsbuildClear* reads STUDIO_ORIGINAL_* attrs and produces correct restore values
  • build/clear symmetry — every {type, property} key that build* can emit is also addressed by buildClear*; an orphan here means a property stranded in committed source HTML

Uses @vitest-environment happy-dom matching the Studio package convention. Element setup via document.createElement + style.setProperty / setAttribute.

Stack

Opens manualEditsDomPatches.test.ts with @vitest-environment happy-dom.
Covers 4 patterns per pair (pathOffset + boxSize): populated output, empty
element, clear restores originals, build/clear symmetry assertion.
Copy link
Copy Markdown
Collaborator

@miguel-heygen miguel-heygen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean characterization test with good structure: @vitest-environment happy-dom, div() factory, assertClearCoversKeys symmetry helper. The 4-pattern (populated / empty / clear-restores-originals / build-clear-symmetry) is a solid template for the rest of T5.

One thing to note for review completeness: populatedBoxEl() sets STUDIO_ORIGINAL_TRANSFORM_DISPLAY_ATTR to "" and the populated test asserts value: "" — correctly capturing current behavior so a future coercion change (empty → null) won't go unnoticed. The weak arrayContaining on the clear test is intentional since T5c will tighten it to a full ordered toEqual. Intentional and fine.

P3 nit: the "clear: restores width and height" test name could mention that it uses arrayContaining on purpose (to be replaced in T5c) — would help future readers distinguish "intentionally partial" from "forgot to be exact."

Copy link
Copy Markdown

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round-1 review of T5a (pathOffset + boxSize characterization). Read the diff (+212/-0) against the implementation in manualEditsDomPatches.ts and verified the test characterizations match the source semantics. Looks good — the 4-pattern structure (populated / empty / clear-restores-originals / build-clear symmetry) is well-chosen for catching refactor regressions, and the populated cases lock in exact toEqual declaration order which is exactly what a refactor of buildPathOffsetPatches could silently drift.

What's strong

  • Declaration-order lock-in via toEqual. buildPathOffsetPatches/buildBoxSizePatches interleave collectInlineStyleOps and collectAttributeOps calls in a specific order; the populated tests assert that exact PatchOperation[] sequence. Any refactor that reorders the helper calls now fails CI immediately. Matches the contract because callers downstream apply the ops in array order against the DOM.
  • assertClearCoversKeys symmetry invariant. "Every {type, property} key that build emits is also addressed by clear" is the right correctness check for this module — an orphan key in build means a property stranded in committed source HTML, which would cause downstream UI breakage that's hard to debug. Treating it as a checkable invariant rather than a manual-review item is the right call.
  • @vitest-environment happy-dom + bare document.createElement. No iframe or shadow-DOM machinery in the setup — the unit under test is a pure DOM-reader, so the minimal environment matches the surface.

Concerns

[important — clear: restores width and height from orig attrs uses partial assertion] Lines 200-216, the buildClearBoxSizePatches clear test uses expect.arrayContaining([...]):

expect(ops).toEqual(
  expect.arrayContaining([
    { type: "inline-style", property: STUDIO_WIDTH_PROP, value: null },
    // …
  ]),
);

This leaves the interleaved restore-then-null loop order (the for (const [attrName, styleProp] of BOX_SIZE_ORIG_ATTRS) { … } in buildClearBoxSizePatches:155-161) completely untested. A refactor that swapped the two ops.push calls inside the loop would produce a different DOM-mutation sequence at runtime but still pass this test. Same risk shape as the populated tests guard against, but the negative-path (clear) case has weaker coverage than the positive-path (build) case.

Looking at the stack — I see #1259 is exactly "T5c review-fix gaps" and replaces this with a full ordered toEqual on populatedBoxEl() covering all 30 ops. So the gap is closed at the stack head. For this PR specifically I'd accept it as "incrementally less complete than the stack head" — but if the stack ever needs to land out of order, this is the seam.

Nits

  • The "clear" cases use div() with only 1-2 attrs set rather than the populatedBoxEl() factory. That gives an "easy" test path with minimal state. Pairing each clear test with a populated-element variant (as #1259 does) tightens it. Already addressed in the stack.
  • assertClearCoversKeys checks one direction (every build key is in clear). The reverse (every clear key is in build) is intentionally not checked because clear can defensively null draft attrs like STUDIO_ROTATION_DRAFT_ATTR that build* never produces — but this asymmetry is implicit. A one-line comment in the helper explaining why the direction is one-way would help a future reader who wonders "why isn't this a ===?"

Plan-doc alignment

T5 in the plan covers manualEditsDomPatches.ts characterization before the refactor; T5a is "pathOffset + boxSize pairs" as written. Scope matches. The next pairs land in #1258 (T5b: rotation + motion) and the gap fixes in #1259 (T5c).

Verdict

Solid foundation for the stack. The arrayContaining weakness on box-size clear is real but cleanly addressed by #1259 — accept the staging. Leaving as a comment.

Review by Rames D Jusso

@vanceingalls vanceingalls merged commit 19f46cd into main Jun 8, 2026
36 checks passed
@vanceingalls vanceingalls deleted the 06-07-test_studio_add_t5a_pathoffset_boxsize_build-patches_characterization branch June 8, 2026 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants