From 23f4454582ddc23a96bca950ff6d09fdfe92df63 Mon Sep 17 00:00:00 2001 From: Ryan Albrecht Date: Tue, 22 Sep 2026 13:01:14 -0700 Subject: [PATCH 1/2] meta(refactor-tasks): Add the no-custom-render-helper convention Test files keep growing local renderFoo() helpers that only forward their arguments into the shared render() or renderHookWithProviders() helper. Each file invents its own name and argument shape, so the JSX under test is hidden behind an indirection the reader has to look up first. The rule asks for an ExampleFoo component instead, so call sites stay in the shape used everywhere else. The react-testing skill gains the same guidance, since that is what agents and humans read while writing a test rather than the scanner config. --- .agents/skills/react-testing/SKILL.md | 42 +++++ .../conventions/no-custom-render-helper.yaml | 161 ++++++++++++++++++ 2 files changed, 203 insertions(+) create mode 100644 .sentry-refactor-tasks/conventions/no-custom-render-helper.yaml diff --git a/.agents/skills/react-testing/SKILL.md b/.agents/skills/react-testing/SKILL.md index c8ef58a89694..ef11883123e7 100644 --- a/.agents/skills/react-testing/SKILL.md +++ b/.agents/skills/react-testing/SKILL.md @@ -120,6 +120,48 @@ const project = ProjectFixture(partialProject) ``` +### Render the component, not a local `renderFoo()` helper + +Do not wrap `render()` in a local helper. Every file invents its own name and +argument shape, so a reader has to go find the helper before any `it()` block +makes sense, and `rerender` stops behaving because the call site no longer +controls the element. + +When a test needs fixed props, wrapper providers or a bit of glue, put that in a +component and render it. + +```tsx +// ❌ The JSX under test is hidden behind a helper +function renderComponent(props: LoadingContainerProps = {}) { + return render( + +
hello!
+ + ); +} + +// ✅ An Example component keeps the call site in the usual shape +function ExampleLoadingContainer(props: LoadingContainerProps) { + return ( + +
hello!
+ + ); +} + +const {rerender} = render(); +rerender(); +``` + +Keep `render()` options (`organization`, `initialRouterConfig`, +`additionalWrapper`) at the call site, and declare the component at module scope +so `rerender` keeps the same component type and preserves state. + +If the helper adds nothing over the component's own props, drop it and call +`render()` directly. A helper that only registers `MockApiClient` +mocks, or one that takes the element to render as a parameter, is not this +pattern and is fine as it is. + ### Use `screen` instead of destructuring ```tsx diff --git a/.sentry-refactor-tasks/conventions/no-custom-render-helper.yaml b/.sentry-refactor-tasks/conventions/no-custom-render-helper.yaml new file mode 100644 index 000000000000..4e79c22e6814 --- /dev/null +++ b/.sentry-refactor-tasks/conventions/no-custom-render-helper.yaml @@ -0,0 +1,161 @@ +name: no-custom-render-helper +severity: warning +tags: [testing, react, jest] + +why: | + Test files often grow a local `renderFoo()` helper that only forwards its + arguments into the shared `render()` / `renderHookWithProviders()` helper from + `sentry-test/reactTestingLibrary`. Every file invents its own name and its own + argument shape, so a reader must first go read the helper before any `it()` + block makes sense, and the standard RTL return value (`rerender`, `router`, + `unmount`) sometimes gets dropped or renamed on the way out. + + When a test needs fixed props, wrapper providers or a bit of glue, put that in + a component instead of in a function. A small `ExampleFoo` component keeps + every call site in the shape the rest of the codebase already uses — + `render()` — so the JSX is visible in the test, extra props can + be passed per test, and `rerender()` works the way RTL + documents it. + + See: .agents/skills/react-testing/SKILL.md + +detect: | + In a test file (`*.spec.ts` / `*.spec.tsx`), look for a locally declared + function whose only job is to call the shared render helper: + + - A `function renderX(...)` declaration, or a `const renderX = (...) => ...` + arrow, declared in the test file or inside a `describe()` block. + - Its body returns (or directly calls) `render(...)`, `renderGlobalModal(...)` + or `renderHookWithProviders(...)` imported from + `sentry-test/reactTestingLibrary`. + - The JSX / hook callback it renders is essentially fixed: the helper's + parameters are spread into props, used to pick between a few literal + prop values, or not used at all. + + Flag the helper declaration itself. + + Do NOT flag: + - Helpers that do real test setup beyond rendering — registering + `MockApiClient.addMockResponse()` mocks, seeding stores, building fixtures, + advancing timers — and never call `render()` (e.g. `renderMockRequests()`). + - Helpers that return extra handles the test needs alongside the render + result, such as a `jest.fn()` spy installed through a context provider + (`return {tracking, ...render(ui, {additionalWrapper})}`). + - Helpers that take the element to render as a parameter and only supply + providers or options (`renderWithX(ui)`), since there is no fixed JSX to + move into a component. + - Render-prop callbacks and test components that return JSX without calling + `render()` at all. + - `render` / `renderHookWithProviders` themselves in + `tests/js/sentry-test/reactTestingLibrary.tsx`, and any other shared helper + under `tests/js/`. + +fix: | + Replace the helper function with a component, then call the shared `render()` + at each call site. + + 1. Name the component `Example` — `ExampleLoadingContainer`, + `ExampleWidgetModal`. (Older tests in the repo use `Test`; either + name is fine, be consistent inside a file.) + 2. Give it the props the helper took as arguments, so each test can vary only + what it cares about. + 3. Move provider wrappers into the component's JSX; move `render()` options + (`organization`, `initialRouterConfig`, `additionalWrapper`) to the call + sites that need them. + 4. Delete the helper and call `render()` in each test. + + Example before: + function renderComponent(props: LoadingContainerProps = {}) { + return render( + +
hello!
+ + ); + } + + it('handles loading', () => { + const {rerender} = renderComponent({isLoading: true}); + }); + + Example after: + function ExampleLoadingContainer(props: LoadingContainerProps) { + return ( + +
hello!
+ + ); + } + + it('handles loading', () => { + const {rerender} = render(); + rerender(); + }); + + For a hook helper, the same move applies to `renderHookWithProviders`: + + // Before + function renderProviders() { + return renderHookWithProviders(() => useScmMessagingProviders(), {organization}); + } + // After — inline the hook call, keep the options at the call site + const {result} = renderHookWithProviders(() => useScmMessagingProviders(), { + organization, + }); + + Gotchas: + 1. Do not re-export the render result through a renamed object; tests expect + the standard `{rerender, unmount, router}` shape from `render()`. + 2. A helper that also returns a spy or a mock handle is doing two jobs — + move the JSX into the component and leave the spy setup in the test, or + leave the helper alone if the spy must be installed through a provider. + 3. Components declared inside `describe()` re-create their identity on every + call; declare `ExampleFoo` at module scope so `rerender` keeps the same + component type and preserves state. + 4. Helpers that are `async` usually await something after rendering (a + `findBy` query, a router navigation) — that awaited part belongs in the + test body, not in the component. + +examples: + bad: + - | + function renderComponent(props: LoadingContainerProps = {}) { + return render( + +
hello!
+ + ); + } + - | + const renderAvatars = ({users, teams}: Props) => + render(); + - | + function renderProviders() { + return renderHookWithProviders(() => useScmMessagingProviders(), {organization}); + } + good: + - | + function ExampleLoadingContainer(props: LoadingContainerProps) { + return ( + +
hello!
+ + ); + } + // in the test: render(); + - | + // Setup helper that registers mocks and never renders + function mockProjectRequests({firstIssue}: {firstIssue?: string} = {}) { + MockApiClient.addMockResponse({url: '/projects/org-slug/project-slug/', body: {}}); + } + - | + // Wrapper that installs a spy through a provider and takes the element + function renderWithTracking(ui: React.ReactElement) { + const tracking = jest.fn(); + return {tracking, ...render(ui, {additionalWrapper: TrackingWrapper})}; + } + +include: + - 'static/**/*.spec.tsx' + - 'static/**/*.spec.ts' + +prefilter: "grep -rlE '^[[:space:]]*(export )?(async )?function render[A-Z]|^[[:space:]]*(const|let) render[A-Z][A-Za-z0-9]*[[:space:]]*=' --include='*.spec.tsx' --include='*.spec.ts' {repo_path}/static/" From 18039956976a7be0659b413e89fbcd0b01d18723 Mon Sep 17 00:00:00 2001 From: Ryan Albrecht Date: Tue, 22 Sep 2026 13:16:34 -0700 Subject: [PATCH 2/2] meta(skills): Tighten the render-helper section in the react-testing skill The section was longer than everything around it and carried a full LoadingContainer example. A generic Widget makes the same point in half the space, matching the terse style of the neighbouring rules. --- .agents/skills/react-testing/SKILL.md | 47 ++++++++------------------- 1 file changed, 14 insertions(+), 33 deletions(-) diff --git a/.agents/skills/react-testing/SKILL.md b/.agents/skills/react-testing/SKILL.md index ef11883123e7..922dcd37bfd3 100644 --- a/.agents/skills/react-testing/SKILL.md +++ b/.agents/skills/react-testing/SKILL.md @@ -120,47 +120,28 @@ const project = ProjectFixture(partialProject) ``` -### Render the component, not a local `renderFoo()` helper +### Render a component, not a `renderFoo()` helper -Do not wrap `render()` in a local helper. Every file invents its own name and -argument shape, so a reader has to go find the helper before any `it()` block -makes sense, and `rerender` stops behaving because the call site no longer -controls the element. - -When a test needs fixed props, wrapper providers or a bit of glue, put that in a -component and render it. +A local helper hides the JSX from every test that calls it, and `rerender` takes +an element, so the call site can no longer control what renders. ```tsx -// ❌ The JSX under test is hidden behind a helper -function renderComponent(props: LoadingContainerProps = {}) { - return render( - -
hello!
- - ); +// ❌ Don't wrap render() in a helper +function renderComponent(props: Props = {}) { + return render(hello); } -// ✅ An Example component keeps the call site in the usual shape -function ExampleLoadingContainer(props: LoadingContainerProps) { - return ( - -
hello!
- - ); +// ✅ Put the fixed parts in a component +function ExampleWidget(props: Props) { + return hello; } - -const {rerender} = render(); -rerender(); +render(); ``` -Keep `render()` options (`organization`, `initialRouterConfig`, -`additionalWrapper`) at the call site, and declare the component at module scope -so `rerender` keeps the same component type and preserves state. - -If the helper adds nothing over the component's own props, drop it and call -`render()` directly. A helper that only registers `MockApiClient` -mocks, or one that takes the element to render as a parameter, is not this -pattern and is fine as it is. +Declare it at module scope so `rerender` keeps the same component type, and keep +`render()` options at the call site. If the helper adds nothing over the +component's own props, drop it and call `render()` directly. Helpers +that only register mocks, or that take the element as a parameter, are fine. ### Use `screen` instead of destructuring