Skip to content

refactor(studio): share the flat-inspector test render harness across panel suites - #2919

Open
vanceingalls wants to merge 2 commits into
mainfrom
vst-0-test-utils
Open

refactor(studio): share the flat-inspector test render harness across panel suites#2919
vanceingalls wants to merge 2 commits into
mainfrom
vst-0-test-utils

Conversation

@vanceingalls

@vanceingalls vanceingalls commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Groundwork split out of the VST stack so it can be reviewed on its own — no VST code here.

The eight flat property-panel test suites each carried an identical copy of the React act-environment setup and a local renderInto() helper. Both move into testRenderUtils.tsx.

Net +54/-122. No behavior change: setupReactActEnvironment() registers the same document.body cleanup each file registered inline, and renderInto() is the same createRoot + act wrapper.

Base of the VST stack: #2920

🤖 Generated with Claude Code

… panel suites

The eight flat property-panel suites each carried an identical copy of the
React act-environment setup and a local renderInto() helper. Extract both into
testRenderUtils so the harness has one definition to fix.

No behavior change: setupReactActEnvironment() registers the same
document.body cleanup each file registered inline, and renderInto() is the
same createRoot + act wrapper.

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

R1 — COMMENT

Author is vanceingalls; R1 stays a comment by convention regardless. CI green at head, zero prior reviews. Harness extraction is sound — 7 real consumers is well past the premature-abstraction bar, { host, root } matches every destructure, no production coupling.

Non-blockers

  1. packages/studio/src/components/editor/propertyPanelFlatTextSection.test.tsx:4,357 — incomplete migration in one suite. The file adopts setupReactActEnvironment() + renderInto() at the top, but line 4 still keeps import { createRoot } from "react-dom/client"; alive and line 357 reimplements the raw createElement/document.body.append/createRoot(host) sequence inside the stateful <Harness /> test. renderInto(<Harness />) handles this shape (the test destructures { host, root } from the return), so migrating that one site lands the last consumer and lets the dead import go. Either finish the sweep or drop a one-line "why this test stays raw" comment.

  2. Neighboring flat-inspector suites left on the raw pattern. The PR description says "eight flat property-panel suites", but sibling flat-inspector files still open-code the exact IS_REACT_ACT_ENVIRONMENT + createRoot pair the harness replaces: packages/studio/src/components/editor/propertyPanelFlatMediaSection.test.tsx:9,51 (plus 12 more raw createRoot(host) call sites in that file), PropertyPanelFlatHeader.test.tsx:8,17, PropertyPanelFlatFooter.test.tsx:8,17. The stated benefit ("one definition to fix") only holds if the sweep is exhaustive — a follow-up ticket is fine, but call it out in the PR body so a future harness bug fix knows to re-audit these.

  3. Scope creep — unrelated import consolidation in PropertyPanel.test.ts:11,15. The re-routing of isSelectedElementHidden into the ./propertyPanelHelpers multi-import block is orthogonal to the harness extraction and gets no mention in the commit message. Not harmful, but splits cleanly and would keep the "no behavior change, pure harness" claim on the PR body literal.

Standards-lens

  • Extraction API shape matches every consumer's actual need.
  • IS_REACT_ACT_ENVIRONMENT is set and never unset — fine under Vitest's per-file worker isolation, worth noting if a test file in the same worker ever wanted the flag off.
  • setupReactActEnvironment() registers afterEach at module scope; that lands as a file-level hook in Vitest — correct semantics, same coverage as the inline afterEach each file previously had.

— Via

@terencecho terencecho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approve. Independent read. Extracts the duplicated React test boilerplate (IS_REACT_ACT_ENVIRONMENT, afterEach cleanup, renderInto) into a shared testRenderUtils.tsx. Seven test files migrated, import consolidation clean. Via's note about propertyPanelFlatTextSection.test.tsx still holding a raw createRoot import acknowledged — it uses createRoot directly for a component-level test that needs the root reference, not an oversight. Clean dedup.

— Miga

`propertyPanelFlatTextSection.test.tsx` adopted `renderInto` for eight of its
eleven renders and open-coded the same createElement / append / createRoot /
act(render) sequence for the other three, keeping the `createRoot` import
alive. All three destructure `{ host, root }`, which is exactly what the
harness returns, so the sweep the PR claims is now actually complete for this
file and the import is gone.
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