Move off Enzyme, to React Testing Library
#2935
Conversation
CallWithChat bundle size is increased❗.
|
Chat bundle size is increased❗.
|
Calling bundle size is increased❗.
|
| expect(callComposite.props().rtl).toBe(mockBaseCompositeProps.rtl); | ||
| expect(callComposite.props().onFetchAvatarPersonaData).toBe(mockBaseCompositeProps.onFetchAvatarPersonaData); | ||
| expect(callComposite.props().onFetchParticipantMenuItems).toBe(mockBaseCompositeProps.onFetchParticipantMenuItems); | ||
| expect(container.querySelector('[data-ui-id="lobby-page"]')).toBeTruthy(); |
There was a problem hiding this comment.
this is the only test I changed what was tested. Before the test simply checked that the props we passed in were the props the composite received -- this doesn't make sense for a test, so I updated this to validate the composite successfully rendered a lobby page as the mock adapter depicts
JamesBurnside
requested review from
alkwa-msft,
dmceachernmsft,
carocao-msft,
edwardlee-msft,
PorterNan,
mgamis-msft,
prabhjot-msft,
AmyL219,
jimchou-dev,
vhuseinova-msft,
Leah-Xia-Microsoft,
jpeng-ms,
waynemo,
emlynmac,
JoshuaLai and
palatter
| }); | ||
|
|
||
| test('Should override button label with `strings` prop', async () => { | ||
| const testLocale = createTestLocale({ devicesButton: { label: Math.random().toString() } }); | ||
| const devicesButtonStrings = { label: Math.random().toString() }; | ||
| const component = mountWithLocalization( | ||
| renderWithLocalization( |
There was a problem hiding this comment.
so with this change does this mean these tests do actually create a DOM?
There was a problem hiding this comment.
Sort of - The testing library's terminology uses the word render, but it doesn't render anything actual graphical, just renders an in memory DOM. Because we use jest, this library lets the testing library dictate who implements that DOM so for us its https://github.com/jsdom/jsdom
| testLocale.strings.dialpad.placeholderText | ||
| ); | ||
| renderWithLocalization(<Dialpad />, testLocale); | ||
| expect(screen.getByRole('textbox').getAttribute('placeholder')).toBe(testLocale.strings.dialpad.placeholderText); |
There was a problem hiding this comment.
is screen a property that just exists inside the test runner we can query?
There was a problem hiding this comment.
Yeah, its basically a way of saying - hey query what's on the "screen" (screen isn't a real screen, just an in memory interpretation of a screen). Ideally we'd only use screen for queries but it can't query anything arbitrary (like data-ui-id) so for those I've used the container returned by the render method
| wrapper.simulate('touchend'); | ||
| fireEvent.touchStart(videoTile); | ||
| jest.runAllTimers(); | ||
| fireEvent.touchEnd(videoTile); |
There was a problem hiding this comment.
Thanks for this conversion. I was worried about triggering touch events with React Testing Library. Nicely done.
| @@ -1,14 +1,13 @@ | |||
| // Copyright (c) Microsoft Corporation. | |||
| // Licensed under the MIT license. | |||
| /* eslint-disable @typescript-eslint/no-non-null-assertion */ | |||
There was a problem hiding this comment.
Just curious why this was necessary
There was a problem hiding this comment.
Typescript isn't smart enough to know that:
var a: someType | undefined;
expect(a).toBeTruthy();
expect(a.something).toBe(...); // Typescript complains here that a might be undefined, but it actually can't be undefined because of the previous `expect`
// so I instead do:
expect(a).toBeTruthy();
expect(a!.something).toBe(...); // notice the ! after the a, this is the non-null assertion, which by default we disallow as its should be used more sparingly
| container: HTMLElement; | ||
| } => { | ||
| const { rerender, container } = render( | ||
| withLiveAnnouncerContext(<LocalizationProvider locale={locale}>{node}</LocalizationProvider>) |
There was a problem hiding this comment.
Why do we have to wrap with LiveAnnouncer?
There was a problem hiding this comment.
Just gets rid of warnings from the react-aria-live package. This warning already exists in our current tests, but this removes the warning
There was a problem hiding this comment.
(we're moving off this package shortly as it does not support react18 so this was a super quick fix to the warning)
|
Failed to pass the Static HTML UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "update_snapshots" label to the PR for updating the snapshot. |
|
Failed to pass the UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "update_snapshots" label to the PR for updating the snapshot. |
|
Failed to pass the Static HTML UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "update_snapshots" label to the PR for updating the snapshot. |
What
Migrate from Enzyme to react testing library
Why
See: Enzyme is dead. Enzyme has no, and is unlikely to have, any react 18 support. So we are migrating to a new testing package.
related: #2939, #1900
How Tested
Unit tests in CI should all pass