-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Add Reassure mobile performance testing library #47954
Conversation
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
|
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.
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
const scenario = async () => { | ||
const richTextInput = <RichText />; | ||
// Simulate user typing text. | ||
fireEvent( richTextInput, 'focus' ); |
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.
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.
const scenario = async () => { | |
const richTextInput = <RichText />; | |
// Simulate user typing text. | |
fireEvent( richTextInput, 'focus' ); | |
const scenario = async (screen) => { | |
// Simulate user typing text. | |
fireEvent( screen, 'focus' ); |
fireEvent( richText, 'onChange', { | ||
nativeEvent: { | ||
eventCount: 1, | ||
target: undefined, | ||
text, | ||
}, | ||
} ); |
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.
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
onChange
for each character of a text set or is that fruitless? - Is rendering and testing
RichText
alone 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: |
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