Add Reassure mobile performance testing library #49221
Conversation
derekblank
added
Mobile App - i.e. Android or iOS
|
Size Change: +4.41 kB (0%) Total Size: 1.35 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 5468ec0. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4614371090
|
There was a problem hiding this comment.
This looks great! Thank you for putting this proposal together. I left a few comments for your consideration. Let me know what you think. 🙇🏻
Also, you may already be aware, I noticed one of the performance test fails with the following error currently:
Test failure
FAIL ../../test/native/rich-test.perf-native.js (82.749 s)
RichText Performance
✓ performance is stable (1223 ms)
✓ should call onFocus when the TextInput component gains focus (866 ms)
✓ should call onBlue when the TextInput component is blurred (829 ms)
✕ should call onSelectionChange when the selection in the TextInput component changes (3 ms)
✓ should call onKeyPress when a key is pressed while the TextInput component is focused (779 ms)
● RichText Performance › should call onSelectionChange when the selection in the TextInput component changes
TypeError: Cannot read property 'nativeEvent' of undefined
633 | shouldDropEventFromAztec( event, logText ) {
634 | const shouldDrop =
> 635 | ! this.isIOS && event.nativeEvent.eventCount <= this.lastEventCount;
| ^
636 | if ( shouldDrop ) {
637 | window.console.log(
638 | `Dropping ${ logText } from Aztec as its event counter is older than latest sent to the native side. Got ${ event.nativeEvent.eventCount } but lastEventCount is ${ this.lastEventCount }.`
at RichText.nativeEvent (../../packages/rich-text/src/component/index.native.js:635:26)
at RichText.shouldDropEventFromAztec (../../packages/rich-text/src/component/index.native.js:645:13)
derekblank
requested review from
dcalhoun
and removed request for
gziolo,
ntwb,
ajitbohra,
nerrad,
ellatrix and
fluiddot
| @@ -108,6 +108,8 @@ | |||
| "test": "cross-env NODE_ENV=test jest --verbose --config ../../test/native/jest.config.js", | |||
| "test:debug": "cross-env NODE_ENV=test node --inspect-brk ../../node_modules/.bin/jest --runInBand --verbose --config ../../test/native/jest.config.js", | |||
| "test:update": "npm run test -- --updateSnapshot", | |||
| "test:perf": "cross-env NODE_ENV=test TEST_RUNNER_PATH=../../node_modules/.bin/jest TEST_RUNNER_ARGS='--runInBand --testMatch \"**/*.perf-native.[jt]s?(x)\" --verbose --config ../../test/native/jest.config.js' reassure", | |||
There was a problem hiding this comment.
What is the rationale behind the perf-native.js suffix? How much work would it be to use a different folder name for this type of tests like benchmark or something similar?
There was a problem hiding this comment.
The perf-native.js suffix is the default file path pattern matcher for the Reassure library; that is why it was utilized. Relatedly, it appears the Reassure project intends to further align with Jest's default pattern matcher using the "perf" name in: callstack/reassure#363.
Modifying our pattern matcher is low effort. I am not opposed to doing so. From reviewing the Gutenberg project's config, there are a couple of existing patterns:
gutenberg/packages/jest-preset-default/jest-preset.js
Lines 21 to 25 in c61ba1e
I'm not too sure why this project's preset configuration supports both __tests__ (Jest default) and test (Gutenberg customization). Existing web performance tests rely upon performance, not benchmark.
@gziolo is your proposal to match **/benchmark/*.test.js for native performance tests?
There was a problem hiding this comment.
'**/test/*.[jt]s?(x)', - is my biggest concern as it might also recognize the file name as a unit test for web code.
I'm not too sure why this project's preset configuration supports both tests (Jest default) and test (Gutenberg customization).
I don't remember exactly now, but I belive this pattern existed before Jest got integrated with Gutenberg.
is your proposal to match **/benchmark/*.test.js for native performance tests?
You will also find benchmark folders, too. performance works for me. The extension of the file isn't a concern, only the folder name used.
There was a problem hiding this comment.
'**/test/*.[jt]s?(x)',- is my biggest concern as it might also recognize the file name as a unit test for web code.
@gziolo ah, I see. The following configuration excludes any native-specific test files, at least in the context of the test:unit script.
gutenberg/test/unit/jest.config.js
Line 35 in 77feb60
Although, my guess is that the current exclusion of the new performance tests is accidental and originates from the lack of escaping the period character.
diff --git a/test/unit/jest.config.js b/test/unit/jest.config.js
index 9d19a5c9fe..39764020cb 100644
--- a/test/unit/jest.config.js
+++ b/test/unit/jest.config.js
@@ -32,7 +32,7 @@ module.exports = {
'<rootDir>/.*/build-module/',
'<rootDir>/.*/build-types/',
'<rootDir>/.+.d.ts$',
- '<rootDir>/.+.native.js$',
+ '<rootDir>/.+\\.native.js$',
'/packages/react-native-*',
],
resolver: '<rootDir>/test/unit/scripts/resolver.js',Regardless, updating our approach to utilize a performance directory works for me. An example mobile performance test file would be packages/block-library/src/paragraph/performance/paragraph.native.js.
Note: the file would not include a test suffix.
There was a problem hiding this comment.
We also use pattern matching for linting, for example:
https://github.com/WordPress/gutenberg/blob/trunk/.eslintrc.js#L369L383
It isn't impossible to make it work with exceptions like currently but it looks like we could avoid it with the /performance/ subdirectory.
There was a problem hiding this comment.
@gziolo @dcalhoun Thanks for the feedback. Based on the discussion, I've moved the *.perf-native.js performance tests to their own colocated /performance/ subdirectory, so the updated directory path would be packages/block-library/src/paragraph/performance/paragraph.perf-native.js.
Based on this above comment, however, is the consensus that we want to use the extension *.native.js in place of *.perf-test.js as well? I am fine with also making this change, but just seeking to clarify if it's still necessary with the added /performance/ directory.
There was a problem hiding this comment.
Based on this above comment, however, is the consensus that we want to use the extension
*.native.jsin place of*.perf-test.jsas well? I am fine with also making this change, but just seeking to clarify if it's still necessary with the added/performance/directory.
Yes, my inclination was to utilize *.native.js as opposed to *.perf-native.js as I believe the perf portion is now superfluous due to the new performance directory.
There was a problem hiding this comment.
Great, makes sense. I've updated the file extensions to *.native.js.
packages/block-library/src/paragraph/test/paragraph.perf-native.js
Outdated
Show resolved
Hide resolved
packages/block-library/src/paragraph/test/paragraph.perf-native.js
Outdated
Show resolved
Hide resolved
| attributes={ {} } | ||
| setAttributes={ jest.fn() } |
There was a problem hiding this comment.
As I reflect on this test structure and our findings regarding the mobile Rich Text performance, I consider the fact that this test structure partially covers the two events we know cause high CPU usage:
Given this test structure mocks attributes and setAttributes, the resulting prop changes that ultimately cause Paragraph — and its ancestors and children — to re-render are not covered in this test. I do not share that to say this test is flawed or lacking value, just that we need to consider if/how we should test the other half of the performance problem.
There was a problem hiding this comment.
The initial intent of this test was to create a deeper editor tree to create a more realistic editing experience using Paragraph block as an ancestor of RichText. The mocked props came from the Paragraph snapshot test, but you make a good point that they're not really testing a major cause of the performance problem while mocked. Given that the performance tests are not using the initializeEditor format like our other integration tests, do you see any obvious ways we could not mock these events in this test in order to test setAttributes? Perhaps something like this (in pseudocode), where RichText.onChange could fire RichText.onTextUpdate and then setAttributes?
<Paragraph
attributes={ { content } }
setAttributes={ jest.fn() }
onChange={ ( content ) => setAttributes( { content } ) }
/>
Alternatively, should we look at moving this test to a parent component that implements Paragraph as a child, or adding those as parent components here?
There was a problem hiding this comment.
The intent makes complete sense and aligns with our previous conversations. Reviewing the implementation merely revealed to me the consideration I shared regarding uncovered portions of the performance issues.
Perhaps something like this (in pseudocode), where RichText.onChange could fire RichText.onTextUpdate and then setAttributes?
If we mock setAttributes we could manually assert the number of times it is invoked. This could be an approach to track the number of time the store is updated or indirectly track subsequent renders that occur when attributes change. However, it would not provide a way to understand or measure the duration for renders that occur when attributes change. To test render durations, our test subject would need to be an ancestor — as you reference — similar to how initializeEditor renders a far deeper tree.
It is is difficult to say which approach is valuable without knowing the optimization we would like to track. The current test structure could provide value if we want to track optimizations within Paragraph that are not triggered by prop updates from ancestors.
My hunch is that we will need to have our test subject be a deeper tree to monitor the impact of onSelectionChange and setAttributes. That would involve creating a form of initializeEditor that is compatible with Reassure. Example pseudo code:
await measurePerformance( <InitializeEditor initialHtml={ paragraphMarkup } />, { scenario: typeWithinParagraph } );There was a problem hiding this comment.
My hunch is that we will need to have our test subject be a deeper tree to monitor the impact of
onSelectionChangeandsetAttributes. That would involve creating a form ofinitializeEditorthat is compatible with Reassure.
I think this is a great idea, and would be a succinct way to generally test any block using Reassure if we could make it work. Given that Reassure is essentially a thin render wrapper around React <Profiler />, I think we could modify the equivalent render code in a new perf-test specific copy of initializeEditor to be compatible with Reassure's render implementation. I'll look at the output of createElement and cloneElement to see what else we'd need to modify to pass child elements to Reassure's render implementation.
packages/block-library/src/paragraph/test/paragraph.perf-native.js
Outdated
Show resolved
Hide resolved
| const onChange = jest.fn(); | ||
| const onSelectionChange = jest.fn(); | ||
|
|
||
| it( 'performance is stable when typing using Rich Text', async () => { |
There was a problem hiding this comment.
We discussed and changed the description to avoid duplication at one point. I am uncertain if it was intentionally changed back. If it was intentional, feel free to ignore this.
| it( 'performance is stable when typing using Rich Text', async () => { | |
| it( 'is stable when typing', async () => { |
What?
Adds the Reassure performance testing library to test mobile performance on React Native components.
Why?
To help analyze and address performance issues within mobile components.
How?
In addition to adding the Reassure library and configuration, the PR also adds mobile performance tests for the native Rich Text component.
Testing Instructions
npm run native test:perf:baselineto set baseline performance testsnpm run native test:perfto test any code changes against the baseline for performance.