getsentry / sentry

Developer-first error tracking and performance monitoring
https://sentry.io
Other
39.07k stars 4.19k forks source link

Upgrade to FE to use React 18 #54432

Closed AbhiPrasad closed 6 months ago

AbhiPrasad commented 1 year ago

Problem Statement

### Investigate
- [ ] https://github.com/getsentry/sentry/pull/54686
### Tasks
- [ ] Upgrade to React 18
- [ ] Upgrade react-testing-library v13 https://github.com/testing-library/react-testing-library/releases/tag/v13.0.0
- [ ] Switch to new react-dom methods
- [ ] Squash `The current testing environment is not configured to support act` errors

https://react.dev/blog/2022/03/29/react-v18

Upgrade guide: https://react.dev/blog/2022/03/08/react-18-upgrade-guide

We'll get:

Solution Brainstorm

To investigate

Product Area

Unknown

AbhiPrasad commented 1 year ago

Sample try when just bumping deps: https://github.com/getsentry/sentry/pull/54686

test failures: https://github.com/getsentry/sentry/actions/runs/5857714436/

TypeError: Cannot read properties of undefined (reading 'current')

It seems that reactHooks.renderHook (from @testing-library/react-hooks) returns an undefined current.

ref https://github.com/testing-library/react-hooks-testing-library#a-note-about-react-18-support, it seems that renderHook is not supported with React 18 and @testing-library/react-hooks at all (even if you use the old renderer)

We probably need to swap to import {renderHook} from '@testing-library/react', but that requires us to upgrade testing library the same time we do react.

An alternative is to swap out all usage of renderHook with our own internal helper (that just vendors in what is in https://github.com/testing-library/react-testing-library/pull/991)

FAIL  static/app/views/replays/detail/network/useSortNetwork.spec.tsx
● useSortNetwork › should the list by timestamp by default

  TypeError: Cannot read properties of undefined (reading 'current')

    94 |     },
    95 |   }),
  > 96 |   TestStubs.Replay.RequestFrame({
       |                                         ^
    97 |     op: 'resource.fetch',
    98 |     description: 'https://pokeapi.co/api/v2/pokemon/mewtu',
    99 |     startTimestamp: new Date(1663131120.198),

    at act (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:15259:59)
    at render (node_modules/@testing-library/react-hooks/lib/native/pure.js:73:34)
    at Object.renderHook (node_modules/@testing-library/react-hooks/lib/core/index.js:114:5)
    at Object.<anonymous> (static/app/views/replays/detail/network/useSortNetwork.spec.tsx:96:41)

Unable to find an element with the text: Nested Container - nested

It seems that expect(await screen.findByText('X')).toBeInTheDocument(); and similar does not work in certain scenarios. Not sure when/why exactly though.

FAIL  static/app/components/events/eventViewHierarchy.spec.tsx
● Event View Hierarchy › does not collapse all nodes when update triggers re-render

  Unable to find an element with the text: Nested Container - nested. This could be because the text is broken up by multiple elements. In this case, you can provide a function for your text matcher to make your matcher more flexible.

  Ignored nodes: comments, <script />, <style />
  ...

    91 |       }
    92 |     );
  > 93 |
       | ^
    94 |     expect(await screen.findByText('Nested Container - nested')).toBeInTheDocument();
    95 |
    96 |     rerender(<EventViewHierarchy project={mockProject} event={event} />);

    at waitForWrapper (node_modules/@testing-library/dom/dist/wait-for.js:187:27)
    at node_modules/@testing-library/dom/dist/query-helpers.js:101:33
    at Object.<anonymous> (static/app/components/events/eventViewHierarchy.spec.tsx:93:46)

Timed out in waitForElementToBeRemoved

Some timeouts, but this might just be a flake, nothing confirmed.

FAIL  static/app/views/onboarding/setupDocs.spec.tsx (7.118 s)
● Onboarding Setup Docs › renders Product Selection › only error monitoring checked

  Timed out in waitForElementToBeRemoved.

  Ignored nodes: comments, <script />, <style />
  ...

    267 |       renderMockRequests({
    268 |         project,
  > 269 |         orgSlug: organization.slug,
        |                                                                ^
    270 |       });
    271 |
    272 |       render(

    at waitForElementToBeRemoved (node_modules/@testing-library/dom/dist/wait-for-element-to-be-removed.js:22:24)
    at Object.<anonymous> (static/app/views/onboarding/setupDocs.spec.tsx:269:64)
scttcper commented 1 year ago

I think in TSC we talked about using 18 with the legacy renderer and migrating after to the new react 18 renderer. This will break the migration into easier chunks since we can get both repos to react 18 and the test updates and then migrate to the new renderer (and probably fix tests again).

Proposed path:

React Hook test migration

Shim @testing-library/react-hooks

@testing-library/react-hooks is now exported directly from RTL. We'll need to export an object that has a similar shape until we migrate all the hook tests to importing renderHook directly.

// In sentry-test/reactTestingLibrary
export const reactHooks = {renderHook: rtl.renderHook, act: rtl.act, waitFor: rtl.waitFor};

Hooks using rerender() need to have props

they no longer keep their initial props

const {result, rerender} = reactHooks.renderHook(useTimeout, {
      initialProps: {timeMs, onTimeout},
});
// Before
rerender()
// After
rerender({timeMs, onTimeout})

Swap waitForNextUpdate for waitFor

should be straight forward. waitForNextUpdate is removed

Remove the returned waitFor from reactHooks.renderHook

waitFor is no longer returned, use the regular one

const {result, waitFor} = reactHooks.renderHook

Figure out how to catch errors? Maybe skip these tests

In some react hook tests we throw an error and check result.error. This no longer works and we'll need to do something else. Maybe skip the tests until then, mostly low value https://github.com/getsentry/sentry/blob/79bb00c53190311a8eacec7d4ed7a578a9bb7099/static/app/components/replays/useReplaysCount.spec.tsx#L31-L38

Remove act from hook tests

it stops working in the next version, should be using waitFor probably https://github.com/getsentry/sentry/blob/master/static/app/views/replays/detail/network/useSortNetwork.spec.tsx#L178-L180

Component test migration

Change RTL jest import

Instead of '@testing-library/jest-dom/extend-expect', in the jest.config. add import '@testing-library/jest-dom'; to the test setup.

Ignore react 18 warning

in setupFramework.ts add

  if (
    /Warning: ReactDOM.render is no longer supported in React 18/.test(errorMessage)
  ) {
    return true;
  }

Use a regular date mock jest.useFakeTimers().setSystemTime

Some tests are using fake timers to mock the date, this is not how we should be freezing the date since it breaks timeouts etc. We have a date mock library we can switch to. https://github.com/getsentry/sentry/blob/79bb00c53190311a8eacec7d4ed7a578a9bb7099/static/app/views/dashboards/widgetBuilder/widgetBuilderDataset.spec.tsx#L403

Fix various act errors

@AbhiPrasad covered this. Need to use more await screen.findBy in tests to silence act warnings that were previously hidden.

fix various waitForElementToBeRemoved

@AbhiPrasad covered this. just swap these for waitFor(() => expect(screen.queryBy()).not.toBeInTheDocument()) or remove

Other things to do

here's where i just kinda mashed a bunch together https://github.com/getsentry/sentry/pull/57987/files