Migrate ReactSuspense fuzz tests to Property Based Tests #18673
Conversation
|
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 6b4c926:
|
|
I am very disinclined to add a library just for the shrinking capability. Is there some other advantage? |
|
To elaborate, automatic shrinking solves a problem that I have not ever experienced. I'm skeptical of adding a layer of third-party abstractions around what is essentially a fancy way of generating random values. I also don't see much point in converting an existing test file that works fine as-is. If you want to add a new fuzz tester that uses one of those libraries, I'll put up less of a fight. I'll still challenge you on the need to add a dependency. Maybe I'd merge the PR then rewrite it without the library :) |
|
I totally get your point. I'd say the most important benefit you'll get from fast-check are not those migrated tests today. Or at least for the moment, as they are green, everything will look the same for you. Shrinking might be helpful and clearly you can leave without that. But I believe fast-check will be really helpful for tests concerning detecting race conditions in Suspense and SuspenseList. It already proved useful as it detected #18661 and I believe it can be a master piece to help you with issue #18669. Indeed, in addition to classical property based testing capabilities it comes with a built-in support to reschedule asynchrounous operations and user interactions. You may be interested to get a look to the tests I wrote for Suspense and SuspenseList on dubzzz@e2cb477 (and described in the issue #18669 - and in this code snippet showing how the scheduler feature could be used to test races on an autocomplete). Something that might also be useful is that fast-check will by default start with "smaller" generated entries and move towards "larger" ones as the test goes (referred as biased in the documentation). While it may look useless, it is pretty helpful in general to detect bugs. Some bugs will never be detected with full random. For instance, if you want to check the stability of a sort you can say sortOnFirstName(sortOnLastName()) is sortOnFirstNameThenOnLastName(). If to test that with fuzz technics you only generate random arrays of objects with two strings it might takes very long to have a single case where first name is duplicated and even longer to find a bug on stability. In case, it can help, fast-check is already used by Jest and Jasmine. |
|
How are you distinguishing fuzz tests from property tests? What would you say is the key difference? In my view, our existing fuzz tests are property tests because they test properties (mostly consistency) that should always hold true. We call them “fuzz” tests because they use randomly generated values to do it. It still sounds to me like you’re mostly extolling the benefits of property testing and not specifically these libraries. I don’t think I need to be sold on property tests, only on using a framework to do it. |
|
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. |
|
Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you! |
Summary
As discussed with @gaearon in #14147 and in the context of #18669, here is a Pull Request to move some of the tests using home-made fuzzing to property based testing solutions.
The advantage of switching to a property based testing approach instead of manually generated random values is that property based testing frameworks come with shrinking capabilities by default. It means that anytime such framework detects an issue in a test (also referred as property) it will try to produce simpler values producing the error too.
As of today, there are mainly 3 leading libraries for property based testing: fast-check, jsverify or testcheck-js. fast-check is the choice added by this PR as it comes with features that would be pretty useful in the future: built-in helpers to detect race-conditions.
Depending on how much time it takes me to migrate
ReactNewContext-test.jsto fast-check, I might add this test to the patch too.Test Plan
Move to property based testing for ReactSuspense fuzz tests.