diff --git a/.agents/skills/react-testing/SKILL.md b/.agents/skills/react-testing/SKILL.md index c8ef58a89694..922dcd37bfd3 100644 --- a/.agents/skills/react-testing/SKILL.md +++ b/.agents/skills/react-testing/SKILL.md @@ -120,6 +120,29 @@ const project = ProjectFixture(partialProject) ``` +### Render a component, not a `renderFoo()` helper + +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 +// ❌ Don't wrap render() in a helper +function renderComponent(props: Props = {}) { + return render(hello); +} + +// ✅ Put the fixed parts in a component +function ExampleWidget(props: Props) { + return hello; +} +render(); +``` + +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 ```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/"