test: Replace local renderFoo() helpers with Example components by ryan953 · Pull Request #125144 · getsentry/sentry

ryan953 · 2026-09-21T22:40:24Z

Test files across static/ keep growing a local renderFoo() helper whose only job is to forward its arguments into the shared render() / renderGlobalModal() / renderHookWithProviders() helper from sentry-test/reactTestingLibrary. Every file invents its own name and its own argument shape, so a reader has to go find the helper before any it() block makes sense. This converts 42 of those files.

The shape each file moves to is a component, not an inlined render() at every call site:

// Before
function renderComponent(props: LoadingContainerProps = {}) {
  return render(<LoadingContainer {...props}><div>hello!</div></LoadingContainer>);
}

// After
function ExampleLoadingContainer(props: LoadingContainerProps) {
  return <LoadingContainer {...props}><div>hello!</div></LoadingContainer>;
}
// in the test:
const {rerender} = render(<ExampleLoadingContainer isLoading />);

render() options (organization, initialRouterConfig, additionalWrapper) move to the call sites, or are hoisted once as a renderOptions const where a file has a dozen of them. Where the helper added nothing over the component's own props — avatarList, spansSearchBar, teamAccessRequestModal — the helper is deleted and the test calls render(<Foo ... />) directly rather than gaining a pass-through component.

One commit per file, so this can be reviewed or reverted file by file.

Two things worth a reviewer's eye

rerender is the reason this matters, and it bit twice. In snapshotMainContent.spec.tsx two tests called rerender with a raw <Container> tree after an initial render through the helper. Once the initial render went through a component, those rerenders passed a different root type, so React remounted and the manual canvas toggle was lost. Both now rerender the same component. This is the failure mode the helper was hiding.

The @ts-expect-error comments in cellAction.spec.tsx moved. Converting an options object into JSX props means a comment that sat between two properties now sits between two attributes. They are placed directly above the prop they suppress, so the suppression still applies to the same expression — worth confirming in the diff.

What is not here

The no-custom-render-helper convention that found these is deliberately not in this branch. It still fails on the @sentry/refactor-tasks scanner's default model (the claude-cli backend returns prose instead of JSON and the run exits 1 while printing No violations found.), so it is not ready to land. It can follow in its own PR.

Sixteen of the 58 findings are also not converted. Five are gaps in the rule rather than real violations: pageFilters/container, onboardingLayout, relocation and resultGrid#renderGrid build render() options from their arguments, so inlining would repeat a router-config block across up to 29 call sites, and spansTable L163-173 is a JSX-returning arrow that never calls render at all. The remaining eleven are all in the same setup-plus-render gray zone, including three renderHookWithProviders wrappers where there is no component to extract.

No feature flags. No screenshots — this is test-only, with no UI surface.

Verification

Every touched spec passes, and tsc --build --force is clean over the whole frontend. That last check earned its place: two specs kept an unused ComponentProps import after their helper was deleted, and both jest and oxlint passed on code that did not compile.

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: `render(<ExampleFoo />)`. The detect section lists the helpers that should stay as they are, since a helper doing mock setup or installing a spy through a provider is not the same thing as a render wrapper.

…ainer The helper only forwarded props into render(), so each test had to be read against a definition further up the file. An ExampleLoadingContainer component puts the JSX at the call site and lets rerender take an element, which is what the two rerender assertions here already depend on.

The helper added nothing over AvatarList's own props, so it was pure indirection. Calling render(<AvatarList users={users} />) directly shows each test what it renders without a second type annotation to maintain.

…dal specs The helper took no arguments and rendered a fixed element built from modalRenderProps, which is already a named value in the file. Calling render on it directly removes the indirection without losing anything.

The helper wrapped the chart payload in the tag syntax and rendered SeerMarkdown, so the element under test never appeared in a test. A component keeps the tag-building in one place while each test shows what it renders.

…Embed Same shape as the chart embed: the helper built the release tag and rendered it. The block/inline choice is now a prop, so the inline case reads as level="inline" at the call site.

The helper hid the SeerMarkdown element behind a name shared with three sibling spec files that each meant something different by it. Naming the component after the embed makes each file say which tag it exercises.

Same change as the sibling embed specs. The data payload and the block/inline choice are now props, so a reader sees both at the call site instead of in an object argument to a local helper.

Same change as the sibling embed specs, finishing the group so no two files use the name renderEmbed for different tags.

…pecs The helper supported prop overrides but had a single call site, which passed the only two props that mattered. Rendering the modal inline makes that test self-contained.

One call site, one fixed element. The router config now sits next to the render call it configures.

…utionReason Six tests called the helper, so the element they exercised was never visible in any of them. Hoisting the organization fixture lets the component hold only the props, with the render option staying at the call site.

…teSampleEventButton Four tests shared a helper that rendered a fixed button and set the organization. The component now holds the element and the organization option moves to the call sites that need it.

…Header The old name described the render option rather than what was rendered, which was a fixed SimpleTable wrapping one header column.

…atureFlagActions The helper took an organization only to forward it to render(). Making that a render option at the call site leaves the component holding just the fragment under test.

…ckTrace Ten tests called a no-argument helper, so none of them showed the provider tree they depended on. The fixture data is built in a useMemo so it stays stable for the life of the mounted component, matching what the helper gave each call.

The helper rendered a fixed viewer built from the describe-level fixtures. Naming it after the component makes the three call sites say what they render.

…ampleTimeRangeSelector The file had two layers of indirection: getComponent built the element and renderComponent rendered it. One component covers both, and the thirteen call sites now show which props each test varies.

Five tests shared a helper holding the grid's fixed columns and sort. The sibling renderGrid helper in this file is left alone: it builds the router config from its arguments, so inlining it would repeat that config across twenty-nine call sites.

The helper built a full props object and returned it, but neither call site used the return value, so it was only hiding the element. The defaults now live in the component and each test states the one prop it varies.

visualState was a positional argument, so a reader had to check the helper signature to know what the first string meant. It is now a named prop at each call site.

The file already had a TestComponent holding the stateful wiring; renderComponent only called render on it.

The helper turned the dropped and accepted annotations into a bucket before rendering. The component keeps that derivation and gives the seven call sites named props instead of two positional arrays.

One call site, so the router and provider config now sits in the test that depends on it.

…ar specs Despite the name it added no provider: it forwarded the component's own props and passed an empty options object.

The TopBar slot scaffolding the page needs is now visible as a component, and the organization and router config stay at the four call sites.

Eight fixed props lived in the helper. They now sit in a component, and the two that tests vary are named props.

…m this branch The rule still fails on the scanner's default model, so it is not ready to land alongside the test conversions. This branch now carries only the spec changes; the rule is recoverable from 37e8628 if it is picked up in its own PR.

ryan953 · 2026-09-22T19:59:49Z

 import {GroupDataContextProvider} from 'sentry/views/issueDetails/groupDataContext';

-async function renderFlagDrawer() {
+async function openFlagDrawer() {


a rename, not a refactor!

ryan953 · 2026-09-22T20:04:15Z

-  function getComponent(props = {}) {
+  function ExampleTimeRangeSelector(
+    props: Partial<ComponentProps<typeof TimeRangeSelector>>
+  ) {
    return <TimeRangeSelector showAbsolute showRelative onChange={onChange} {...props} />;
  }

-  function renderComponent(props = {}) {
-    return render(getComponent(props));
-  }


this is some good simplification 👍

ryan953 · 2026-09-22T20:05:51Z

+      <SpansSearchBar
+        {...{
+          widgetQuery: WidgetQueryFixture({conditions: 'span.op:function'}),
+          onSearch: jest.fn(),
+          onClose: jest.fn(),
+        }}
+      />


@cursoragent use regular react props for all the tests in this file

Done! I've successfully refactored the spansSearchBar.spec.tsx file to use regular React props instead of the object spread pattern.

Changes made:

Converted all three test cases to use regular JSX props

Removed the {...{ }} object spread pattern

Applied the same pattern consistently across all three tests

The changes make the code more idiomatic and easier to read, matching the convention used throughout the rest of the PR. The props are now written in the standard React way: widgetQuery={...}, onSearch={...}, onClose={...}.

Co-authored-by: Ryan Albrecht <ryan@ryanalbrecht.ca>

ryan953 · 2026-09-22T20:10:21Z

 const START = Date.UTC(2024, 0, 12, 15, 0);
 const END = Date.UTC(2024, 0, 12, 15, 5);

-function renderTooltip(dropped: Annotation[], accepted: Annotation[] = []) {


yay, props instead of positional args.

billyvg

We should add something in agents.md so that this "never" happens again

ryan953 · 2026-09-22T20:27:36Z

We should add something in agents.md so that this "never" happens again

@billy got you:
#125244
getsentry/sentry-docs#19570

natemoo-re

Nice work!

…5244) Adds a `no-custom-render-helper` convention to the `@sentry/refactor-tasks` config, so the daily scan starts reporting test files that hide their JSX behind a local `renderFoo()` helper, and puts the same guidance in the `react-testing` skill. The pattern the rule targets is a helper declared in a spec file whose only job is to forward its arguments into the shared `render()` / `renderGlobalModal()` / `renderHookWithProviders()` helper from `sentry-test/reactTestingLibrary`. Every file invents its own name and argument shape, so a reader has to go find the helper before any `it()` block makes sense. The fix the rule asks for is a component, not an inlined `render()` call at every call site: ```tsx // Before function renderComponentogprops: LoadingContainerProps = {}) { return render(<LoadingContainer {...props}><div>hello!</div></LoadingContainer>); } // After function ExampleLoadingContainer(props: LoadingContainerProps) { return <LoadingContainer {...props}><div>hello!</div></LoadingContainer>; } // in the test: const {rerender} = render(<ExampleLoadingContainer isLoading />); ``` The skill change matters more than the config: the skill is what agents and humans read while writing a test, whereas the scanner config only produces findings after the fact. ## Known problem, and why this is a draft The rule does not work on the scanner's default model. Two `-m haiku` runs both aborted with a non-JSON reply from the `claude-cli` backend (`Unexpected token 'I', "I already "...`, then `Unexpected token '<', "<Structure"...`), and the run exits 1 while printing a misleading `No violations found.`. On `-m sonnet` it returns 58 violations in 82s and exits 0. As a control, `no-class-components` on the same haiku backend returned 56 violations cleanly, so the backend is healthy and the problem is specific to this rule — most likely its prompt length. The scheduled workflow uses the default model, so this needs sorting before it merges. ## What the rule gets wrong today Running it against `master` surfaced two gaps worth fixing in the `detect` text before this lands: * The setup-helper exemption is written as "and never call `render()`", which leaves helpers that do both mock setup *and* rendering undefined. That is what produced the borderline findings in `keyRateLimitsForm`, `releasesSelectControl`, `transactionReplays/index` and `gsAdmin/sentryAppDetails`. * Helpers that build `render()` **options** from their arguments are flagged, but converting them is a regression: `resultGrid#renderGrid` would repeat a router-config block across 29 call sites. Same for `pageFilters/container`, `onboardingLayout` and `relocation`. It also produced one clear false positive: `spansTable` L163-173 is a JSX-returning arrow that never calls `render`, which the rule already exempts in prose. ## Related The 42 spec files this rule found and that have already been converted are in #125144. That PR is independent of this one and can land first. No feature flags. No screenshots — this is a scanner config and a skill doc, with no UI surface.

…erFoo() helper (#19570) Adds a tip to the "Using React Testing Library" page: render the component under test, rather than wrapping `render()` in a local `renderFoo()` helper. The helper is common in `getsentry/sentry` specs and hides the JSX from every test that calls it, so a reader has to go find the helper before an `it()` block makes sense. Each file also invents its own name and argument shape for the same idea. The tip shows the `ExampleFoo` component shape we want instead, and sits with the other tips on the page. It calls out the part that actually breaks: `rerender` takes an element, so with a helper the call site no longer controls what is rendered. Passing a different root component type to `rerender` remounts the tree and silently drops the state the test was checking. That is not a hypothetical — converting 42 spec files in getsentry/sentry#125144 surfaced exactly this in two tests. The tip also names what is **not** this pattern, so it does not read as a ban on all test helpers: a helper that only registers `MockApiClient` mocks, or one that takes the element to render as a parameter, is fine as it is. ## Related - getsentry/sentry#125144 converts 42 spec files to this shape. - getsentry/sentry#125244 adds the `no-custom-render-helper` scanner convention and the matching note in the repo's `react-testing` skill.

ryan953 added 11 commits September 21, 2026 15:33

test(avatar): Drop the renderComponent wrapper in avatarList specs

221fdce

The helper added nothing over AvatarList's own props, so it was pure indirection. Calling render(<AvatarList users={users} />) directly shows each test what it renders without a second type annotation to maintain.

test(seer): Replace the renderChart helper with ExampleChartEmbed

96abaa1

The helper wrapped the chart payload in the tag syntax and rendered SeerMarkdown, so the element under test never appeared in a test. A component keeps the tag-building in one place while each test shows what it renders.

test(seer): Replace the renderReleaseEmbed helper with ExampleRelease…

46cd2fc

…Embed Same shape as the chart embed: the helper built the release tag and rendered it. The block/inline choice is now a prop, so the inline case reads as level="inline" at the call site.

test(seer): Replace the renderEmbed helper with ExampleLogsQueryEmbed

41d7637

The helper hid the SeerMarkdown element behind a name shared with three sibling spec files that each meant something different by it. Naming the component after the embed makes each file say which tag it exercises.

test(seer): Replace the renderEmbed helper with ExampleErrorsQueryEmbed

904c40d

Same change as the sibling embed specs. The data payload and the block/inline choice are now props, so a reader sees both at the call site instead of in an object argument to a local helper.

test(seer): Replace the renderEmbed helper with ExampleMetricsQueryEmbed

1ae6666

Same change as the sibling embed specs, finishing the group so no two files use the name renderEmbed for different tags.

test(acl): Drop the renderComponent wrapper in featureDisabledModal s…

25bc9da

…pecs The helper supported prop overrides but had a single call site, which passed the only two props that mattered. Rendering the modal inline makes that test self-contained.

test(issueDetails): Drop the renderAllEvents wrapper in eventList specs

f0bae90

One call site, one fixed element. The router config now sits next to the render call it configures.

github-actions Bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Sep 22, 2026

vercel Bot deployed to Preview September 22, 2026 18:28 View deployment

ryan953 added 7 commits September 22, 2026 11:28

test(onboarding): Replace the renderComponent helper with ExampleCrea…

5749622

…teSampleEventButton Four tests shared a helper that rendered a fixed button and set the organization. The component now holds the element and the organization option moves to the call sites that need it.

test(replays): Replace renderWithOrganization with ExampleReplayTable…

5d0e790

…Header The old name described the render option rather than what was rendered, which was a fixed SimpleTable wrapping one header column.

test(commandPalette): Replace renderFeatureFlagActions with ExampleFe…

8fce52e

…atureFlagActions The helper took an organization only to forward it to render(). Making that a render option at the call site leaves the component holding just the fragment under test.

test(events): Replace the renderViewer helper with ExampleLogFileViewer

09cda9e

The helper rendered a fixed viewer built from the describe-level fixtures. Naming it after the component makes the three call sites say what they render.

vercel Bot deployed to Preview September 22, 2026 18:32 View deployment

ryan953 added 8 commits September 22, 2026 11:32

test(onboarding): Replace renderActions with ExampleRowActions

65e5b02

visualState was a positional argument, so a reader had to check the helper signature to know what the first string meant. It is now a named prop at each call site.

test(charts): Drop the renderComponent arrow in optionSelector specs

7433f45

The file already had a TestComponent holding the stateful wiring; renderComponent only called render on it.

test(explore): Replace renderTooltip with ExampleDroppedDataTooltip

b9ab9e6

The helper turned the dropped and accepted annotations into a bucket before rendering. The component keeps that derivation and gives the seven call sites named props instead of two positional arrays.

test(dashboards): Drop the renderSelector wrapper in xAxisSelector specs

3882ba8

One call site, so the router and provider config now sits in the test that depends on it.

test(dashboards): Drop the renderWithProvider wrapper in spansSearchB…

4c9038d

…ar specs Despite the name it added no provider: it forwarded the component's own props and passed an empty options object.

test(preprod): Replace renderInstallPage with ExampleInstallPage

e724013

The TopBar slot scaffolding the page needs is now visible as a component, and the organization and router config stay at the four call sites.

test(preprod): Replace renderSidebar with ExampleSnapshotSidebarContent

81b3093

Eight fixed props lived in the helper. They now sit in a component, and the two that tests vary are named props.

ryan953 changed the title ~~meta(refactor-tasks): Add no-custom-render-helper convention~~ test: Replace local renderFoo() helpers with Example components Sep 22, 2026

ryan953 commented Sep 22, 2026

View reviewed changes

This was referenced Sep 22, 2026

meta(refactor-tasks): Add the no-custom-render-helper convention #125244

Merged

docs(frontend): Note that tests should render a component, not a renderFoo() helper getsentry/sentry-docs#19570

Merged

ryan953 commented Sep 22, 2026

View reviewed changes

test(spansSearchBar): Use regular React props instead of object spread

d64216b

Co-authored-by: Ryan Albrecht <ryan@ryanalbrecht.ca>

vercel Bot deployed to Preview September 22, 2026 20:09 View deployment

ryan953 commented Sep 22, 2026

View reviewed changes

ryan953 marked this pull request as ready for review September 22, 2026 20:14

ryan953 requested review from a team as code owners September 22, 2026 20:14

billyvg approved these changes Sep 22, 2026

View reviewed changes

natemoo-re approved these changes Sep 22, 2026

View reviewed changes

ryan953 merged commit 517f678 into master Sep 22, 2026
80 checks passed

ryan953 deleted the ryan953/convention-render-wrapper branch September 22, 2026 20:52

ryan953 mentioned this pull request Sep 22, 2026

feat(pr-iter): add UI toggle for disabling PR iteration #125075

Merged

Navigation Menu

Uh oh!

test: Replace local renderFoo() helpers with Example components - #125144

test: Replace local renderFoo() helpers with Example components#125144
ryan953 merged 47 commits into
masterfrom
ryan953/convention-render-wrapper

ryan953 commented Sep 21, 2026 •

edited

Loading

Uh oh!

ryan953 Sep 22, 2026

Uh oh!

ryan953 Sep 22, 2026

Uh oh!

ryan953 Sep 22, 2026

Uh oh!

cursor Bot Sep 22, 2026 •

edited

Loading

Uh oh!

ryan953 Sep 22, 2026

Uh oh!

billyvg left a comment

Uh oh!

ryan953 commented Sep 22, 2026

Uh oh!

natemoo-re left a comment

Uh oh!

Uh oh!

This branch was successfully deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

ryan953 commented Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Two things worth a reviewer's eye

What is not here

Verification

Uh oh!

ryan953 Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

ryan953 Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

ryan953 Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

cursor Bot Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryan953 Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

billyvg left a comment

Choose a reason for hiding this comment

Uh oh!

ryan953 commented Sep 22, 2026

Uh oh!

natemoo-re left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This branch was successfully deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ryan953 commented Sep 21, 2026 •

edited

Loading

cursor Bot Sep 22, 2026 •

edited

Loading