Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix erroneous react-dom usage in native tests #37921

Merged

Conversation

dcalhoun
Copy link
Member

@dcalhoun dcalhoun commented Jan 12, 2022

Description

Reinstate react-platform.native.js file, which was removed in b7b62d2, as it was no longer strictly necessary. However, the removal led to Jest loading the DOM-specific version of the file when mocking Gutenberg modules that depended upon react-platform.js. Jest would seemingly load react-dom when attempting to auto-mock the module, e.g. reusable-block-types-tab.native.js > use-select/index.js > @wordpress/element > react-platform.js. Reinstating react-platform.native.js addresses this issue by ensuring that Jest does not encounter an import of react-dom.

react-dom Error Details
TypeError: Cannot use 'in' operator to search for 'WebkitAnimation' in undefined

  at getVendorPrefixedEventName (node_modules/react-dom/cjs/react-dom.development.js:5011:58)
  at node_modules/react-dom/cjs/react-dom.development.js:5019:21
  at Object.<anonymous> (node_modules/react-dom/cjs/react-dom.development.js:26261:5)
  at Object.<anonymous> (node_modules/react-dom/index.js:37:20)

Additionally, these changes fix a false-positive ReactNativeEditor test. This test failed when run in isolation. It would appear it was dependent upon the jest.mock('../setup') found in sibling tests.

After defining a mock for this specific test, it was discovered that the assertion did not await the asynchronous query found within the test. The assertion resulted in a false-positive as the returned query Promise technically matches toBeDefined. async/await was added to ensure the test assertion awaits the query result, as well as fixes the following related log warning.

Async Test Warning
A worker process has failed to exit gracefully and has been force exited. This is likely caused by tests leaking due to improper teardown. Try running with --detectOpenHandles to find leaks. Active timers can also cause this, ensure that .unref() was called on them.

How has this been tested?

  • Verified native tests pass locally.
  • Verified native Demo editor launched on device without issue.

Screenshots

n/a

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).
  • I've updated related schemas if appropriate.

This file was removed in the following commit, as it was no longer
strictly necessary. b7b62d2#diff-f02069349f238fb47a268bb7fcc03c9768331db18ead0e28d8ecad7bbc05037c

However, the removal led to Jest loading the DOM-specific version of the
file when mocking Gutenberg modules that depended upon
`react-platform.js`. Jest would seemingly load `react-dom` when
attempting to auto-mock a module, e.g. `jest.mock( '@wordpress/data/src/components/use-select' );`.

The loading of `react-dom` resulted in the following error:

```
TypeError: Cannot use 'in' operator to search for 'WebkitAnimation' in undefined

  at getVendorPrefixedEventName (node_modules/react-dom/cjs/react-dom.development.js:5011:58)
  at node_modules/react-dom/cjs/react-dom.development.js:5019:21
  at Object.<anonymous> (node_modules/react-dom/cjs/react-dom.development.js:26261:5)
  at Object.<anonymous> (node_modules/react-dom/index.js:37:20)
```

Reinstating `react-platform.native.js` addresses this issue by ensuring
that Jest does not encounter an import of `react-dom`.
This test failed when run in isolation. It would appear it was dependent
upon the `jest.mock('../setup')` found in sibling tests.

After defining a mock for this specific test, it was discovered that the
assertion did not await the asynchronous query found within the test.
The assertion resulted in a false-positive as the returned query
`Promise` technically matches `toBeDefined`. `async`/`await` was added
to ensure the test assertion awaits the query result, as well as fixes
the following related log warning.

```
A worker process has failed to exit gracefully and has been force exited. This is likely caused by tests leaking due to improper teardown. Try running with --detectOpenHandles to find leaks. Active timers can also cause this, ensure that .unref() was called on them.
```
@dcalhoun dcalhoun added [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Type] Bug An existing feature does not function as intended labels Jan 12, 2022
Comment on lines +181 to +186
jest.mock( '../setup', () => {
return {
__esModule: true,
default: jest.fn( () => <MockEditor /> ),
};
} );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test failed when run in isolation, as it appeared dependent upon the mock of ../setup found in sibling tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's interesting, my idea for this test case was actually to verify that the entire editor is initialized including rendering the initial HTML. In any case, and in order to prevent blocking the PR and Jest upgrade, we could stick with this change and tackle my original idea in a future PR 👍 .

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fluiddot I leveraged a mock of ../setup as a quick fix to avoid errors from additional dependencies of the ../setup component. I think asserting against a deeper render makes a lot of sense. 👍🏻

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any case, and in order to prevent blocking the PR and Jest upgrade, we could stick with this change and tackle my original idea in a future PR 👍 .

@dcalhoun heads up that I created a PR related to this comment.

} );

const screen = initGutenberg();
const blockList = await waitFor( () =>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The missing await for the asynchronous query meant the test assertion succeeded as a false-positive.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry for that, I fell into the same trap when adding other integration tests in the past 🤦 . Thanks for catching it 🙇 !

I did an identical test in GB-mobile (reference), so I'll apply the same fix there 🔧 .

@github-actions
Copy link

Size Change: 0 B

Total Size: 1.13 MB

ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 960 B
build/admin-manifest/index.min.js 1.1 kB
build/annotations/index.min.js 2.75 kB
build/api-fetch/index.min.js 2.21 kB
build/autop/index.min.js 2.12 kB
build/blob/index.min.js 459 B
build/block-directory/index.min.js 6.28 kB
build/block-directory/style-rtl.css 1.01 kB
build/block-directory/style.css 1.01 kB
build/block-editor/default-editor-styles-rtl.css 378 B
build/block-editor/default-editor-styles.css 378 B
build/block-editor/index.min.js 140 kB
build/block-editor/style-rtl.css 14.6 kB
build/block-editor/style.css 14.6 kB
build/block-library/blocks/archives/editor-rtl.css 61 B
build/block-library/blocks/archives/editor.css 60 B
build/block-library/blocks/archives/style-rtl.css 65 B
build/block-library/blocks/archives/style.css 65 B
build/block-library/blocks/audio/editor-rtl.css 150 B
build/block-library/blocks/audio/editor.css 150 B
build/block-library/blocks/audio/style-rtl.css 111 B
build/block-library/blocks/audio/style.css 111 B
build/block-library/blocks/audio/theme-rtl.css 125 B
build/block-library/blocks/audio/theme.css 125 B
build/block-library/blocks/block/editor-rtl.css 161 B
build/block-library/blocks/block/editor.css 161 B
build/block-library/blocks/button/editor-rtl.css 470 B
build/block-library/blocks/button/editor.css 470 B
build/block-library/blocks/button/style-rtl.css 560 B
build/block-library/blocks/button/style.css 560 B
build/block-library/blocks/buttons/editor-rtl.css 292 B
build/block-library/blocks/buttons/editor.css 292 B
build/block-library/blocks/buttons/style-rtl.css 275 B
build/block-library/blocks/buttons/style.css 275 B
build/block-library/blocks/calendar/style-rtl.css 207 B
build/block-library/blocks/calendar/style.css 207 B
build/block-library/blocks/categories/editor-rtl.css 84 B
build/block-library/blocks/categories/editor.css 83 B
build/block-library/blocks/categories/style-rtl.css 79 B
build/block-library/blocks/categories/style.css 79 B
build/block-library/blocks/code/style-rtl.css 90 B
build/block-library/blocks/code/style.css 90 B
build/block-library/blocks/code/theme-rtl.css 131 B
build/block-library/blocks/code/theme.css 131 B
build/block-library/blocks/columns/editor-rtl.css 108 B
build/block-library/blocks/columns/editor.css 108 B
build/block-library/blocks/columns/style-rtl.css 406 B
build/block-library/blocks/columns/style.css 406 B
build/block-library/blocks/comment-template/style-rtl.css 127 B
build/block-library/blocks/comment-template/style.css 127 B
build/block-library/blocks/comments-pagination-numbers/editor-rtl.css 123 B
build/block-library/blocks/comments-pagination-numbers/editor.css 121 B
build/block-library/blocks/comments-pagination/editor-rtl.css 222 B
build/block-library/blocks/comments-pagination/editor.css 209 B
build/block-library/blocks/comments-pagination/style-rtl.css 235 B
build/block-library/blocks/comments-pagination/style.css 231 B
build/block-library/blocks/cover/editor-rtl.css 546 B
build/block-library/blocks/cover/editor.css 547 B
build/block-library/blocks/cover/style-rtl.css 1.22 kB
build/block-library/blocks/cover/style.css 1.22 kB
build/block-library/blocks/embed/editor-rtl.css 293 B
build/block-library/blocks/embed/editor.css 293 B
build/block-library/blocks/embed/style-rtl.css 417 B
build/block-library/blocks/embed/style.css 417 B
build/block-library/blocks/embed/theme-rtl.css 124 B
build/block-library/blocks/embed/theme.css 124 B
build/block-library/blocks/file/editor-rtl.css 300 B
build/block-library/blocks/file/editor.css 300 B
build/block-library/blocks/file/style-rtl.css 255 B
build/block-library/blocks/file/style.css 255 B
build/block-library/blocks/file/view.min.js 322 B
build/block-library/blocks/freeform/editor-rtl.css 2.44 kB
build/block-library/blocks/freeform/editor.css 2.44 kB
build/block-library/blocks/gallery/editor-rtl.css 965 B
build/block-library/blocks/gallery/editor.css 967 B
build/block-library/blocks/gallery/style-rtl.css 1.6 kB
build/block-library/blocks/gallery/style.css 1.6 kB
build/block-library/blocks/gallery/theme-rtl.css 122 B
build/block-library/blocks/gallery/theme.css 122 B
build/block-library/blocks/group/editor-rtl.css 159 B
build/block-library/blocks/group/editor.css 159 B
build/block-library/blocks/group/style-rtl.css 57 B
build/block-library/blocks/group/style.css 57 B
build/block-library/blocks/group/theme-rtl.css 78 B
build/block-library/blocks/group/theme.css 78 B
build/block-library/blocks/heading/style-rtl.css 114 B
build/block-library/blocks/heading/style.css 114 B
build/block-library/blocks/html/editor-rtl.css 332 B
build/block-library/blocks/html/editor.css 333 B
build/block-library/blocks/image/editor-rtl.css 810 B
build/block-library/blocks/image/editor.css 809 B
build/block-library/blocks/image/style-rtl.css 507 B
build/block-library/blocks/image/style.css 511 B
build/block-library/blocks/image/theme-rtl.css 124 B
build/block-library/blocks/image/theme.css 124 B
build/block-library/blocks/latest-comments/style-rtl.css 284 B
build/block-library/blocks/latest-comments/style.css 284 B
build/block-library/blocks/latest-posts/editor-rtl.css 137 B
build/block-library/blocks/latest-posts/editor.css 137 B
build/block-library/blocks/latest-posts/style-rtl.css 528 B
build/block-library/blocks/latest-posts/style.css 527 B
build/block-library/blocks/list/style-rtl.css 94 B
build/block-library/blocks/list/style.css 94 B
build/block-library/blocks/media-text/editor-rtl.css 266 B
build/block-library/blocks/media-text/editor.css 263 B
build/block-library/blocks/media-text/style-rtl.css 493 B
build/block-library/blocks/media-text/style.css 490 B
build/block-library/blocks/more/editor-rtl.css 431 B
build/block-library/blocks/more/editor.css 431 B
build/block-library/blocks/navigation-link/editor-rtl.css 649 B
build/block-library/blocks/navigation-link/editor.css 650 B
build/block-library/blocks/navigation-link/style-rtl.css 94 B
build/block-library/blocks/navigation-link/style.css 94 B
build/block-library/blocks/navigation-submenu/editor-rtl.css 299 B
build/block-library/blocks/navigation-submenu/editor.css 299 B
build/block-library/blocks/navigation-submenu/view.min.js 343 B
build/block-library/blocks/navigation/editor-rtl.css 1.93 kB
build/block-library/blocks/navigation/editor.css 1.94 kB
build/block-library/blocks/navigation/style-rtl.css 1.82 kB
build/block-library/blocks/navigation/style.css 1.81 kB
build/block-library/blocks/navigation/view.min.js 2.82 kB
build/block-library/blocks/nextpage/editor-rtl.css 395 B
build/block-library/blocks/nextpage/editor.css 395 B
build/block-library/blocks/page-list/editor-rtl.css 401 B
build/block-library/blocks/page-list/editor.css 402 B
build/block-library/blocks/page-list/style-rtl.css 175 B
build/block-library/blocks/page-list/style.css 175 B
build/block-library/blocks/paragraph/editor-rtl.css 157 B
build/block-library/blocks/paragraph/editor.css 157 B
build/block-library/blocks/paragraph/style-rtl.css 273 B
build/block-library/blocks/paragraph/style.css 273 B
build/block-library/blocks/post-author/style-rtl.css 175 B
build/block-library/blocks/post-author/style.css 176 B
build/block-library/blocks/post-comments-form/style-rtl.css 446 B
build/block-library/blocks/post-comments-form/style.css 446 B
build/block-library/blocks/post-comments/style-rtl.css 521 B
build/block-library/blocks/post-comments/style.css 521 B
build/block-library/blocks/post-excerpt/editor-rtl.css 73 B
build/block-library/blocks/post-excerpt/editor.css 73 B
build/block-library/blocks/post-excerpt/style-rtl.css 69 B
build/block-library/blocks/post-excerpt/style.css 69 B
build/block-library/blocks/post-featured-image/editor-rtl.css 721 B
build/block-library/blocks/post-featured-image/editor.css 721 B
build/block-library/blocks/post-featured-image/style-rtl.css 153 B
build/block-library/blocks/post-featured-image/style.css 153 B
build/block-library/blocks/post-template/editor-rtl.css 99 B
build/block-library/blocks/post-template/editor.css 98 B
build/block-library/blocks/post-template/style-rtl.css 305 B
build/block-library/blocks/post-template/style.css 305 B
build/block-library/blocks/post-terms/style-rtl.css 73 B
build/block-library/blocks/post-terms/style.css 73 B
build/block-library/blocks/post-title/style-rtl.css 80 B
build/block-library/blocks/post-title/style.css 80 B
build/block-library/blocks/preformatted/style-rtl.css 103 B
build/block-library/blocks/preformatted/style.css 103 B
build/block-library/blocks/pullquote/editor-rtl.css 198 B
build/block-library/blocks/pullquote/editor.css 198 B
build/block-library/blocks/pullquote/style-rtl.css 389 B
build/block-library/blocks/pullquote/style.css 388 B
build/block-library/blocks/pullquote/theme-rtl.css 167 B
build/block-library/blocks/pullquote/theme.css 167 B
build/block-library/blocks/query-pagination-numbers/editor-rtl.css 122 B
build/block-library/blocks/query-pagination-numbers/editor.css 121 B
build/block-library/blocks/query-pagination/editor-rtl.css 221 B
build/block-library/blocks/query-pagination/editor.css 211 B
build/block-library/blocks/query-pagination/style-rtl.css 234 B
build/block-library/blocks/query-pagination/style.css 231 B
build/block-library/blocks/query/editor-rtl.css 131 B
build/block-library/blocks/query/editor.css 132 B
build/block-library/blocks/quote/style-rtl.css 187 B
build/block-library/blocks/quote/style.css 187 B
build/block-library/blocks/quote/theme-rtl.css 223 B
build/block-library/blocks/quote/theme.css 226 B
build/block-library/blocks/rss/editor-rtl.css 202 B
build/block-library/blocks/rss/editor.css 204 B
build/block-library/blocks/rss/style-rtl.css 289 B
build/block-library/blocks/rss/style.css 288 B
build/block-library/blocks/search/editor-rtl.css 165 B
build/block-library/blocks/search/editor.css 165 B
build/block-library/blocks/search/style-rtl.css 397 B
build/block-library/blocks/search/style.css 398 B
build/block-library/blocks/search/theme-rtl.css 64 B
build/block-library/blocks/search/theme.css 64 B
build/block-library/blocks/separator/editor-rtl.css 99 B
build/block-library/blocks/separator/editor.css 99 B
build/block-library/blocks/separator/style-rtl.css 245 B
build/block-library/blocks/separator/style.css 245 B
build/block-library/blocks/separator/theme-rtl.css 172 B
build/block-library/blocks/separator/theme.css 172 B
build/block-library/blocks/shortcode/editor-rtl.css 474 B
build/block-library/blocks/shortcode/editor.css 474 B
build/block-library/blocks/site-logo/editor-rtl.css 744 B
build/block-library/blocks/site-logo/editor.css 744 B
build/block-library/blocks/site-logo/style-rtl.css 181 B
build/block-library/blocks/site-logo/style.css 181 B
build/block-library/blocks/site-tagline/editor-rtl.css 86 B
build/block-library/blocks/site-tagline/editor.css 86 B
build/block-library/blocks/site-title/editor-rtl.css 84 B
build/block-library/blocks/site-title/editor.css 84 B
build/block-library/blocks/social-link/editor-rtl.css 177 B
build/block-library/blocks/social-link/editor.css 177 B
build/block-library/blocks/social-links/editor-rtl.css 670 B
build/block-library/blocks/social-links/editor.css 669 B
build/block-library/blocks/social-links/style-rtl.css 1.32 kB
build/block-library/blocks/social-links/style.css 1.32 kB
build/block-library/blocks/spacer/editor-rtl.css 332 B
build/block-library/blocks/spacer/editor.css 332 B
build/block-library/blocks/spacer/style-rtl.css 48 B
build/block-library/blocks/spacer/style.css 48 B
build/block-library/blocks/table/editor-rtl.css 471 B
build/block-library/blocks/table/editor.css 472 B
build/block-library/blocks/table/style-rtl.css 481 B
build/block-library/blocks/table/style.css 481 B
build/block-library/blocks/table/theme-rtl.css 188 B
build/block-library/blocks/table/theme.css 188 B
build/block-library/blocks/tag-cloud/style-rtl.css 214 B
build/block-library/blocks/tag-cloud/style.css 215 B
build/block-library/blocks/template-part/editor-rtl.css 560 B
build/block-library/blocks/template-part/editor.css 559 B
build/block-library/blocks/template-part/theme-rtl.css 101 B
build/block-library/blocks/template-part/theme.css 101 B
build/block-library/blocks/text-columns/editor-rtl.css 95 B
build/block-library/blocks/text-columns/editor.css 95 B
build/block-library/blocks/text-columns/style-rtl.css 166 B
build/block-library/blocks/text-columns/style.css 166 B
build/block-library/blocks/verse/style-rtl.css 87 B
build/block-library/blocks/verse/style.css 87 B
build/block-library/blocks/video/editor-rtl.css 571 B
build/block-library/blocks/video/editor.css 572 B
build/block-library/blocks/video/style-rtl.css 173 B
build/block-library/blocks/video/style.css 173 B
build/block-library/blocks/video/theme-rtl.css 124 B
build/block-library/blocks/video/theme.css 124 B
build/block-library/common-rtl.css 908 B
build/block-library/common.css 905 B
build/block-library/editor-rtl.css 10 kB
build/block-library/editor.css 10 kB
build/block-library/index.min.js 164 kB
build/block-library/reset-rtl.css 474 B
build/block-library/reset.css 474 B
build/block-library/style-rtl.css 10.8 kB
build/block-library/style.css 10.8 kB
build/block-library/theme-rtl.css 672 B
build/block-library/theme.css 676 B
build/block-serialization-default-parser/index.min.js 1.09 kB
build/block-serialization-spec-parser/index.min.js 2.79 kB
build/blocks/index.min.js 46.3 kB
build/components/index.min.js 214 kB
build/components/style-rtl.css 15.5 kB
build/components/style.css 15.5 kB
build/compose/index.min.js 11.2 kB
build/core-data/index.min.js 13.3 kB
build/customize-widgets/index.min.js 11.4 kB
build/customize-widgets/style-rtl.css 1.5 kB
build/customize-widgets/style.css 1.49 kB
build/data-controls/index.min.js 631 B
build/data/index.min.js 7.49 kB
build/date/index.min.js 31.9 kB
build/deprecated/index.min.js 485 B
build/dom-ready/index.min.js 304 B
build/dom/index.min.js 4.5 kB
build/edit-navigation/index.min.js 16 kB
build/edit-navigation/style-rtl.css 3.76 kB
build/edit-navigation/style.css 3.76 kB
build/edit-post/classic-rtl.css 546 B
build/edit-post/classic.css 547 B
build/edit-post/index.min.js 29.6 kB
build/edit-post/style-rtl.css 7.16 kB
build/edit-post/style.css 7.16 kB
build/edit-site/index.min.js 37.2 kB
build/edit-site/style-rtl.css 6.83 kB
build/edit-site/style.css 6.82 kB
build/edit-widgets/index.min.js 16.5 kB
build/edit-widgets/style-rtl.css 4.17 kB
build/edit-widgets/style.css 4.17 kB
build/editor/index.min.js 38.3 kB
build/editor/style-rtl.css 3.71 kB
build/editor/style.css 3.7 kB
build/element/index.min.js 3.29 kB
build/escape-html/index.min.js 517 B
build/format-library/index.min.js 6.58 kB
build/format-library/style-rtl.css 571 B
build/format-library/style.css 571 B
build/hooks/index.min.js 1.63 kB
build/html-entities/index.min.js 424 B
build/i18n/index.min.js 3.71 kB
build/is-shallow-equal/index.min.js 501 B
build/keyboard-shortcuts/index.min.js 1.8 kB
build/keycodes/index.min.js 1.39 kB
build/list-reusable-blocks/index.min.js 1.72 kB
build/list-reusable-blocks/style-rtl.css 838 B
build/list-reusable-blocks/style.css 838 B
build/media-utils/index.min.js 2.92 kB
build/notices/index.min.js 925 B
build/nux/index.min.js 2.08 kB
build/nux/style-rtl.css 747 B
build/nux/style.css 743 B
build/plugins/index.min.js 1.84 kB
build/primitives/index.min.js 924 B
build/priority-queue/index.min.js 582 B
build/react-i18n/index.min.js 671 B
build/react-refresh-entry/index.min.js 8.44 kB
build/react-refresh-runtime/index.min.js 7.31 kB
build/redux-routine/index.min.js 2.65 kB
build/reusable-blocks/index.min.js 2.22 kB
build/reusable-blocks/style-rtl.css 256 B
build/reusable-blocks/style.css 256 B
build/rich-text/index.min.js 11 kB
build/server-side-render/index.min.js 1.58 kB
build/shortcode/index.min.js 1.49 kB
build/token-list/index.min.js 639 B
build/url/index.min.js 1.9 kB
build/viewport/index.min.js 1.05 kB
build/warning/index.min.js 248 B
build/widgets/index.min.js 7.15 kB
build/widgets/style-rtl.css 1.16 kB
build/widgets/style.css 1.16 kB
build/wordcount/index.min.js 1.04 kB

compressed-size-action

@dcalhoun dcalhoun requested a review from fluiddot January 12, 2022 17:31
@dcalhoun dcalhoun mentioned this pull request Jan 12, 2022
7 tasks
@gziolo
Copy link
Member

gziolo commented Jan 13, 2022

The override for react-platform.js file makes a lot of sense because there is no react-dom in React Native 👍🏻

Let's wait for approval from someone that is better suited to confirm it's the right approach. CI is finally green so I like that PR a lot 😄

Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎊 !

I've just left a couple of comments but none of them should block the PR.

Reinstate react-platform.native.js file, which was removed in b7b62d2, as it was no longer strictly necessary. However, the removal led to Jest loading the DOM-specific version of the file when mocking Gutenberg modules that depended upon react-platform.js. Jest would seemingly load react-dom when attempting to auto-mock the module, e.g. reusable-block-types-tab.native.js > use-select/index.js > @wordpress/element > react-platform.js. Reinstating react-platform.native.js addresses this issue by ensuring that Jest does not encounter an import of react-dom.

Great catch, I didn't realize about this issue when I did the initialization refactor 🤦 , thanks for spotting it 🙇 . I also noticed that due to this, we included react-dom in the JS bundle (checked in the latest source map), but I guess is innocuous apart from increasing the bundle's size a bit (the size of react-dom.production.min.js is 100kb).

} );

const screen = initGutenberg();
const blockList = await waitFor( () =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry for that, I fell into the same trap when adding other integration tests in the past 🤦 . Thanks for catching it 🙇 !

I did an identical test in GB-mobile (reference), so I'll apply the same fix there 🔧 .

Comment on lines +181 to +186
jest.mock( '../setup', () => {
return {
__esModule: true,
default: jest.fn( () => <MockEditor /> ),
};
} );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's interesting, my idea for this test case was actually to verify that the entire editor is initialized including rendering the initial HTML. In any case, and in order to prevent blocking the PR and Jest upgrade, we could stick with this change and tackle my original idea in a future PR 👍 .

@gziolo gziolo merged commit bdfdeeb into update/jest-27 Jan 13, 2022
@gziolo gziolo deleted the chore/fix-erroneous-react-dom-usage-in-native-tests branch January 13, 2022 12:06
gziolo pushed a commit that referenced this pull request Jan 14, 2022
* Reinstate react-platform.native.js file

This file was removed in the following commit, as it was no longer
strictly necessary. b7b62d2#diff-f02069349f238fb47a268bb7fcc03c9768331db18ead0e28d8ecad7bbc05037c

However, the removal led to Jest loading the DOM-specific version of the
file when mocking Gutenberg modules that depended upon
`react-platform.js`. Jest would seemingly load `react-dom` when
attempting to auto-mock a module, e.g. `jest.mock( '@wordpress/data/src/components/use-select' );`.

The loading of `react-dom` resulted in the following error:

```
TypeError: Cannot use 'in' operator to search for 'WebkitAnimation' in undefined

  at getVendorPrefixedEventName (node_modules/react-dom/cjs/react-dom.development.js:5011:58)
  at node_modules/react-dom/cjs/react-dom.development.js:5019:21
  at Object.<anonymous> (node_modules/react-dom/cjs/react-dom.development.js:26261:5)
  at Object.<anonymous> (node_modules/react-dom/index.js:37:20)
```

Reinstating `react-platform.native.js` addresses this issue by ensuring
that Jest does not encounter an import of `react-dom`.

* Fix false-positive ReactNativeEditor test

This test failed when run in isolation. It would appear it was dependent
upon the `jest.mock('../setup')` found in sibling tests.

After defining a mock for this specific test, it was discovered that the
assertion did not await the asynchronous query found within the test.
The assertion resulted in a false-positive as the returned query
`Promise` technically matches `toBeDefined`. `async`/`await` was added
to ensure the test assertion awaits the query result, as well as fixes
the following related log warning.

```
A worker process has failed to exit gracefully and has been force exited. This is likely caused by tests leaking due to improper teardown. Try running with --detectOpenHandles to find leaks. Active timers can also cause this, ensure that .unref() was called on them.
```
gziolo pushed a commit that referenced this pull request Jan 18, 2022
* Reinstate react-platform.native.js file

This file was removed in the following commit, as it was no longer
strictly necessary. b7b62d2#diff-f02069349f238fb47a268bb7fcc03c9768331db18ead0e28d8ecad7bbc05037c

However, the removal led to Jest loading the DOM-specific version of the
file when mocking Gutenberg modules that depended upon
`react-platform.js`. Jest would seemingly load `react-dom` when
attempting to auto-mock a module, e.g. `jest.mock( '@wordpress/data/src/components/use-select' );`.

The loading of `react-dom` resulted in the following error:

```
TypeError: Cannot use 'in' operator to search for 'WebkitAnimation' in undefined

  at getVendorPrefixedEventName (node_modules/react-dom/cjs/react-dom.development.js:5011:58)
  at node_modules/react-dom/cjs/react-dom.development.js:5019:21
  at Object.<anonymous> (node_modules/react-dom/cjs/react-dom.development.js:26261:5)
  at Object.<anonymous> (node_modules/react-dom/index.js:37:20)
```

Reinstating `react-platform.native.js` addresses this issue by ensuring
that Jest does not encounter an import of `react-dom`.

* Fix false-positive ReactNativeEditor test

This test failed when run in isolation. It would appear it was dependent
upon the `jest.mock('../setup')` found in sibling tests.

After defining a mock for this specific test, it was discovered that the
assertion did not await the asynchronous query found within the test.
The assertion resulted in a false-positive as the returned query
`Promise` technically matches `toBeDefined`. `async`/`await` was added
to ensure the test assertion awaits the query result, as well as fixes
the following related log warning.

```
A worker process has failed to exit gracefully and has been force exited. This is likely caused by tests leaking due to improper teardown. Try running with --detectOpenHandles to find leaks. Active timers can also cause this, ensure that .unref() was called on them.
```
gziolo added a commit that referenced this pull request Jan 20, 2022
* Testing: Upgrade Jest to v27

* Fix native test timeouts caused by combining fake timers and setImmediate (#37715)

* Upgrade to @testing-library/[email protected]

* Clean up fake timer usage after tests

This may be unnecessary, but avoid potential issues where fake timers
are unexpectedly used and cause breakages in other tests.

* Enable combination of modern fake timers and waitFor

Previously, `waitFor` would timeout when `jest.useFakeTimers('modern')`
was enabled. The 'modern' version is now the default in Jest 27.

The Jest preset from `@testing-library/react-native` provides a
workaround for a larger issue in React Native and Jest that mutates the
global `Promise` object. https://git.io/JSDZI

* Remove global enabling of fake timers

Enabling fake timers can have negative consequences with `waitFor`, e.g.
causing unexpected timeouts. Enabling it globally is far-reaching and
this should likely be enabled within individual tests as needed.

* Replace jest-jasmine2 with jest-circus

The latter is considered the successor to the former. We seemingly do
not depend on anything explicitly provided by `jest-jasmine2` and should
likely move on from it.

* Switch testing environment from jsdom to node

Improves speed of test environment setup and fixes a timeout issue when
combining `waitFor` and `jest.useFakeTimers('modern')`. It is not yet
exactly pinpointed as to _why_ this fixes the timeout issue. It appears
to related to `setImmediate` and `setTimeout`.

* Remove setImmediate global for testing environment

This may be unnecessary if `testEnvironment: 'node'` is retained.
However, most tests are currently broken due to missing DOM APIs.

* Polyfill required DOM APIs for testing environment

Now that `testEnvironment: 'node'` is utilized for the testing
environment, we must mirror the app runtime and polyfill the necessary
DOM APIs used in the source.

The Enzyme configuration removed conflicts with the switch from
`testEnvironment: 'jsdom'` to `testEnvironment: 'node'`. Enzyme depends
upon `react-dom`, which introduces far more dependencies upon DOM APIs.
Currently, all Enzyme-related tests fail and need to be replaced with
`@testing-library/react-native`.

* Avoid import of react-dom within native file

Importing `react-dom` introduces additional dependencies upon the DOM
API and is incompatible with `testEnvironment: 'node'`. The `act`
utility is available from `@testing-library/react-native`.

* Explicitly toggle fake timers in tests

This may not be necessary, but may help avoid unexpected issues from
lingering fake timers, e.g. timeout errors while using `waitFor`.

* Reinstate legacy Jest timers

The Jest preset from `@testing-library/react-native` that fixed support
for "modern" timers by modifying the polyfilled the global `Promise`
resulted in new failures from within React Native itself. Specifically,
core React Native components rely upon `.done` from the `promise`
package. `.done` is a non-standard method that does not exist on the
global `Promise` used by `@testing-library/react-native`'s preset.

* Disable erroneously failing test

This previously passing test now fails after upgrading
`@testing-library/react-native` due to changes in the library. Setting
`pointerEvent` to "box-none" or "none" currently erroneously prevents
triggering other events unrelated to pressing on the element, e.g.
`onTouch*`, `onLayout`.

https://git.io/JSHZt

* Reinstate jest-jasmine2 to support done callback

The Jest team create jest-circus as the predecessor for jest-jasmine2.
The former does not support the `done` callback with async/await, and
would appear to have no plans to do so.

It would likely benefit us to refactor the one current test using
`done` away from it, and embrace `jest-circus` to maintain alignment
with Jest core.

* Refactor native unit tests away from done callback

The Jest team created `jest-circus` as the predecessor for
`jest-jasmine2`. The former does not support the `done` callback with
async/await, and would appear to have no plans to do so.

In order to embrace `jest-circus` and maintain alignment with Jest core,
the one test using `done` was refactored to avoid it

- https://git.io/JSHWU
- https://git.io/JSHWk

* Remove Enzyme from Editor tests

* Remove Enzyme from Paragraph tests

* Remove Enzyme from BlockMover tests

* Remove Enzyme from BlockEdit test

* Remove Enzyme from LinksUI test

* Remove Enzyme from ListEdit tests

* Remove Enzyme from Platform tests

* Remove Enzyme from BlockTypesTab tests

* Remove Enzyme from HTMLTextInput tests

* Remove Enzyme from ReusableBlockTab tests

* Remove Enzyme from MediaUpload tests

* Remove Enzyme from BlockMediaUpdateProgress tests

* Remove Enzyme from MediaUploadProgress tests

* Fix ReferencEerror in Inserter test

Usage of `react-test-renderer` caused the following error. Leveraging
`@testing-library/react-native` instead resolved it for an unknown
reason.

```
ReferenceError: You are trying to `import` a file after the Jest environment has been torn down. From packages/block-editor/src/components/inserter/test/index.native.js.

      468 |                     style: listStyle,
      469 |                     safeAreaBottomInset,
    > 470 |                     scrollEnabled,
          |                                             ^
      471 |                     automaticallyAdjustContentInsets: false,
      472 |             };
      473 |

      at Object.get PanResponder [as PanResponder] (node_modules/react-native/index.js:251:12)
      at BottomSheet.render (packages/components/src/mobile/bottom-sheet/index.native.js:470:39)
      at finishClassComponent (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:8459:31)
      at updateClassComponent (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:8409:24)
      at beginWork (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:9990:16)
```

* Fix ReferencEerror in Verse test

Usage of `react-test-renderer` caused the following error. Leveraging
`@testing-library/react-native` instead resolved it for an unknown
reason.

```
ReferenceError: You are trying to `import` a file after the Jest environment has been torn down. From packages/block-editor/src/components/inserter/test/index.native.js.

      468 |                     style: listStyle,
      469 |                     safeAreaBottomInset,
    > 470 |                     scrollEnabled,
          |                                             ^
      471 |                     automaticallyAdjustContentInsets: false,
      472 |             };
      473 |

      at Object.get PanResponder [as PanResponder] (node_modules/react-native/index.js:251:12)
      at BottomSheet.render (packages/components/src/mobile/bottom-sheet/index.native.js:470:39)
      at finishClassComponent (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:8459:31)
      at updateClassComponent (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:8409:24)
      at beginWork (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:9990:16)
```

* Fix ReferencEerror in Audio test

Usage of `react-test-renderer` caused the following error. Leveraging
`@testing-library/react-native` instead resolved it for an unknown
reason.

```
ReferenceError: You are trying to `import` a file after the Jest environment has been torn down. From packages/block-editor/src/components/inserter/test/index.native.js.

      468 |                     style: listStyle,
      469 |                     safeAreaBottomInset,
    > 470 |                     scrollEnabled,
          |                                             ^
      471 |                     automaticallyAdjustContentInsets: false,
      472 |             };
      473 |

      at Object.get PanResponder [as PanResponder] (node_modules/react-native/index.js:251:12)
      at BottomSheet.render (packages/components/src/mobile/bottom-sheet/index.native.js:470:39)
      at finishClassComponent (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:8459:31)
      at updateClassComponent (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:8409:24)
      at beginWork (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:9990:16)
```

* Fix ReferencEerror in File test

Usage of `react-test-renderer` caused the following error. Leveraging
`@testing-library/react-native` instead resolved it for an unknown
reason.

```
ReferenceError: You are trying to `import` a file after the Jest environment has been torn down. From packages/block-editor/src/components/inserter/test/index.native.js.

      468 |                     style: listStyle,
      469 |                     safeAreaBottomInset,
    > 470 |                     scrollEnabled,
          |                                             ^
      471 |                     automaticallyAdjustContentInsets: false,
      472 |             };
      473 |

      at Object.get PanResponder [as PanResponder] (node_modules/react-native/index.js:251:12)
      at BottomSheet.render (packages/components/src/mobile/bottom-sheet/index.native.js:470:39)
      at finishClassComponent (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:8459:31)
      at updateClassComponent (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:8409:24)
      at beginWork (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:9990:16)
```

* Fix ReferencEerror in Search test

Usage of `react-test-renderer` caused the following error. Leveraging
`@testing-library/react-native` instead resolved it for an unknown
reason.

```
ReferenceError: You are trying to `import` a file after the Jest environment has been torn down. From packages/block-editor/src/components/inserter/test/index.native.js.

      468 |                     style: listStyle,
      469 |                     safeAreaBottomInset,
    > 470 |                     scrollEnabled,
          |                                             ^
      471 |                     automaticallyAdjustContentInsets: false,
      472 |             };
      473 |

      at Object.get PanResponder [as PanResponder] (node_modules/react-native/index.js:251:12)
      at BottomSheet.render (packages/components/src/mobile/bottom-sheet/index.native.js:470:39)
      at finishClassComponent (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:8459:31)
      at updateClassComponent (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:8409:24)
      at beginWork (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:9990:16)
```

* Fix ReferencEerror in Missing test

Usage of `react-test-renderer` caused the following error. Leveraging
`@testing-library/react-native` instead resolved it for an unknown
reason.

```
ReferenceError: You are trying to `import` a file after the Jest environment has been torn down. From packages/block-editor/src/components/inserter/test/index.native.js.

      468 |                     style: listStyle,
      469 |                     safeAreaBottomInset,
    > 470 |                     scrollEnabled,
          |                                             ^
      471 |                     automaticallyAdjustContentInsets: false,
      472 |             };
      473 |

      at Object.get PanResponder [as PanResponder] (node_modules/react-native/index.js:251:12)
      at BottomSheet.render (packages/components/src/mobile/bottom-sheet/index.native.js:470:39)
      at finishClassComponent (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:8459:31)
      at updateClassComponent (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:8409:24)
      at beginWork (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:9990:16)
```

* Upgrade to @testing-library/[email protected]

* Add assertions to block type tab tests

Improve test clarity with explicit expect assertions.

Co-authored-by: Carlos Garcia <[email protected]>

* Remove unnecessary abstraction

The switch away from Enzyme reduced this abstraction to a single line,
so it provides less value now.

* Consistently assert media block update progress spinner removal

This increases consistency amongst the tests, as most already include
this assertion.

* Update MediaUpload test to select 'Choose from device' option

This selection better aligns with the test description.

* Remove duplicative matchMedia global definition

The test environment now imports the globals setup file used by the app
runtime. That file includes a `matchMedia` global definition as well.

* Add assertions to LinkSettings tests

Improve test clarity with explicit expect assertions.

* Removed shallow renderer usage in tests

Shallow rendering components is generally considered a non-optimal
approach to testing React components by the React community. This
replaces `shallow` with `render` to further test integration of the
subject components.

* Remove unused import

The `React` import was utilized by the now removed `shallow` render
implementation.

* Remove unnecessary top-level beforeAll usage

Jest runs each test file independently, so top-level code will not
impact other test files. Since the `(before|after)All` usage in code
changed is all top-level, and not within a `describe`, it is
superfluous.

Co-authored-by: Carlos Garcia <[email protected]>

* Fix erroneous react-dom usage in native tests (#37921)

* Reinstate react-platform.native.js file

This file was removed in the following commit, as it was no longer
strictly necessary. b7b62d2#diff-f02069349f238fb47a268bb7fcc03c9768331db18ead0e28d8ecad7bbc05037c

However, the removal led to Jest loading the DOM-specific version of the
file when mocking Gutenberg modules that depended upon
`react-platform.js`. Jest would seemingly load `react-dom` when
attempting to auto-mock a module, e.g. `jest.mock( '@wordpress/data/src/components/use-select' );`.

The loading of `react-dom` resulted in the following error:

```
TypeError: Cannot use 'in' operator to search for 'WebkitAnimation' in undefined

  at getVendorPrefixedEventName (node_modules/react-dom/cjs/react-dom.development.js:5011:58)
  at node_modules/react-dom/cjs/react-dom.development.js:5019:21
  at Object.<anonymous> (node_modules/react-dom/cjs/react-dom.development.js:26261:5)
  at Object.<anonymous> (node_modules/react-dom/index.js:37:20)
```

Reinstating `react-platform.native.js` addresses this issue by ensuring
that Jest does not encounter an import of `react-dom`.

* Fix false-positive ReactNativeEditor test

This test failed when run in isolation. It would appear it was dependent
upon the `jest.mock('../setup')` found in sibling tests.

After defining a mock for this specific test, it was discovered that the
assertion did not await the asynchronous query found within the test.
The assertion resulted in a false-positive as the returned query
`Promise` technically matches `toBeDefined`. `async`/`await` was added
to ensure the test assertion awaits the query result, as well as fixes
the following related log warning.

```
A worker process has failed to exit gracefully and has been force exited. This is likely caused by tests leaking due to improper teardown. Try running with --detectOpenHandles to find leaks. Active timers can also cause this, ensure that .unref() was called on them.
```

* [RNMobile] Fix `act` warnings that might be triggered after test finishes (#38052)

* Fix act warnings after test finishes

This is a workaround for addressing the act warnings that might caused by store updates executed after the test finishes.

* Update comment of setImmediate workaround

Co-authored-by: David Calhoun <[email protected]>

* Unmount editor when test finish

* Update initialize editor helper

The function is now synchronous as we don't have to wait for any element.

* Update tests that use initialize editor

* Fix act warnings from store resolvers with fake timers (#38077)

* Leverage fake timers to resolve store resolvers

During editor initialization, asynchronous store resolvers rely upon
`setTimeout` to run at the end of the current JavaScript block
execution. In order to prevent "act" warnings triggered by updates to
the React tree, we leverage fake timers to manually tick and await the
resolution of the current block execution before proceeding.

* Update RichText test `initializeEditor` usage

The refactor of `initializeEditor` to leverage fake timers means this
test no longer needs to `await` asynchronous work or manuall `unmount`
its subject component.

* Refactor usage of `initializeEditor` in tests

Now that `initializeEditor` leverages fake timers to resolve
asynchronous store resolvers, the tests no longer need to await a
resolution of `initializeEditor`.

* Fix `act` warnings in Image edit test

Retrieving text from the clipboard is an asynchronous action. The
component updated React state once the clipboard resolved. This resulted
in a state update triggering an `act` warning. Because there is no
visible change to the rendered output, e.g. updated text or UI, we must
await the resolution of the clipboard promise itself.

Co-authored-by: David Calhoun <[email protected]>

Co-authored-by: David Calhoun <[email protected]>
Co-authored-by: Carlos Garcia <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants