test: update vitests for RTL idioms and builder adoption#13165
Open
test: update vitests for RTL idioms and builder adoption#13165
Conversation
|
E2E Tests 🚀 |
midleman
changed the title
Two-PR plan: PR1 modernizes 14 already-migrated .vitest.* files with RTL idioms (Dhruvi's feedback) and builder adoption, plus codifies the conventions in .claude/rules/vitest-tests.md and the review-vitest-tests skill. PR2 finishes the Mocha to Vitest migration for the remaining 18 Positron-origin .test.ts files (6 trivial, 12 builder-fit, 3 investigated inline, 1 deferred). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Removed reserved-slot placeholder from trivial bucket, corrected builder-fit count to 10, collapsed cellEditorPrimitives into the builder-fit bucket with a re-confirm note, and pinned the skill path to the exact file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… cleanup) 18 tasks: capture baseline, add RTL idioms section to rules, tighten review-vitest-tests skill checks, then 14 file rewrites in difficulty order (8 RTL-only, 3 builder-only, 3 RTL+builder combined), final verification + PR open. Self-contained per-task with shared conversion recipes for RTL queries and builder substitutions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… matchers Codifies the conventions Dhruvi flagged in the #13033 review: prefer RTL queries over container.querySelector, jest-dom matchers over raw asserts, and toBeInTheDocument over toBeTruthy for DOM presence.
Tightens check #2 (builder adoption) to flag TestInstantiationService, upstream workbenchInstantiationService, accessor casts, and singleton mutation. Tightens check #11 to flag any container.querySelector as a failure (was: "acceptable when RTL queries aren't feasible"). Adds new check #12 for assertion idioms (toBeInTheDocument over toBeTruthy, etc.).
Code review found two consistency gaps with .claude/rules/vitest-tests.md: - Check #12 was missing toBeNull and assert.equal (rules file has them). - Check #11 dropped getByTestId as the second escape hatch and lacked guidance for elements with no text/role. Also normalizes the query-priority arrow glyph (`>` -> `->`) so the skill and the rules file use the same form.
Replaces container.querySelector + assert.strictEqual pattern with
getByText({ selector }) + jest-dom matchers per Dhruvi's #13033 review
suggestion. Establishes the canonical RTL idiom example referenced in
.claude/rules/vitest-tests.md.
Wires @testing-library/jest-dom into the Vitest setup so .vitest.tsx files can use toBeInTheDocument(), toHaveTextContent(), toHaveClass(), toBeDisabled(), and the rest of the canonical RTL assertion matchers that the .claude/rules/vitest-tests.md "RTL idioms" section recommends. Without this, the recommended matchers don't exist; tests have to fall back to bare getByText() (relies on throw) or expect(queryBy*).toBeNull() for absence. Matchers make assertions more explicit and the failure messages more readable.
The src/vs/test/vitest/** directory's import allowlist needs the Testing Library packages so the new setup.ts can import jest-dom's vitest extension and so reactTestingLibrary.tsx can import RTL.
…tubs Replaces direct PositronReactServices.services singleton mutation + beforeEach/afterEach save-restore dance with .stub() calls in the createTestContainer() builder. Because TableSummaryDataGridInstance reads PositronReactServices.services (the static singleton) in its constructor rather than receiving services via props/context, a single-line bridge remains in beforeEach to assign ctx.reactServices onto the singleton. The save/restore pair is no longer needed: the builder rebuilds the DI container fresh for every test.
… preset Replaces upstream workbenchInstantiationService() helper with createTestContainer().withWorkbenchServices() + explicit stubs.
Replaces upstream workbenchInstantiationService() in createPositronNotebookTestServices() with positronWorkbenchInstantiationService -- the same Positron service stack that createTestContainer().withWorkbenchServices() wires up. The helper runs inside it()/beforeEach callbacks so it cannot use the fluent builder directly (build() registers a describe-level beforeEach), but calling the underlying Positron factory gives us the same preset stack without the upstream dependency. Consumers keep the same public API via the returned TestServices object.
…WorkbenchInstantiationService Replaces direct TestInstantiationService usage in the shared notebook test helper with positronWorkbenchInstantiationService, the factory that the Positron builder's workbench preset delegates to internally. Consumers keep the same public API.
These internal brainstorming and implementation-plan artifacts belong in local notes, not committed alongside the code change they produced.
- CellTextOutput.vitest.tsx: reset MockContextKeyService._keys between
tests so a context key set in one test doesn't leak to the next.
- positronFindWidget.vitest.tsx: toHaveTextContent('') matches
substrings, so '' always passes; use /^$/ for exact empty-string.
- testPositronNotebookInstance.ts + positronNotebookFind.vitest.ts:
ITestInstantiationService doesn't expose .get(), so three call sites
now use invokeFunction(accessor => accessor.get(X)).
- notebookErrorBoundary.vitest.tsx: GoodComponent renders "Hello" and
the recovered div renders "Recovered", so getByText replaces
querySelector for two fixtures that have real text handles.
- .claude/rules/vitest-tests.md: assert.* ban now applies to .vitest.ts
too (not just .vitest.tsx), showcase notation fixed, and a note
clarifies that bare getByRole/getByText is itself an assertion
(wrapping in toBeInTheDocument is redundant).
- .claude/skills/review-vitest-tests/SKILL.md: same clarifications in
checks #2 (exception for TestCommandService bootstrap and for shared
test helpers) and #12 (assert.* applies to all .vitest.* files).
The rules update in c557696 says bare getByText is itself the assertion; the two new getByText call sites in the same commit were wrapped in expect(...).toBeInTheDocument(), contradicting the rule. Unwrapped.
… reference The RTL idioms, builder adoption patterns, and assertion anti-patterns were duplicated between the rules doc and the review-vitest-tests skill, which created drift risk (and in fact drifted during the PR1 review cycle). Consolidates the three rules into an Anti-patterns reference section in .claude/rules/vitest-tests.md with one table per category (builder, RTL, assertion style). Each row names what to avoid, what to use instead, and any exceptions. The skill's checks #2, #11, and #12 collapse to a single check that points reviewers at the reference -- the rules doc is now the single source of truth.
- 19 redundant expect(getBy*(...)).toBeInTheDocument() wrappers removed across actionBarWidget, notebookErrorBoundary, positronFindWidget, CellOutputActionBar, and CellOutputCollapseButton. The anti-patterns table says bare getBy* IS the assertion; 9693369 unwrapped the 2 freshly-added sites but missed the ~17 pre-existing ones. - actionBarWidget.vitest.tsx: two describe-scope Emitters are now disposed in afterAll. They can't live in ctx.disposables (that fires per-test; would break next test's commandService) but they should be disposed once at suite end. - CellTextOutput.vitest.tsx: private-field resets now guarded by a runtime invariant. If upstream renames configurationService.configuration or contextKeyService._keys, the test throws with a clear message instead of silently leaking state between tests.
Enforces RTL best practices at lint time so reviewers don't have to spot them by grep (which misses multi-argument forms). Caught 9 additional violations that the branch review's grep-based scan missed, including an `.not.toBeNull()` form and `queryByRole().toBeInTheDocument()` sites in actionBarWidget, reactTestingLibrary, positronFindWidget, topActionBarSessionManager, CellTextOutput, and notebookErrorBoundary. Rules enabled: - prefer-implicit-assert: bare getBy* IS the assertion (the thing grep missed) - prefer-presence-queries: use getBy* for presence, queryBy* for absence - no-render-in-lifecycle: render() in beforeEach/beforeAll leaks state - no-debugging-utils: no screen.debug() in committed code - no-unnecessary-act: RTL auto-wraps render/userEvent - await-async-queries / await-async-utils / no-await-sync-queries: correct async handling on findBy* and waitFor Not enabled: - no-node-access: would double-flag sites already caught by the pre-existing local/no-restricted-syntax rule, which we disable per-line at documented structural escape hatches. Leaving it off avoids churn. The .claude/rules/vitest-tests.md Anti-patterns reference now notes which rows are lint-enforced vs documentation-only.
Three late-round review findings:
- notebookErrorBoundary Retry test: loop over ['cell', 'output'] rewritten
as it.each so the failing iteration is identified in the test name
(preserves the diagnostic the prior message label provided).
- notebookErrorBoundary two unwrapped getBy* sites: intent-describing
comments added above each call so failure context isn't lost
("Children should re-render after retry", "Error boundary should
catch provider errors").
- .claude/rules/vitest-tests.md: exception column for
expect(getBy*).toBeInTheDocument() row rephrased -- it was describing
a different pattern (queryBy*), not a real exception. Also added
"prefer data-testid" nudge to the querySelector escape-hatch row.
- fireEvent -> userEvent across 3 files (actionBarWidget, emptyConsole, topActionBarSessionManager). userEvent v14 fires the full real-user event sequence (pointerdown/mousedown/pointerup/mouseup/click for a click) where fireEvent dispatches a single synthetic event. - Query destructuring -> screen across 13 files. Eliminates the need to update destructure lists as queries change; matches Kent's guidance. - Lint rules prefer-user-event and prefer-screen-queries now enforce both going forward. Test count preserved (601 passed / 11 skipped). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reverses the earlier convention. Testing Library's ESLint plugin docs recommend prefer-explicit-assert (not prefer-implicit-assert) because wrapping makes intent visible and keeps every test assertion reading uniformly -- every line leads with `expect(`. Kent C. Dodds makes the same argument in "Common Mistakes with React Testing Library" #18. The lint rule is flipped to prefer-explicit-assert; ~48 bare sites wrapped; rules doc updated. Tests unchanged (601 passed / 11 skipped).
Plain .vitest.ts authoring no longer loads ~90 lines of React-specific content. The split also removes two tables of anti-patterns that were duplicating eslint-plugin-testing-library rules (drift risk). Remaining Positron-specific rules (builder adoption, singleton mutation) stay in the core file as a small table. - vitest-tests.md: 147 -> 105 lines. Plain/builder core, no RTL. - vitest-rtl.md: new, 77 lines. React quick start, query priority, jest-dom matchers, explicit-assert convention, escape hatches, enforcement (lint rule names), showcase links. - review-vitest-tests skill: setup now reads vitest-rtl.md for .tsx; check #2 splits into builder-table scan (vitest-tests.md) and a `npx eslint` run for RTL/assertion rules. - author-vitest-tests skill: analysis and writing steps point at the split files. Line 169's stale "container.querySelector is acceptable" blurb deleted -- the rules file is the source of truth.
Two drift-reduction cuts in the rules files: - vitest-rtl.md "Enforcement" section used to list every testing-library/* rule by name. That duplicated eslint.config.js; collapsed to a one-line pointer at the config. Adding/removing a lint rule now updates one place. - Removed the paths: frontmatter from both rules files. Claude Code doesn't use it for auto-loading (skills explicitly Read the files), so the paths list was decorative at best and misleading at worst (suggested the files auto-trigger on file edits, which they don't).
…ore drafting the plan Phase 2 previously told the author to read the rules files in step 2 of "Writing each test" -- after the plan gate. But drafting the plan requires knowing the rules (pattern, preset, stubs). A test subagent correctly noticed it had to read rules BEFORE drafting, not after. New flow: - Prepare: read source + rules + decision table + builder JSDoc - Draft the plan (and stop at dev-confirmation gate) - Writing each test: (the rules-read step is gone; this is pure execution) Also collapsed the "Choosing the right pattern" and "Choosing the right preset" subsections into Prepare -- they were reference pointers that belonged at the start, not between the plan and the writing phase.
Four fixes from a skill review:
1. Writing each test: steps were 1, 4, 5, 6, 7 (missing 2 and 3 from an
earlier edit). Renumbered to 1-5.
2. Single file section collapsed from 8 lines to 4. The 3-step
mini-flow duplicated Phase 2 Prepare; replaced with a clean
"single-file vs branch/PR" fork at the top of the skill.
3. "Don't guess -- iterate" paragraph moved from orphan mid-section
(between the subheading and the numbered list) into step 2 where
it belongs: that's the step that runs the test.
4. Key Rules renamed "Hard rules" and trimmed from 8 bullets to 5 --
the removed bullets ("Show your reasoning", "Don't over-mock",
"Don't skip the review") were already covered in the body. What
remains is the set of rules NOT asserted elsewhere.
Net: ~210 lines. Structurally tighter without losing content.
Four cleanups from a second skill review: 1. Writing-each-test checklist no longer restates File setup, iteration pattern, or assertion style -- those all live in the rules file. Step 1 now points at the rules and keeps only the 3 authoring-specific quality items (describe-block structure, setup-size helper extraction, import minimization). 2. Plan template's "Preset" placeholder replaced "bare / runtime / notebook / workbench" (misleading -- no .bare() method) with actual method names from positronTestContainer.ts. 3. Prepare step 2 (Read the rules files) dropped -- the rules files are referenced from specific later steps where Claude needs them. Kept steps 1 (source + neighbors), 2 (CLAUDE.md decision table), 3 (builder JSDoc) -- these direct attention to specific sections. 4. Top-level "Single file vs branch/PR" fork merged into Phase 1's opening as a "Skip this phase if..." line. The fork was stating the same thing twice. Net: 212 -> 197 lines. Structurally tighter; rules file is the single source of truth for content that used to be duplicated in the skill.
midleman
force-pushed
the
mi/vitest-pr1-rtl-modernize
branch
from
49b4df6 to
fdcb1a5
Compare
Tables force alignment across rows with uneven prose length. A list with 'Avoid' + 'use instead' inline and 'Exception' as a sub-bullet reads cleaner for humans and binds each exception tightly to its rule for Claude. Also matches the bulleted-prose pattern the rest of the file already uses.
midleman
commented
Apr 23, 2026
| @@ -0,0 +1,62 @@ | |||
| # Positron Vitest + React Testing Library | |||
Contributor
Author
There was a problem hiding this comment.
Pulled React test rules into separate file since not ALL tests need this info.
Finding 1 (MEDIUM): CellTextOutput private-field reset was incomplete.
TestConfigurationService has a `configurationByRoot` TernarySearchTree
populated by the `setUserConfiguration({ resource, ... })` overload,
which our reset never cleared. No test in this file uses that overload
today, but the reset should be complete either way. Added a defensive
`.clear()` on the root field.
Finding 2 (LOW): 4x repeated comment block in columnSummaryCell with
personal-name reference removed. The code + eslint-disable marker are
self-documenting.
- reactTestingLibrary.vitest.tsx: replace "this file demonstrates both idioms" comment with a technical explanation (the test exercises the render result object directly; screen.* isn't applicable). - vitest-rtl.md: fix rule-name inconsistency -- text now says the querySelector-flagging rule is `no-restricted-syntax` (matching the disable comments), not `local/no-restricted-syntax`. - vitest-rtl.md + vitest-tests.md: code-block examples were using 4-space indentation; Positron uses tabs. Converted.
…plicit Prepare step 2 previously said 'the decision table is the source of truth for pattern selection' -- which lumps together two checks. Split into (1) confirm Vitest is the right test type at all (not Mocha / ext-host / E2E) -- stop if not; (2) within Vitest, pick the pattern. The gate check used to be implicit; now it's a first-class step.
…warnings
Roborev 2199: drop the private-field reset cast in CellTextOutput.vitest
(workbench preset already provides fresh services per test), add afterAll
to null out the reactServices singleton in tableSummaryDataGridInstance,
and assert focus before dispatching keyboard events in actionBarWidget.
Branch-wide sweep of Vitest warnings that slipped the hygiene gate (it
fails only on eslint errors, not warnings): thread optional data-testid
through Icon/ThemeIcon/URIIcon so TopActionBar tests can query by role-
like testids instead of 17 .action-bar-button-* querySelector calls; add
data-testids to startupStatus, placeholderThumbnail, CellTextOutput, and
NotebookCellQuickFix; swap columnSummaryCell destructures to screen.*;
expose private _findInput operations via TestableConsoleFindWidget
methods instead of (widget as any)._findInput; and drop the disallowed
`import type { Mock } from 'vitest'` in positronNotebookFind.
Also update the review-vitest-tests skill to run eslint with
--max-warnings 0 so future reviews surface these class of warnings.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous commit hardcoded data-testid='action-bar-button-icon' /
'action-bar-button-label' into the shared sub-components. Any tree that
renders two or more ActionBarButtons would trip getByTestId's
"multiple elements" error. Thread data-testid as an optional prop
instead; TopActionBarSessionManager opts in with scoped names
('session-manager-icon' / 'session-manager-label') and tests query
those.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Migrate positronNotebookFind.vitest.ts off the escape-hatch pattern (empty builder shell + positronNotebookInstantiationService() inside beforeEach + ITestInstantiationService.invokeFunction(accessor => accessor.get(X)) dance) onto a proper preset. The stub wiring moves to src/vs/test/vitest/presets/notebookEditor.ts; positronNotebookInstantiationService now delegates to it so createTestPositronNotebookInstance() keeps working for any remaining callers. New usage: const ctx = createTestContainer().withNotebookEditorServices().build(); // ctx.get(IModelService), ctx.instantiationService.stub(...), etc. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ices()
Switch positronNotebookCell, positronNotebookInstance, positronNotebookSplitJoin,
positronNotebookOutline, and positronNotebookCellOutputs off the bare
createTestContainer().build() + createTestPositronNotebookInstance(ctx.disposables)
escape hatch and onto the same builder preset the find controller test
already uses. Update createTestPositronNotebookInstance() to take the
whole ctx ({instantiationService, disposables}) so callers opt in via
`.withNotebookEditorServices()` at describe scope and the notebook
instance receives the builder's instantiation service.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Summary
eslint-plugin-testing-library.What changed
container.querySelector→getByRole/getByText/getByAltText/getByTestIdassert.*/toBeTruthy→ jest-dom matchers (toBeInTheDocument,toHaveTextContent,toHaveClass,toBeDisabled,toHaveAttribute).TestInstantiationService, upstreamworkbenchInstantiationService(), and hand-rolled accessor casts →createTestContainer().withReactServices().stub(...).build().fireEvent→userEvent(fires full real-user event sequences)render→screen.getByRole(...)etc.expect(getBy*(...)).toBeInTheDocument()for pure existence checks (every assertion leads withexpect()@testing-library/jest-dom,@testing-library/user-event, andeslint-plugin-testing-libraryadded and wired in. Lint rules enforce the conventions going forward so drift is structurally prevented.QA Notes
@:top-action-bar @:plots @:positron-notebooks @:console @:data-explorer @:sessions @:web
🤖 Generated with Claude Code