-
Notifications
You must be signed in to change notification settings - Fork 46.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
Bug/Test: Add fuzzing tests for React.Suspense #18721
Conversation
It would actually have detected the issue facebook#18657. It found the following counterexample: ``` [Scheduler` -> [task#2] sequence::Scheduling "8" with priority 3 resolved -> [task#1] promise::Request for "447b0ed" resolved with value "resolved 447b0ed!"`,"447b0ed",[{"priority":3,"text":"8"}],<function :: ["447b0ed"] => true, ["8"] => true>] ``` Reproduced by https://codesandbox.io/s/strange-frost-d4ujl?file=/src/App.js Related to facebook#18669
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 78861fd:
|
What is the process for turning this test into a plain case? |
Meaning this particular failure. |
Could you convert this into a hard-coded test failure? We'll need to make sure the tests pass in master before we can merge it. Or else disable it. I'm still not thrilled with the idea of adding a fuzz test framework, since once it's added as a dependency it's going to give people "permission" to add fuzzing everywhere willy nilly, which is not necessarily a good thing. (Also I appreciate the work you did to write a fuzz tester and report the bug! Setting my quibbles aside, that's very helpful.) My biggest concern is that when it fails, it should easy to debug and convert into something that makes sense. |
For example, one thing I strove for in the other fuzz testers I wrote (which aren't perfect but have been very helpful) is that the test failures can be copy pasted into a hard-coded case: react/packages/react-reconciler/src/__tests__/ReactSuspenseFuzz-test.internal.js Lines 371 to 384 in 30cee2f
It's also easy for me to read this output and guess what might be going on |
To clarify, I'm not opposed to merging some version of this if we fix the bugs in master first. But I would prefer if the output when it fails were more actionable. |
You are totally right, we need to have an easy way to take the output and make it a real test. I'll work on some ways to achieve that. |
Actually there is a built-in way to do that. Whenever a test fails, the framework will report you something like But I agree we can do better.
For the moment, the process is the following. Let's take https://circleci.com/gh/facebook/react/138395?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link as an example. The reported error is:
Given that the error above can be seen as:
And no other tasks have been received (ie no task for |
@gaearon @acdlite What do you think of a CodeSandbox ready output? Something like https://codesandbox.io/s/practical-oskar-6yzgv?file=/src/index.js I drafted a hacky way to do that for the moment. |
I updated the Pull Request, in order to come with better reports. With the latest modification I made, in case of failure, the error log contain a link to a runnable CodeSandbox where you can directly replay the failure (see log below). Here one of those links: here. And here is what has been reported:
In case you want to re-run this precise test in the future, you may have a look here: https://github.com/facebook/react/pull/18721/files#r439758854 |
What do you think of this new version? |
Is there any updates? Maybe worth closing this PR if it does not add value to the test suite of React? |
Sorry, we're pretty behind on reviews in general. Updating a PR is always helpful. Quick thoughts:
|
Thanks a lot for your quick response. I'll work on a proper answer after the rebase |
This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated. |
Summary
This PR introduces a new test for
React.Suspense
. The suggested test is fully based on fuzzing and relies on fast-check library to handle fuzzing and shrinking. It reported the following error - after shrinking - on my side:Reproduced by https://codesandbox.io/s/strange-frost-d4ujl?file=/src/App.js
Issue related to #18669
As adding a new library into React might be problematic, feel free to close this PR if it does not make sense or is not useful anymore. Not adding this library would require to develop a scheduler like the one offered by fast-check in the test suite of react.
Test Plan