Skip to content

Commit 5dc8a66

Browse files
seeMclaudedhruvisompura
authored
React test helper: setupReactRenderer() (#12083)
Adds shared React test infrastructure and an ESLint rule to enforce correct usage. Related to #10000. - New `setupReactRenderer()` helper manages the React root lifecycle via mocha `setup`/`teardown` hooks. - New ESLint rule to enforce that `setupReactRenderer()` is called before `ensureNoDisposablesAreLeakedInTestSuite()` in test suites. Mocha teardown is FIFO, so the React teardown must unmount components (and cleanup effects which often cleans up disposables) before the leak checker inspects disposables. - Updated Claude rules given above - Added `PositronFindWidget` tests and refactored action bar widget and column summary cell tests to test everything out ### Release Notes #### New Features - N/A #### Bug Fixes - N/A ### QA Notes No user-facing changes. All changes are to test infrastructure and developer tooling. --------- Signed-off-by: Wasim Lorgat <mwlorgat@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Dhruvi Sompura <dhruvi.sompura@posit.co>
1 parent 5f62b64 commit 5dc8a66

File tree

10 files changed

+460
-83
lines changed

10 files changed

+460
-83
lines changed

.claude/rules/core-tests.md

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,18 @@ disposables.add(someDisposable);
1818

1919
## Services (prefer in this order)
2020

21-
1. Real services directly (e.g. `new RuntimeStartupService(...)`).
22-
2. Existing `Test*` service classes - search from `src/vs/workbench/test/browser/positronWorkbenchTestServices.ts`.
23-
3. For many dependencies, use `positronWorkbenchInstantiationService`.
21+
1. `{} as T` for services the code under test doesn't call (e.g. dependencies it only passes through to collaborators).
22+
2. Real services directly (e.g. `new RuntimeStartupService(...)`).
23+
3. Existing `Test*` service classes - search from `src/vs/workbench/test/browser/positronWorkbenchTestServices.ts`.
24+
4. For many dependencies, use `positronWorkbenchInstantiationService`.
25+
26+
## Structure
27+
28+
- Group related tests with nested `suite()` calls, not comment headers like `// --- Section ---`.
2429

2530
## Mocking
2631

27-
- Use `Partial<T>` with only the members your test needs, cast with `as T`.
32+
- Use `Partial<T>` with only the members your test needs, cast with `as T`. This might trigger `local/code-no-dangerous-type-assertions`; add `/* eslint-disable local/code-no-dangerous-type-assertions */` below the copyright header.
2833
- For reusable mocks, create a test helper class implementing `Partial<T>`.
2934

3035
## Sinon

.claude/rules/react-tests.md

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,14 @@ paths:
55

66
# React Tests
77

8-
React component tests run in Electron (not browser). No React testing libraries are available - only `assert`, `sinon`, `react-dom`, and `react-dom/client`.
8+
React component tests run in Electron with `assert`, `sinon`, and `react-dom`. No React testing libraries (Testing Library, Enzyme, etc.) are available.
99

10-
## Constraints
10+
## Setup
1111

12-
- Place tests in `test/browser/` adjacent to source: `feature/browser/components/foo.tsx` -> `feature/test/browser/foo.test.tsx`
13-
- Use `mainWindow.document` instead of `document` for DOM operations.
14-
- `querySelector` requires `/* eslint-disable no-restricted-syntax */` below the copyright header.
15-
- Render via a helper that wraps `root.render()` in `flushSync()` so React flushes synchronously.
16-
- Teardown order matters: (1) `root.unmount()`, (2) `container.remove()`, (3) `sinon.restore()`.
12+
Use `setupReactRenderer()` from `base/test/browser/react.js` to manage the DOM container and React root. Call it **before** `ensureNoDisposablesAreLeakedInTestSuite()` -- mocha teardown is FIFO, so React must unmount before the leak checker runs.
13+
14+
## General
15+
16+
- Place tests in `test/browser/` adjacent to source.
17+
- `render()` returns the DOM container -- query it for assertions.
18+
- `querySelector<T>` requires `/* eslint-disable no-restricted-syntax */` below the copyright header.
Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
/*---------------------------------------------------------------------------------------------
2+
* Copyright (C) 2026 Posit Software, PBC. All rights reserved.
3+
* Licensed under the Elastic License 2.0. See LICENSE.txt for license information.
4+
*--------------------------------------------------------------------------------------------*/
5+
6+
import * as eslint from 'eslint';
7+
import type * as estree from 'estree';
8+
import { TSESTree } from '@typescript-eslint/utils';
9+
10+
export default new class SetupReactRendererBeforeDisposablesCheck implements eslint.Rule.RuleModule {
11+
12+
readonly meta: eslint.Rule.RuleMetaData = {
13+
type: 'problem',
14+
messages: {
15+
order: '`setupReactRenderer()` must be called before `ensureNoDisposablesAreLeakedInTestSuite()`. ' +
16+
'Mocha runs teardown hooks in FIFO order, so the React teardown must register first to unmount ' +
17+
'components and dispose VS Code disposables before the leak checker inspects them.',
18+
},
19+
schema: false,
20+
};
21+
22+
create(context: eslint.Rule.RuleContext): eslint.Rule.RuleListener {
23+
return {
24+
// Match setupReactRenderer() as a direct statement (expression or variable declaration) inside a suite callback.
25+
['CallExpression[callee.name="suite"] > :function > BlockStatement > :matches(ExpressionStatement, VariableDeclaration) CallExpression[callee.name="setupReactRenderer"]']: (node: estree.CallExpression) => {
26+
// Walk up to the BlockStatement to find sibling statements.
27+
let stmt = node as TSESTree.Node;
28+
while (stmt.parent && stmt.parent.type !== 'BlockStatement') {
29+
stmt = stmt.parent;
30+
}
31+
const block = stmt.parent as TSESTree.BlockStatement;
32+
const setupIdx = block.body.indexOf(stmt as TSESTree.Statement);
33+
34+
// Scan backward: if ensureNoDisposablesAreLeakedInTestSuite appears earlier, report it.
35+
for (let i = setupIdx - 1; i >= 0; i--) {
36+
if (isCallInStatement(block.body[i], 'ensureNoDisposablesAreLeakedInTestSuite')) {
37+
context.report({ node: block.body[i] as estree.Node, messageId: 'order' });
38+
return;
39+
}
40+
}
41+
},
42+
};
43+
}
44+
};
45+
46+
/** Check whether a statement contains a call to the given function name. */
47+
function isCallInStatement(node: TSESTree.Statement, name: string): boolean {
48+
if (node.type === 'ExpressionStatement') {
49+
return isCallTo(node.expression, name);
50+
}
51+
if (node.type === 'VariableDeclaration') {
52+
return node.declarations.some(d => d.init && isCallTo(d.init, name));
53+
}
54+
return false;
55+
}
56+
57+
function isCallTo(node: TSESTree.Expression, name: string): boolean {
58+
return node.type === 'CallExpression'
59+
&& node.callee.type === 'Identifier'
60+
&& node.callee.name === name;
61+
}
Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
/*---------------------------------------------------------------------------------------------
2+
* Copyright (C) 2026 Posit Software, PBC. All rights reserved.
3+
* Licensed under the Elastic License 2.0. See LICENSE.txt for license information.
4+
*--------------------------------------------------------------------------------------------*/
5+
6+
// Test file to verify the code-setup-react-renderer-before-disposables-check ESLint rule.
7+
8+
import { setupReactRenderer } from '../../src/vs/base/test/browser/react.js';
9+
import { ensureNoDisposablesAreLeakedInTestSuite } from '../../src/vs/base/test/common/utils.js';
10+
11+
// -----
12+
// Valid
13+
// -----
14+
15+
suite('correct order - destructured', () => {
16+
const { render } = setupReactRenderer();
17+
ensureNoDisposablesAreLeakedInTestSuite();
18+
});
19+
20+
suite('correct order - expression statement', () => {
21+
setupReactRenderer();
22+
ensureNoDisposablesAreLeakedInTestSuite();
23+
});
24+
25+
suite('correct order - ensureNoDisposables assigned to variable', () => {
26+
setupReactRenderer();
27+
const disposables = ensureNoDisposablesAreLeakedInTestSuite();
28+
});
29+
30+
suite('only setupReactRenderer - no ensureNoDisposables', () => {
31+
setupReactRenderer();
32+
});
33+
34+
suite('only ensureNoDisposables - no setupReactRenderer', () => {
35+
ensureNoDisposablesAreLeakedInTestSuite();
36+
});
37+
38+
suite('neither function present', () => {
39+
});
40+
41+
// -------
42+
// Invalid
43+
// -------
44+
45+
suite('wrong order - destructured', () => {
46+
// eslint-disable-next-line local/code-setup-react-renderer-before-disposables-check
47+
ensureNoDisposablesAreLeakedInTestSuite();
48+
const { render } = setupReactRenderer();
49+
});
50+
51+
suite('wrong order - expression statement', () => {
52+
// eslint-disable-next-line local/code-setup-react-renderer-before-disposables-check
53+
ensureNoDisposablesAreLeakedInTestSuite();
54+
setupReactRenderer();
55+
});
56+
57+
suite('wrong order - ensureNoDisposables assigned to variable', () => {
58+
// eslint-disable-next-line local/code-setup-react-renderer-before-disposables-check
59+
const disposables = ensureNoDisposablesAreLeakedInTestSuite();
60+
setupReactRenderer();
61+
});

eslint.config.js

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,16 @@ export default tseslint.config(
176176
'jsx-a11y/no-static-element-interactions': 'error',
177177
},
178178
},
179+
// React Tests
180+
{
181+
files: [
182+
'src/vs/**/*.test.tsx',
183+
'.eslint-plugin-local/**/*.tsx',
184+
],
185+
rules: {
186+
'local/code-setup-react-renderer-before-disposables-check': 'error',
187+
}
188+
},
179189
// --- End Positron ---
180190
// TS
181191
{

src/vs/base/test/browser/react.tsx

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
/*---------------------------------------------------------------------------------------------
2+
* Copyright (C) 2026 Posit Software, PBC. All rights reserved.
3+
* Licensed under the Elastic License 2.0. See LICENSE.txt for license information.
4+
*--------------------------------------------------------------------------------------------*/
5+
6+
import { ReactElement } from 'react';
7+
import { flushSync } from 'react-dom';
8+
import { createRoot, Root } from 'react-dom/client';
9+
import { mainWindow } from '../../browser/window.js';
10+
11+
/**
12+
* Sets up a React render root for component tests. Registers mocha `setup` and
13+
* `teardown` hooks that create/destroy the DOM container and React root.
14+
*
15+
* Must be called before `ensureNoDisposablesAreLeakedInTestSuite()` because
16+
* mocha runs teardown hooks in FIFO order. The React teardown must run first so
17+
* that deferred `useEffect` cleanups dispose VS Code disposables before the leak
18+
* checker inspects them.
19+
*
20+
* @example
21+
* ```ts
22+
* suite('MyWidget', () => {
23+
* const { render } = setupReactRenderer();
24+
* ensureNoDisposablesAreLeakedInTestSuite();
25+
*
26+
* test('renders', () => {
27+
* const container = render(<MyWidget />);
28+
* assert.ok(container.querySelector('.my-widget'));
29+
* });
30+
* });
31+
* ```
32+
*/
33+
export function setupReactRenderer() {
34+
let root: Root;
35+
let el: HTMLElement;
36+
37+
setup(() => {
38+
el = mainWindow.document.createElement('div');
39+
mainWindow.document.body.appendChild(el);
40+
root = createRoot(el);
41+
});
42+
43+
teardown(async () => {
44+
root.unmount();
45+
el.remove();
46+
});
47+
48+
return {
49+
/**
50+
* Render a React element synchronously via `flushSync` and return the
51+
* DOM container the React root is mounted into.
52+
*/
53+
render(element: ReactElement): HTMLElement {
54+
if (root === undefined) {
55+
throw new Error('React root is not initialized. Did you try to render before setup?');
56+
}
57+
flushSync(() => {
58+
root.render(element);
59+
});
60+
return el;
61+
},
62+
};
63+
}

0 commit comments

Comments
 (0)