Add Reassure mobile performance testing library #47954
Closed
Conversation
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
Only 1 of the 2 tests works. I left notes as to why the other fails. Also, I quickly used the `toBeTruthy` assertion, it may not be the most appropriate. The `toBeInTheDocument` assertion is only relevant to DOM tests, though. I do not believe we can use that.
|
Size Change: 0 B Total Size: 1.33 MB ℹ️ View Unchanged
|
|
Flaky tests detected in 1954d30. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4220376392
|
dcalhoun
reviewed
Feb 10, 2023
There was a problem hiding this comment.
This is great progress and exploration! Thank you for doing this work.
I left comments on the approach in a few places, and raised some larger questions in regards to what we want to test exactly. I recognize the latter is a bit more open-ended and difficult to answer.
packages/block-editor/src/components/rich-text/test/index.perf-test.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/rich-text/test/index.perf-test.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/rich-text/test/index.perf-test.js
Outdated
Show resolved
Hide resolved
Comment on lines
27
to
30
| const scenario = async () => { | ||
| const richTextInput = <RichText />; | ||
| // Simulate user typing text. | ||
| fireEvent( richTextInput, 'focus' ); |
There was a problem hiding this comment.
This test fails currently, but that is expected to me as this line is improper to my understanding. This appears to set richTextInput to a separate copy of RichText than the one passed to measurePerformance. The test currently fails with the following error:
TypeError: Cannot read property 'parent' of undefined
My thought is that maybe this should reference screen passed to the scenario callback.
Suggested change
| const scenario = async () => { | |
| const richTextInput = <RichText />; | |
| // Simulate user typing text. | |
| fireEvent( richTextInput, 'focus' ); | |
| const scenario = async (screen) => { | |
| // Simulate user typing text. | |
| fireEvent( screen, 'focus' ); |
Comment on lines
18
to
24
| fireEvent( richText, 'onChange', { | ||
| nativeEvent: { | ||
| eventCount: 1, | ||
| target: undefined, | ||
| text, | ||
| }, | ||
| } ); |
There was a problem hiding this comment.
This fires a single event of onChange, which makes total sense for test preparation. Speaking at a higher level, while a user this event fires several times for each character. This raises two questions for me:
- Does it make sense to mimic typing by triggering
onChangefor each character of a text set or is that fruitless? - Is rendering and testing
RichTextalone a meaningful performance test or do we need a deeper editor tree?
For the first question, it could be moot. If 1 onChange invocations equals 1 render, then 20 onChange invocations, in theory, equals 20 renders. If they are equal, is there value in mimicking typing via multiple events?
For the second question, this likely comes down to where performance issues originate. Is the problem that RichText renders to frequently due to internal state? Is it due to props from parents? Is it due to context changes? It is difficult to know the most appropriate way to test performance without knowing the origin issue we want to avoid. Avoiding (and tracking) unnecessary re-renders is a good practice, but to what end?
I share this not to say this test is incorrect, but to begin a discussion as to where/how performance tests provide meaningful value.
There was a problem hiding this comment.
Good questions.
Does it make sense to mimic typing by triggering onChange for each character of a text set or is that fruitless?
I don't think it would be a bad thing to measure microinteractions like this. They may produce useful metrics, and the only negative I can see is spending too much time writing Reassure tests. In the beginning of this project, I envisioned that the Reassure test suite would need to expand to potentially every component in order to measure performance effectively. I'm re-evaulating that notion as we continue to write tests and explore testing organization. For the purposes of this PR, I wasn't intending for this to be a very robust test case, but I would very much welcome further discussion of how the structure of these tests could evolve into a suite of robust test cases.
Is rendering and testing RichText alone a meaningful performance test or do we need a deeper editor tree?
This was very much on my mind while writing the test as well -- I was trying to think of ways to avoid false positives or false reinforcement within the test structure itself. Having done a small amount of performance testing with Reassure, I think considering the depth of the editor tree is a very good topic to explore (or potentially discuss with the creators of the library) to see if the depth of nodes makes an impact on performance results, particularly as it relates to tracing exactly where re-renders originate.
packages/block-editor/src/components/rich-text/test/index.perf-test.js
Outdated
Show resolved
Hide resolved
packages/components/src/mobile/html-text-input/test/index.perf-test.js
Outdated
Show resolved
Hide resolved
* test: Linter considers measurePerfomrance an assertion Performance tests often only assert the performance of a component via `measurePerformance`. This change adds `measurePerformance` to the list of functions that are considered assertions to avoid erroneous lint warnings. * test: Fix errors in Rich Text performance test - Remove unnecessary lint warning disable line caused by erroneous placement of `screen` argument. - Query UI components via user-facing attributes, e.g. a11y label. - Pass mock function to required `onChange` prop to `RichText` component. * test: Rename native editor performance test files To avoid web test scripts from including the performance files for the native mobile editor, we must include the term "native" in the file name. This renames the test files to follow the existing pattern of not including "test" in the file name.
|
Closing in favor of #49221 due to merge conflicts: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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, the PR also adds mobile performance tests.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast