-
Notifications
You must be signed in to change notification settings - Fork 47k
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
Suspense fuzz tester #14147
Suspense fuzz tester #14147
Conversation
packages/react-reconciler/src/__tests__/ReactSuspenseFuzz-test.internal.js
Outdated
Show resolved
Hide resolved
ebb0ee7
to
61937c5
Compare
ReactDOM: size: 🔺+0.1%, gzip: 🔺+0.2% Details of bundled changes.Comparing: f9e9913...a8e9909 react-dom
react-art
react-test-renderer
react-reconciler
react-native-renderer
scheduler
Generated by 🚫 dangerJS |
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.
Does this catch the bugs fixed by #14083 too?
d42f01f
to
d9666f1
Compare
Technically this doesn't test that updates are handled consistently, does it? Would it be more solid if we instead incremented every value in the cache before suspending, and the test verified that all rendered output is original output + 1 after it settles? |
d9666f1
to
01c1093
Compare
@gaearon Could you rephrase? Are you concerned that the cache isn't being cleared in between renders? Each one uses a separate cache. |
Here's an example test case. In this test, the Text component suspends for 2 seconds on initial mount. 100 ms after that, it updates itself by incrementing a counter in local state. That update will suspend for 200 ms. The Container component will also schedule an update remount once after 250 ms, which will trigger the Text mount and updates again (since it's a fresh component). react/packages/react-reconciler/src/__tests__/ReactSuspenseFuzz-test.internal.js Lines 346 to 354 in 01c1093
The runner will keep advancing time until everything settles, both promises and updates. The final output of this test is |
Vestigial behavior that should have been removed in facebook#13823. Found using the Suspense fuzz tester in facebook#14147.
Vestigial behavior that should have been removed in facebook#13823. Found using the Suspense fuzz tester in facebook#14147.
@sophiebits Yep! |
01c1093
to
481e594
Compare
The fuzzer works by generating a random tree of React elements. The tree two types of custom components: - A Text component suspends rendering on initial mount for a fuzzy duration of time. It may update a fuzzy number of times; each update supsends for a fuzzy duration of time. - A Container component wraps some children. It may remount its children a fuzzy number of times, by updating its key. The tree may also include nested Suspense components. After this tree is generated, the tester sets a flag to temporarily disable Text components from suspending. The tree is rendered synchronously. The output of this render is the expected output. Then the tester flips the flag back to enable suspending. It renders the tree again. This time the Text components will suspend for the amount of time configured by the props. The tester waits until everything has resolved. The resolved output is then compared to the expected output generated in the previous step. Finally, we render once more, but this time in concurrent mode. Once again, the resolved output is compared to the expected output. I tested by commenting out various parts of the Suspense implementation to see if broke in the expected way. I also confirmed that it would have caught facebook#14133, a recent bug related to deletions.
Adds more fuzziness to the generated tests. Specifcally, introduces nested Suspense cases, where the fallback of a Suspense component also suspends. This flushed out a bug (yay!) whose test case I've hard coded.
481e594
to
b0e538b
Compare
So if there's a failure, we can bisect.
2e2401e
to
a8e9909
Compare
…acebook#14157) Vestigial behavior that should have been removed in facebook#13823. Found using the Suspense fuzz tester in facebook#14147.
* Don't warn if an unmounted component is pinged * Suspense fuzz tester The fuzzer works by generating a random tree of React elements. The tree two types of custom components: - A Text component suspends rendering on initial mount for a fuzzy duration of time. It may update a fuzzy number of times; each update supsends for a fuzzy duration of time. - A Container component wraps some children. It may remount its children a fuzzy number of times, by updating its key. The tree may also include nested Suspense components. After this tree is generated, the tester sets a flag to temporarily disable Text components from suspending. The tree is rendered synchronously. The output of this render is the expected output. Then the tester flips the flag back to enable suspending. It renders the tree again. This time the Text components will suspend for the amount of time configured by the props. The tester waits until everything has resolved. The resolved output is then compared to the expected output generated in the previous step. Finally, we render once more, but this time in concurrent mode. Once again, the resolved output is compared to the expected output. I tested by commenting out various parts of the Suspense implementation to see if broke in the expected way. I also confirmed that it would have caught facebook#14133, a recent bug related to deletions. * When a generated test case fails, log its input * Moar fuzziness Adds more fuzziness to the generated tests. Specifcally, introduces nested Suspense cases, where the fallback of a Suspense component also suspends. This flushed out a bug (yay!) whose test case I've hard coded. * Use seeded random number generator So if there's a failure, we can bisect.
…acebook#14157) Vestigial behavior that should have been removed in facebook#13823. Found using the Suspense fuzz tester in facebook#14147.
* Don't warn if an unmounted component is pinged * Suspense fuzz tester The fuzzer works by generating a random tree of React elements. The tree two types of custom components: - A Text component suspends rendering on initial mount for a fuzzy duration of time. It may update a fuzzy number of times; each update supsends for a fuzzy duration of time. - A Container component wraps some children. It may remount its children a fuzzy number of times, by updating its key. The tree may also include nested Suspense components. After this tree is generated, the tester sets a flag to temporarily disable Text components from suspending. The tree is rendered synchronously. The output of this render is the expected output. Then the tester flips the flag back to enable suspending. It renders the tree again. This time the Text components will suspend for the amount of time configured by the props. The tester waits until everything has resolved. The resolved output is then compared to the expected output generated in the previous step. Finally, we render once more, but this time in concurrent mode. Once again, the resolved output is compared to the expected output. I tested by commenting out various parts of the Suspense implementation to see if broke in the expected way. I also confirmed that it would have caught facebook#14133, a recent bug related to deletions. * When a generated test case fails, log its input * Moar fuzziness Adds more fuzziness to the generated tests. Specifcally, introduces nested Suspense cases, where the fallback of a Suspense component also suspends. This flushed out a bug (yay!) whose test case I've hard coded. * Use seeded random number generator So if there's a failure, we can bisect.
I know that this PR is not recent at all and I felt on the test while browsing the source code of React. Out of curiosity: Why don't you used property based testing libraries to help you to generate those random tests? You may have a look to fast-check, jsverify or testcheck-js for more details. |
Wanna send a PR converting it? :-) For me personally learning the API has been a barrier. I couldn’t actually figure out how to generate recursive things with testcheck-js. |
Yes, I will give it a try but with fast-check as it handles recursive types (the lib is used by jest and jasmine) |
@gaearon Here is a first draft of the test migrated to property based testing. If you believe it can be useful, I can rework and clean a bit the code before opening a PR. See dubzzz@0b1df02 Side note: For the moment I removed the variable |
Looks interesting. Send a PR and let’s discuss? |
I just opened the PR to move fuzz tests of ReactSuspense to property based testing. I will also migrate |
The fuzzer works by generating a random tree of React elements. The tree two types of custom components:
The tree may also include nested Suspense components.
After this tree is generated, the tester sets a flag to temporarily disable Text components from suspending. The tree is rendered synchronously. The output of this render is the expected output.
Then the tester flips the flag back to enable suspending. It renders the tree again. This time the Text components will suspend for the amount of time configured by the props. The tester waits until everything has resolved. The resolved output is then compared to the expected output generated in the previous step.
Finally, we render once more, but this time in concurrent mode. Once again, the resolved output is compared to the expected output.
I tested by commenting out various parts of the Suspense implementation to see if broke in the expected way. I also confirmed that it would have caught #14133, a recent bug related to deletions.
The final commit also includes a fix for a bug the tester found. I can split this into a separate PR.