test: React 18 snapshot adjustments #6944

tkajtoch · 2023-07-13T13:37:42Z

Summary

Note: This branch contains changes from #6943 and needs rebasing when that one is merged.

This PR fixes all snapshot mismatches when running on different versions of React.

It adds describeByReactVersion and testByReactVersion test utilities to separate snapshots by currently ran version of React for parts of the code where small implementation differences generate different snapshots, specifically hello-pangea/dnd using different unique ID generators depending on version of React used (https://github.com/hello-pangea/dnd/blob/b1340b18b2987a31426fe6743b5ce3004e09fa08/src/view/use-unique-id.ts#L17-L37)

QA

Checkout this branch locally
Run yarn to install dependencies
Run REACT_VERSION=17 yarn test-unit and ensure there are no errors
Run yarn test-unit and ensure there are not snapshot errors different than class attribute differences.
This needs an improved emotion class name serializer that I'll add in a separate PR.

…ions

…yReactVersion utility

…y v14 (react 18-compatible) serializing and earlier versions

tkajtoch · 2023-07-13T15:57:34Z

src/components/highlight/highlight.tsx

  return (
    <Fragment>
-      {preMatch}
+      {preMatch || undefined}


None of these || undefined changes affect output DOM, but they do unify how React Virtual DOM is being serialized no matter what versions of React and RTL are used.

tkajtoch · 2023-07-13T15:58:23Z

src/components/markdown_editor/__snapshots__/markdown_editor.test.tsx.snap

        id="editorId"
        rows="6"
-        style="height:100%;max-height:"
+        style="height:100%"


React 16-17 and 18 serializers generate different results here. The code underneath unifies it while keeping it working the same.

breehall

Hey Tomasz, I pulled this branch locally and didn't have any issues when I ran REACT_VERSION=17 yarn test-unit, but when running yarn test-unit, I did come across quite a few snapshot failures. I didn't have any issue with running yarn, so I don't think it has anything to do with me using an M1, but I never rule it out. There were about 377 snapshot failures.

I have also few questions about this process if that's ok!

How do we determine which components need to have test cases that target React 17 or React 18?
How long will we have to maintain two sets of test cases for the components that do require it?

breehall · 2023-07-13T19:24:43Z

src/components/markdown_editor/markdown_editor_text_area.tsx

+    // Ignore invalid empty string style values
+    const style: CSSProperties = {};
+    if (height !== '') {
+      style.height = height;
+    }
+    if (maxHeight !== '') {
+      style.maxHeight = maxHeight;
+    }
+


Super clean! 🧽

tkajtoch · 2023-07-13T20:03:39Z

Hi @breehall! Thanks for testing the code locally! I forgot to mention that snapshots failing on React 18 because of class attribute mismatch should be ignored in this PR because they need emotion class name serializer adjustments that I'll add separately.

How do we determine which components need to have test cases that target React 17 or React 18?

Realistically having tests targeting different React versions should be very rare. In our case we only need it because our drag and drop library @hello-pangea/dnd uses two different random ID generators - one for React 18 and one for lower versions. This causes snapshots to be different between versions. So that's only the tests that render EuiDraggable or EuiDroppable somewhere in their DOM tree.

How long will we have to maintain two sets of test cases for the components that do require it?

Technically the test cases are the same, but their snapshots differ between React versions due to tiny changes here and there. Since we only need them because of the DND library, I'd say there are at least two ways of getting rid of them:

Replacing snapshot tests for these components with more specific DOM querying (expecting elements and so on)
If possible, unifying the two random ID generators used in @hello-pangea/dnd so they generate the same results no matter what version of React they're executed with.

breehall

Thank you so much for your explanations! Per your guidance, I'll skip over the test failures for now.

cee-chen · 2023-07-13T20:42:45Z

Replacing snapshot tests for these components with more specific DOM querying (expecting elements and so on)

This would have been my strong preference, personally, especially if the react-drag/drop components are the only ones with this issue.

The other major consideration to keep in mind here is how downstream Kibana snapshots are going to be affected. One option we could take is using our .testenv mocks to remove flakiness between renders and for, e.g. <EuiDraggable> or <EuiDroppable> items, to return a static <span> or similar instead of rendering out the actual DOM (which is what we currently do for EuiIcon, for example: https://github.com/elastic/eui/blob/c7b8818d97ca961b0ed0996fd1bb91a1bf222f23/src/components/icon/icon.testenv.tsx)

tkajtoch · 2023-07-13T20:45:42Z

@cee-chen @breehall I just remembered what I wanted to add to my previous comment. None of our customers will need to use the conditional describe or test functions when running their tests. It exists purely to handle our React version switcher logic together with the huge amount of snapshot tests we have in place.

cee-chen · 2023-07-13T20:51:28Z

Gotcha, that's definitely fair that consumers (I hope to god, anyway) won't be in the same situation as us in terms of having to support multiple versions of React.

The .testenv option is worth keeping in mind if we run into any more snapshot-related scenarios where it feels easier to stub out the DOM than to try and print it during the React 18 feature work.

* test: unify style properties values between React 18 and earlier versions * test: Separate Droppable snapshots by React version and add describeByReactVersion utility * test: separate Draggable snapshots by React version * test: separate column selector snapshots by React version * test: separate column sorting snapshots by React version * fix: unify empty string rendering in snapshots between testing-library v14 (react 18-compatible) serializing and earlier versions * docs: add testByReactVersion jsdoc

tkajtoch self-assigned this Jul 13, 2023

tkajtoch requested a review from a team July 13, 2023 14:08

tkajtoch added 6 commits July 13, 2023 17:54

test: unify style properties values between React 18 and earlier vers…

e17f663

…ions

test: Separate Droppable snapshots by React version and add describeB…

4448bb9

…yReactVersion utility

test: separate Draggable snapshots by React version

f51a1a2

test: separate column selector snapshots by React version

c907d8f

test: separate column sorting snapshots by React version

b63c22f

fix: unify empty string rendering in snapshots between testing-librar…

9c8441d

…y v14 (react 18-compatible) serializing and earlier versions

tkajtoch force-pushed the test/react-18-snapshot-adjustments branch from 9a6d2c3 to 9c8441d Compare July 13, 2023 15:54

tkajtoch commented Jul 13, 2023

View reviewed changes

docs: add testByReactVersion jsdoc

908c14c

breehall reviewed Jul 13, 2023

View reviewed changes

breehall approved these changes Jul 13, 2023

View reviewed changes

tkajtoch merged commit bf846b9 into elastic:feature/react-18 Jul 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test: React 18 snapshot adjustments #6944

test: React 18 snapshot adjustments #6944

Uh oh!

tkajtoch commented Jul 13, 2023 •

edited

Loading

Uh oh!

tkajtoch Jul 13, 2023

Uh oh!

tkajtoch Jul 13, 2023 •

edited

Loading

Uh oh!

breehall left a comment

Uh oh!

breehall Jul 13, 2023

Uh oh!

tkajtoch commented Jul 13, 2023

Uh oh!

breehall left a comment

Uh oh!

cee-chen commented Jul 13, 2023

Uh oh!

tkajtoch commented Jul 13, 2023

Uh oh!

cee-chen commented Jul 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

test: React 18 snapshot adjustments #6944

test: React 18 snapshot adjustments #6944

Uh oh!

Conversation

tkajtoch commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

QA

Uh oh!

tkajtoch Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

tkajtoch Jul 13, 2023 • 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!

breehall left a comment

Choose a reason for hiding this comment

Uh oh!

breehall Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

tkajtoch commented Jul 13, 2023

Uh oh!

breehall left a comment

Choose a reason for hiding this comment

Uh oh!

cee-chen commented Jul 13, 2023

Uh oh!

tkajtoch commented Jul 13, 2023

Uh oh!

cee-chen commented Jul 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tkajtoch commented Jul 13, 2023 •

edited

Loading

tkajtoch Jul 13, 2023 •

edited

Loading