-
Notifications
You must be signed in to change notification settings - Fork 4k
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
[RNMobile] Refactor react-native-editor
initialization test
#37955
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
e8d809f
Refactor react-native-editor initialization test
fluiddot ae0aa9f
Remove commented line code
fluiddot b1964b5
Get block list component synchronously
fluiddot 4e08331
Remove jest-console package import
fluiddot 78f61bb
Merge branch 'trunk' into rnmobile/update/editor-initialization-tests
fluiddot 493e054
Improve error handling in waitFor test helper
fluiddot c89ed9d
Update register gutenberg tests
fluiddot d24cd67
Update initialize editor test case
fluiddot f43de21
Remove async from initialize editor test case
fluiddot File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,14 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import { AppRegistry, Text } from 'react-native'; | ||
import { render, waitFor } from 'test/helpers'; | ||
import { AppRegistry } from 'react-native'; | ||
import { initializeEditor, render } from 'test/helpers'; | ||
|
||
/** | ||
* WordPress dependencies | ||
*/ | ||
import * as wpHooks from '@wordpress/hooks'; | ||
import '@wordpress/jest-console'; | ||
|
||
/** | ||
* Internal dependencies | ||
|
@@ -18,24 +19,18 @@ import setupLocale from '../setup-locale'; | |
jest.mock( 'react-native/Libraries/ReactNative/AppRegistry' ); | ||
jest.mock( '../setup-locale' ); | ||
|
||
const initGutenberg = ( registerParams ) => { | ||
const getEditorComponent = ( registerParams ) => { | ||
let EditorComponent; | ||
AppRegistry.registerComponent.mockImplementation( | ||
( name, componentProvider ) => { | ||
EditorComponent = componentProvider(); | ||
} | ||
); | ||
registerGutenberg( registerParams ); | ||
|
||
return render( <EditorComponent /> ); | ||
return EditorComponent; | ||
}; | ||
|
||
describe( 'Register Gutenberg', () => { | ||
beforeEach( () => { | ||
// We need to reset modules to guarantee that setup module is imported on every test. | ||
jest.resetModules(); | ||
} ); | ||
Comment on lines
-34
to
-37
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is no longer necessary as the setup module import is guaranteed by using the |
||
|
||
it( 'registers Gutenberg editor component', () => { | ||
registerGutenberg(); | ||
expect( AppRegistry.registerComponent ).toHaveBeenCalled(); | ||
|
@@ -53,7 +48,9 @@ describe( 'Register Gutenberg', () => { | |
}; | ||
} ); | ||
|
||
initGutenberg(); | ||
const EditorComponent = getEditorComponent(); | ||
// Modules are isolated upon editor rendering in order to guarantee that the setup module is imported on every test. | ||
jest.isolateModules( () => render( <EditorComponent /> ) ); | ||
|
||
// "invocationCallOrder" can be used to compare call orders between different mocks. | ||
// Reference: https://git.io/JyBk0 | ||
|
@@ -77,7 +74,9 @@ describe( 'Register Gutenberg', () => { | |
}; | ||
} ); | ||
|
||
initGutenberg( { beforeInitCallback } ); | ||
const EditorComponent = getEditorComponent( { beforeInitCallback } ); | ||
// Modules are isolated upon editor rendering in order to guarantee that the setup module is imported on every test. | ||
jest.isolateModules( () => render( <EditorComponent /> ) ); | ||
|
||
// "invocationCallOrder" can be used to compare call orders between different mocks. | ||
// Reference: https://git.io/JyBk0 | ||
|
@@ -94,16 +93,18 @@ describe( 'Register Gutenberg', () => { | |
|
||
// An empty component is provided in order to listen for render calls of the editor component. | ||
const onRenderEditor = jest.fn(); | ||
const EditorComponent = () => { | ||
const MockEditor = () => { | ||
onRenderEditor(); | ||
return null; | ||
}; | ||
jest.mock( '../setup', () => ( { | ||
__esModule: true, | ||
default: jest.fn().mockReturnValue( <EditorComponent /> ), | ||
default: jest.fn().mockReturnValue( <MockEditor /> ), | ||
} ) ); | ||
|
||
initGutenberg(); | ||
const EditorComponent = getEditorComponent(); | ||
// Modules are isolated upon editor rendering in order to guarantee that the setup module is imported on every test. | ||
jest.isolateModules( () => render( <EditorComponent /> ) ); | ||
|
||
const hookCallIndex = 0; | ||
// "invocationCallOrder" can be used to compare call orders between different mocks. | ||
|
@@ -123,16 +124,18 @@ describe( 'Register Gutenberg', () => { | |
|
||
// An empty component is provided in order to listen for render calls of the editor component. | ||
const onRenderEditor = jest.fn(); | ||
const EditorComponent = () => { | ||
const MockEditor = () => { | ||
onRenderEditor(); | ||
return null; | ||
}; | ||
jest.mock( '../setup', () => ( { | ||
__esModule: true, | ||
default: jest.fn().mockReturnValue( <EditorComponent /> ), | ||
default: jest.fn().mockReturnValue( <MockEditor /> ), | ||
} ) ); | ||
|
||
initGutenberg(); | ||
const EditorComponent = getEditorComponent(); | ||
// Modules are isolated upon editor rendering in order to guarantee that the setup module is imported on every test. | ||
jest.isolateModules( () => render( <EditorComponent /> ) ); | ||
|
||
const hookCallIndex = 0; | ||
// "invocationCallOrder" can be used to compare call orders between different mocks. | ||
|
@@ -152,16 +155,18 @@ describe( 'Register Gutenberg', () => { | |
|
||
// An empty component is provided in order to listen for render calls of the editor component. | ||
const onRenderEditor = jest.fn(); | ||
const EditorComponent = () => { | ||
const MockEditor = () => { | ||
onRenderEditor(); | ||
return null; | ||
}; | ||
jest.mock( '../setup', () => ( { | ||
__esModule: true, | ||
default: jest.fn().mockReturnValue( <EditorComponent /> ), | ||
default: jest.fn().mockReturnValue( <MockEditor /> ), | ||
} ) ); | ||
|
||
initGutenberg(); | ||
const EditorComponent = getEditorComponent(); | ||
// Modules are isolated upon editor rendering in order to guarantee that the setup module is imported on every test. | ||
jest.isolateModules( () => render( <EditorComponent /> ) ); | ||
|
||
const hookCallIndex = 1; | ||
// "invocationCallOrder" can be used to compare call orders between different mocks. | ||
|
@@ -176,19 +181,17 @@ describe( 'Register Gutenberg', () => { | |
expect( hookCallOrder ).toBeGreaterThan( onRenderEditorCallOrder ); | ||
} ); | ||
|
||
it( 'initializes the editor', async () => { | ||
const MockEditor = () => <Text>Mock Editor</Text>; | ||
jest.mock( '../setup', () => { | ||
return { | ||
__esModule: true, | ||
default: jest.fn( () => <MockEditor /> ), | ||
}; | ||
} ); | ||
it( 'initializes the editor', () => { | ||
// Unmock setup module to render the actual editor component. | ||
jest.unmock( '../setup' ); | ||
|
||
const EditorComponent = getEditorComponent(); | ||
const screen = initializeEditor( {}, { component: EditorComponent } ); | ||
const blockList = screen.getByTestId( 'block-list-wrapper' ); | ||
|
||
const screen = initGutenberg(); | ||
const blockList = await waitFor( () => | ||
screen.getByText( 'Mock Editor' ) | ||
); | ||
expect( blockList ).toBeDefined(); | ||
expect( blockList ).toHaveProperty( 'type', 'View' ); | ||
expect( console ).toHaveLoggedWith( 'Hermes is: true' ); | ||
// It's expected that some blocks are upgraded and inform about it (example: "Updated Block: core/cover") | ||
expect( console ).toHaveInformed(); | ||
} ); | ||
} ); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -101,11 +101,12 @@ export function waitFor( | |
{ timeout, interval } = { timeout: 1000, interval: 50 } | ||
) { | ||
let result; | ||
let lastError; | ||
const check = ( resolve, reject, time = 0 ) => { | ||
try { | ||
result = cb(); | ||
} catch ( e ) { | ||
//NOOP | ||
} catch ( error ) { | ||
lastError = error; | ||
} | ||
if ( ! result && time < timeout ) { | ||
setTimeout( | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
); | ||
return; | ||
} | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, I'm planning to open a ticket specific to addressing this topic.
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.Actually, I managed to find a workaround by delaying the query passed to the
waitFor
function an execution block cycle (check thernmobile/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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.Agreed. We appear to be aligned that it is not safe or good practice to ignore the warnings or assume they are safe.
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: ensureact
errors are not overlooked. I.e. someone is far more likely to notice — and hopefully fix — a failing test (due to anact
error) than a passing test that happens to also log anact
error.Example Failure from act + @wordpress/jest-console
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
:@wordpress/jest-console
@wordpress/jest-console
-
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.- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dcalhoun I removed the use of
@wordpress/jest-console
package 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 🙇 .There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?I think the new approach is an appropriate solution while we further investigate more global, long-term solutions.
👀 Following along that conversation and exploration as well. 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🤷♂️ .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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.