[RNMobile] Refactor react-native-editor initialization test
#37955
Conversation
fluiddot
added
the
Mobile App - i.e. Android or iOS
|
Size Change: +2 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
| // It's expected that some blocks are upgraded and inform about it (example: "Updated Block: core/cover") | ||
| expect( console ).toHaveInformed(); |
There was a problem hiding this comment.
This might be giving us a clue about using outdated versions of blocks in initialHtml file.
There was a problem hiding this comment.
By not passing arguments to toHaveInformed, I would imagine we expose ourselves to additional console.info calls accumulating without our knowledge. From reviewing the source of @wordpress/jest-console, I suppose there is no way to further constrain this assertion without passing the entirety of the explicit block update output, which is not manageable unfortunately.
This might be giving us a clue about using outdated versions of blocks in
initialHtmlfile.
With this are you implying that we should fix the initialHtml rather than suppressing the console.info message here?
There was a problem hiding this comment.
By not passing arguments to toHaveInformed, I would imagine we expose ourselves to additional console.info calls accumulating without our knowledge. From reviewing the source of @wordpress/jest-console, I suppose there is no way to further constrain this assertion without passing the entirety of the explicit block update output, which is not manageable unfortunately.
Exactly, there might be some cases where you could overlook extra or unexpected info logs using this function without passing arguments. In the case of this test, although we know the expected log output, I decided not to include it due to its size.
With this are you implying that we should fix the initialHtml rather than suppressing the console.info message here?
Yep, I haven't investigated this further, but the fact that some blocks are being upgraded makes me wonder whether they should be reviewed, in case the HTML we're using for them is old.
There was a problem hiding this comment.
In any case, I think we could address this in a different PR, I'll open a ticket for this as a follow-up.
| import * as wpHooks from '@wordpress/hooks'; | ||
| import '@wordpress/jest-console'; |
There was a problem hiding this comment.
I found this library quite useful for assuring we don't produce unexpected logs. In the future, it would be great to enable it globally 🤩 .
An important note about using it is that it silences potential logs that happen after the test. For example, these test cases are generating "act" warnings (i.e. An update to EditorProvider inside a test was not wrapped in act(...).) due to the fact that some store state updates are happening after the test is finished. In the beginning, I thought of this as an issue, but I think it's safe to omit them as they are happening after the test ends.
There was a problem hiding this comment.
I found this library quite useful for assuring we don't produce unexpected logs. In the future, it would be great to enable it globally 🤩 .
Agreed. I would love to enable this globally, we'll just need to address all the current warnings and errors in our test logs first.
An important note about using it is that it silences potential logs that happen after the test.
Yeah, this has confused me before while debugging failing web tests, which do enable @wordpress/jest-console globally. In order to intentionally log information, one has to explicitly assert the log should occur.
For example, these test cases are generating "act" warnings (i.e.
An update to EditorProvider inside a test was not wrapped in act(...).) due to the fact that some store state updates are happening after the test is finished. In the beginning, I thought of this as an issue, but I think it's safe to omit them as they are happening after the test ends.
While these particular updates may not dangerous for this specific context, I think it may be dangerous to set the precedent that it is safe to ignore the warnings. It is very possible for a post-assertion update to nullify the validity of a successful assertion. E.g. if one asserts an element is present, a later state update may unexpectedly and erroneously removes the element, which would mean the test is a false positive. The logs occurring "after the test ends" I believe is merely a result of them occurring after the final assertion, which doesn't inherently make them safe.
Is there a way for us to address the act warnings in these tests?
There was a problem hiding this comment.
Agreed. I would love to enable this globally, we'll just need to address all the current warnings and errors in our test logs first.
In fact, I'm planning to open a ticket specific to addressing this topic.
While these particular updates may not dangerous for this specific context, I think it may be dangerous to set the precedent that it is safe to ignore the warnings. It is very possible for a post-assertion update to nullify the validity of a successful assertion. E.g. if one asserts an element is present, a later state update may unexpectedly and erroneously remove the element, which would mean the test is a false positive. The logs occurring "after the test ends" I believe is merely a result of them occurring after the final assertion, which doesn't inherently make them safe.
Good point, I understand and share your concern about the potential side effects of ignoring the warnings. However, using the @wordpress/jest-console package would make it very easy to ignore them, so it would be great to discuss whether including (although its benefits) it would be safe in relation to this issue. I guess the main problem here is how to know when the editor and its components finish triggering all the state updates, although I'd like to note that it's also important that we identify potential errors upon unmounting the editor or components, in order to prevent memory leaks caused by unclosed listeners.
Being this said, I'm thinking of creating a native version of the supported matchers and excluding the error logs. This way we would assure the "act" errors (note that they're warnings but logged via console.error) are logged hence, we could easily identify them in the output.
Is there a way for us to address the act warnings in these tests?
Actually, I managed to find a workaround by delaying the query passed to the waitFor function an execution block cycle (check the rnmobile/add/use-immediate-wait-for branch). I haven't investigated the original issue, but I have the gut feeling that it might be related to how the store actions and updates are executed.
There was a problem hiding this comment.
However, using the
@wordpress/jest-consolepackage would make it very easy to ignore them, so it would be great to discuss whether including (although its benefits) it would be safe in relation to this issue.
The absence of @wordpress/jest-console makes it far easier for warnings and errors to go unnoticed, as they will not necessarily cause test failures. I agree @wordpress/jest-console makes it easier to explicitly hide warnings and errors. I would posit the former is worst. The latter is at least an intentional act.
I guess the main problem here is how to know when the editor and its components finish triggering all the state updates, although I'd like to note that it's also important that we identify potential errors upon unmounting the editor or components, in order to prevent memory leaks caused by unclosed listeners.
Agreed. We appear to be aligned that it is not safe or good practice to ignore the warnings or assume they are safe.
Being this said, I'm thinking of creating a native version of the supported matchers and excluding the error logs. This way we would assure the "act" errors (note that they're warnings but logged via
console.error) are logged hence, we could easily identify them in the output.
Maybe I am mistaken, but I interpret the intent of @wordpress/jest-console is to fail tests when console logs, warnings, or errors occur. act errors would trigger test failures when @wordpress/jest-console is enabled. So, @wordpress/jest-console itself should already accomplish your proposed end goal: ensure act errors are not overlooked. I.e. someone is far more likely to notice — and hopefully fix — a failing test (due to an act error) than a passing test that happens to also log an act error.
Example Failure from act + @wordpress/jest-console
FAIL ../block-library/src/cover/test/edit.native.js (7.967 s)
when no media is attached
○ skipped adds an image or video
when an image is attached
✕ discards canceled focal point changes (343 ms)
○ skipped edits the image
○ skipped replaces the image
○ skipped clears the image within image edit button
○ skipped toggles a fixed background
○ skipped edits the focal point with a slider
○ skipped edits the focal point with a text input
○ skipped clears the media within cell button
color settings
○ skipped sets a color for the overlay background when the placeholder is visible
○ skipped sets a gradient overlay background when a solid background was already selected
○ skipped toggles between solid colors and gradients
○ skipped clears the selected overlay color and mantains the inner blocks
● when an image is attached › discards canceled focal point changes
expect(jest.fn()).not.toHaveErrored(expected)
Expected mock function not to be called but it was called with:
["Warning: An update to %s inside a test was not wrapped in act(...).·
When testing, code that causes React state updates should be wrapped into act(...):·
act(() => {
/* fire events that update state */
});
/* assert on the output */·
This ensures that you're testing the behavior the user would see in the browser. Learn more at https://reactjs.org/link/wrap-tests-with-act%s", "ForwardRef(NavigationContainer)", "
at NavigationContainer (/Users/[REDACTED]/Sites/a8c/gutenberg/node_modules/@react-navigation/native/lib/commonjs/NavigationContainer.tsx:40:5)
at View
at View (/Users/[REDACTED]/Sites/a8c/gutenberg/node_modules/react-native/jest/mockComponent.js:28:18)
at BottomSheetNavigationContainer (/Users/[REDACTED]/Sites/a8c/gutenberg/packages/components/src/mobile/bottom-sheet/bottom-sheet-navigation/navigation-container.native.js:63:2)
at View
at View (/Users/[REDACTED]/Sites/a8c/gutenberg/node_modules/react-native/jest/mockComponent.js:28:18)
at View
at View (/Users/[REDACTED]/Sites/a8c/gutenberg/node_modules/react-native/jest/mockComponent.js:28:18)
at KeyboardAvoidingView (/Users/[REDACTED]/Sites/a8c/gutenberg/packages/components/src/mobile/bottom-sheet/keyboard-avoiding-view.native.js:26:16)
at Modal
at /Users/[REDACTED]/Sites/a8c/gutenberg/test/native/setup.js:88:8
at BottomSheet (/Users/[REDACTED]/Sites/a8c/gutenberg/packages/components/src/mobile/bottom-sheet/index.native.js:51:16)
at BottomSheetSettings (/Users/[REDACTED]/Sites/a8c/gutenberg/packages/block-editor/src/components/block-settings/container.native.js:29:2)
at WithDispatch(BottomSheetSettings) (/Users/[REDACTED]/Sites/a8c/gutenberg/packages/data/src/components/with-dispatch/index.js:95:26)
at /Users/[REDACTED]/Sites/a8c/gutenberg/packages/data/src/components/with-select/index.js:56:24
at WithSelect(WithDispatch(BottomSheetSettings)) (/Users/[REDACTED]/Sites/a8c/gutenberg/packages/compose/src/higher-order/pure/index.tsx:35:3)
at SlotFillProvider (/Users/[REDACTED]/Sites/a8c/gutenberg/packages/components/src/slot-fill/provider.js:18:16)
at CoverEdit"],["Warning: An update to %s inside a test was not wrapped in act(...).·
When testing, code that causes React state updates should be wrapped into act(...):·
act(() => {
/* fire events that update state */
});
/* assert on the output */·
This ensures that you're testing the behavior the user would see in the browser. Learn more at https://reactjs.org/link/wrap-tests-with-act%s", "Cover", "
at Cover (/Users/[REDACTED]/Sites/a8c/gutenberg/packages/block-library/src/cover/edit.native.js:83:2)
at WithDispatch(Component) (/Users/[REDACTED]/Sites/a8c/gutenberg/packages/data/src/components/with-dispatch/index.js:95:26)
at /Users/[REDACTED]/Sites/a8c/gutenberg/packages/data/src/components/with-select/index.js:56:24
at WithSelect(WithDispatch(Component)) (/Users/[REDACTED]/Sites/a8c/gutenberg/packages/compose/src/higher-order/pure/index.tsx:35:3)
at Edit (/Users/[REDACTED]/Sites/a8c/gutenberg/packages/block-editor/src/components/block-edit/edit.native.js:29:10)
at WithToolbarControls(Edit) (/Users/[REDACTED]/Sites/a8c/gutenberg/packages/block-editor/src/hooks/align.js:119:11)
at WithInspectorControl(WithToolbarControls(Edit)) (/Users/[REDACTED]/Sites/a8c/gutenberg/packages/block-editor/src/hooks/anchor.js:68:45)
at WithToolbarControls(WithInspectorControl(WithToolbarControls(Edit))) (/Users/[REDACTED]/Sites/a8c/gutenberg/packages/block-editor/src/hooks/style.js:262:33)
at WithFilters(Edit) (/Users/[REDACTED]/Sites/a8c/gutenberg/packages/components/src/higher-order/with-filters/index.js:49:18)
at BlockEdit (/Users/[REDACTED]/Sites/a8c/gutenberg/packages/block-editor/src/components/block-edit/index.js:23:10)
at SlotFillProvider (/Users/[REDACTED]/Sites/a8c/gutenberg/packages/components/src/slot-fill/provider.js:18:16)
at CoverEdit"]
34 | function assertExpectedCalls() {
35 | if ( spy.assertionsNumber === 0 && spy.mock.calls.length > 0 ) {
> 36 | expect( console ).not[ matcherName ]();
| ^
37 | }
38 | }
39 |
at Object.assertExpectedCalls (packages/jest-console/src/index.js:36:4)
Test Suites: 1 failed, 1 total
Tests: 1 failed, 12 skipped, 13 total
Snapshots: 0 total
Time: 9.12 s
Ran all test suites matching /packages\/block-library\/src\/cover\/test\/edit.native.js/i.
Actually, I managed to find a workaround by delaying the query passed to the
waitForfunction an execution block cycle (check thernmobile/add/use-immediate-wait-forbranch). I haven't investigated the original issue, but I have the gut feeling that it might be related to how the store actions and updates are executed.
Interesting. I have limited knowledge of the error, so I am uncertain to the appropriateness of the possible fix. 🤔
There was a problem hiding this comment.
Maybe I am mistaken, but I interpret the intent of
@wordpress/jest-consoleis to fail tests when console logs, warnings, or errors occur.acterrors would trigger test failures when@wordpress/jest-consoleis enabled. So,@wordpress/jest-consoleitself should already accomplish your proposed end goal: ensureacterrors are not overlooked. I.e. someone is far more likely to notice — and hopefully fix — a failing test (due to anacterror) than a passing test that happens to also log anacterror.
That's correct, however, it doesn't identify the act warnings that happen after the test finishes, which are the ones that we wouldn't notice if we enable @wordpress/jest-console.
Let me share the following table, in order to make clearer the benefits and drawbacks of whether enable @wordpress/jest-console:
Using @wordpress/jest-console |
Not using @wordpress/jest-console |
|---|---|
| Benefits: - act warnings will make the test fail hence, it will require contributors to fix them.- Other log types will also be required to be addressed, which would help in having more robust tests. Drawbacks: - Any type of log that happens before or after a test won't be displayed and either considered a test failure. This implies that for component rendering, some act warnings will be unnoticed. |
Benefits: - We won't experience the drawbacks of using @wordpress/jest-console related to omitting warnings or error logs out of the scope of tests.Drawbacks: - Nevertheless, the logs won't produce test failures, which would require manual observation of test output. - For some tests, the output might be too verbose, unless we mock console functions for those cases. |
There was a problem hiding this comment.
Looks like using Jest v27 would help on this issue, per this comment, I noticed that a recently added test is failing due to an act warning being logged after the test is finished 🤩 , and includes the @wordpress/jest-console (this is the first test we have introduced it).
There was a problem hiding this comment.
That's correct, however, it doesn't identify the
actwarnings that happen after the test finishes, which are the ones that we wouldn't notice if we enable@wordpress/jest-console.
Interesting. I was not aware of that. That is really surprising. I wonder why that is. My understanding of act warnings was that they specifically warn about changes that occur after the test finishes. @wordpress/jest-console "swallows" all act warnings — both during and after tests — but only fails the test for act warnings that occur "during" the test?
@dcalhoun I removed the use of
@wordpress/jest-consolepackage in 4e08331, note that it required a small refactor to cover the same logic. I'd appreciate it if you could take a look and let me know your thoughts about that approach, thanks 🙇 .
I think the new approach is an appropriate solution while we further investigate more global, long-term solutions.
Looks like using Jest v27 would help on this issue, per this comment, I noticed that a recently added test is failing due to an
actwarning being logged after the test is finished 🤩 , and includes the@wordpress/jest-console(this is the first test we have introduced it).
👀 Following along that conversation and exploration as well. 👍🏻
There was a problem hiding this comment.
Interesting. I was not aware of that. That is really surprising. I wonder why that is. My understanding of
actwarnings was that they specifically warn about changes that occur after the test finishes.@wordpress/jest-console"swallows" all act warnings — both during and after tests — but only fails the test for act warnings that occur "during" the test?
Exactly, only fails for act warnings that occur during the test. However, when using Jest v27, I noticed that this behavior changed and also makes the test fail when those warnings happen after the test 🤷♂️ .
I think the new approach is an appropriate solution while we further investigate more global, long-term solutions.
Great, I'll proceed with it, and we'll see how we could improve it in the next iteration 👍.
There was a problem hiding this comment.
Heads up that I re-enabled this import as with Jest v27 the act warnings that occur after the test are also identified as errors.
| jest.isolateModules( () => { | ||
| screen = render( <EditorComponent { ...editorProps } /> ); | ||
| } ); |
There was a problem hiding this comment.
With this approach, we no longer need to reset modules for testing the call order of hooks and callbacks. Besides, I found out that resetting modules was breaking the test cases that render the editor.
| import * as wpHooks from '@wordpress/hooks'; | ||
| import '@wordpress/jest-console'; |
There was a problem hiding this comment.
I found this library quite useful for assuring we don't produce unexpected logs. In the future, it would be great to enable it globally 🤩 .
Agreed. I would love to enable this globally, we'll just need to address all the current warnings and errors in our test logs first.
An important note about using it is that it silences potential logs that happen after the test.
Yeah, this has confused me before while debugging failing web tests, which do enable @wordpress/jest-console globally. In order to intentionally log information, one has to explicitly assert the log should occur.
For example, these test cases are generating "act" warnings (i.e.
An update to EditorProvider inside a test was not wrapped in act(...).) due to the fact that some store state updates are happening after the test is finished. In the beginning, I thought of this as an issue, but I think it's safe to omit them as they are happening after the test ends.
While these particular updates may not dangerous for this specific context, I think it may be dangerous to set the precedent that it is safe to ignore the warnings. It is very possible for a post-assertion update to nullify the validity of a successful assertion. E.g. if one asserts an element is present, a later state update may unexpectedly and erroneously removes the element, which would mean the test is a false positive. The logs occurring "after the test ends" I believe is merely a result of them occurring after the final assertion, which doesn't inherently make them safe.
Is there a way for us to address the act warnings in these tests?
| // It's expected that some blocks are upgraded and inform about it (example: "Updated Block: core/cover") | ||
| expect( console ).toHaveInformed(); |
There was a problem hiding this comment.
By not passing arguments to toHaveInformed, I would imagine we expose ourselves to additional console.info calls accumulating without our knowledge. From reviewing the source of @wordpress/jest-console, I suppose there is no way to further constrain this assertion without passing the entirety of the explicit block update output, which is not manageable unfortunately.
This might be giving us a clue about using outdated versions of blocks in
initialHtmlfile.
With this are you implying that we should fix the initialHtml rather than suppressing the console.info message here?
Due to this package removal the tests related to initialization have been refactored to cover the same logic.
| // Some of the store updates that happen upon editor initialization are executed at the end of the current | ||
| // Javascript block execution and after the test is finished. In order to prevent "act" warnings due to | ||
| // this behavior, we wait for the execution block to be finished before acting on the test. | ||
| await act( | ||
| () => new Promise( ( resolve ) => setImmediate( resolve ) ) | ||
| ); |
There was a problem hiding this comment.
If we don't wait for the JS block to be finished, we get a bunch of act warnings:
Warning: An update to EditorProvider inside a test was not wrapped in act(...).
There was a problem hiding this comment.
Leaving a note that we are actively investigating this issue and approach further in #38052. We should likely retroactively apply whatever global solution we find there to this test.
| // Some of the store updates that happen upon editor initialization are executed at the end of the current | ||
| // Javascript block execution and after the test is finished. In order to prevent "act" warnings due to | ||
| // this behavior, we wait for the execution block to be finished before acting on the test. | ||
| await act( | ||
| () => new Promise( ( resolve ) => setImmediate( resolve ) ) | ||
| ); |
There was a problem hiding this comment.
Leaving a note that we are actively investigating this issue and approach further in #38052. We should likely retroactively apply whatever global solution we find there to this test.
Thanks for the review 🙇 , now that we have a better workaround (the one you introduced for fixing the |
# Conflicts: # packages/react-native-editor/src/test/index.test.js
`initGutenberg` function now returns the Editor component instead of rendering the editor.
Now the initialize editor case renders the actual Editor component including the example initial html.
There was a problem hiding this comment.
@dcalhoun I've updated the PR with the latest changes from trunk, including the Jest v27 updated, which implied applying some updates to the test cases. Additionally, I updated the initializes the editor test case to cover the actual rendering of the editor with the example initial HTML. Let me know if you could do another review, thanks 🙇 !
| import * as wpHooks from '@wordpress/hooks'; | ||
| import '@wordpress/jest-console'; |
There was a problem hiding this comment.
Heads up that I re-enabled this import as with Jest v27 the act warnings that occur after the test are also identified as errors.
| beforeEach( () => { | ||
| // We need to reset modules to guarantee that setup module is imported on every test. | ||
| jest.resetModules(); | ||
| } ); |
There was a problem hiding this comment.
This is no longer necessary as the setup module import is guaranteed by using the jest.isolateModules function when rendering the editor.
| @@ -122,7 +123,7 @@ export function waitFor( | |||
| ).then( () => { | |||
| if ( ! result ) { | |||
| reject( | |||
| `waitFor timed out after ${ timeout }ms for callback:\n${ cb }` | |||
| `waitFor timed out after ${ timeout }ms for callback:\n${ cb }\n${ lastError.toString() }` | |||
There was a problem hiding this comment.
While testing I realized that errors not directly related to timeout, like syntax errors, were omitted. For this reason, I updated this function by displaying now the last error that caused the timeout:
Example 1 - Querying for an unexisting text:
Example 2 - Reference error:
Description
Add test case for covering that the editor initializes correctly and renders the example initial HTML content.
How has this been tested?
Unit Tests / Mobilepasses.npm run native testand observe that all tests pass.Screenshots
N/A
Types of changes
Improvement
Checklist:
*.native.jsfiles for terms that need renaming or removal).