Skip to content

Add More Fuzz Tests #18669

Closed
Closed
@gaearon

Description

@gaearon

There’s some things we don’t have a sufficient coverage of. Currently we catch them from production product bugs but this is not sustainable. We’re hoping some refactors will drastically simplify the model — but nevertheless we should invest in better fuzz test coverage. That’s how we caught similar bugs before at an earlier stage.

One thing we’re lacking coverage for is what happens to Suspense boundaries as updates are dispatched at different priorities in different order, and what happens when we’ve had to yield. We need to verify that Suspense always “wakes up” when Promises are resolved and there’s nothing to be suspended on. We also need to test this in combination with render phase updates.

Here’s examples of bugs that I want a fuzzer to catch: #18657 #18020 #18486 #18357 #18353 #18644 #18412.

@dubzzz I believe you were interested in this? This would take some effort but would be a major contribution.

Activity

dubzzz

dubzzz commented on Apr 18, 2020

@dubzzz

Yes, I'll be really interested in helping on this subject.

For the moment, I already drafted some fuzz tests based on property based testing for SuspenseList (here). They detected the issue #18661.

Waiting your feedback on the following scenarii and on #18673.


Here are the scenarii I am covering so far with those tests - please let me know if my scenarii are based on wrong hypothesis:

  • should wait all components to be resolved before showing any in "together" mode

The test generates a random tree of components with only SuspenseList having mode="together" and Suspense.

If there is still one pending Suspense, it checks that the SuspenseList is still waiting and not rendering anything. Then it resolves out-of-order one of the pending Suspense.

As soon as everything gets resolved, the test checks that everything is displayed properly.

  • should display components up-to the first unresolved one as resolved, next ones should be considered unresolved in "forward" mode

Close to the test described above but for mode="forwards".

The check is obviously different: We expect all the Suspense up to the first unresolved one not included to be shown correctly. And starting from the first unresolved one we expect them to show the fallback.

In this test (it is not the case in the first one), we render a first tree of components then a second one with some nodes identical, some removed, some added. This second render might happen any time between two resolutions of Suspense.

Example:

Tree A::
<SuspenseList key="a" mode="forwards">
  <Suspense key="a.a"><A/></Suspense>
  <Suspense key="a.c"><C/></Suspense>
</SuspenseList>

Tree B::
<SuspenseList key="a" mode="forwards">
  <Suspense key="a.a"><A/></Suspense>
  <Suspense key="a.b"><B/></Suspense>
</SuspenseList>

Resolution order might be (render Tree A is done first):
- B Loaded        -- test expects [Loading A, Loading C]
- Render Tree B   -- test expects [Loading A, Loading B]
- C Loaded        -- test expects [Loading A, Loading B]
- A Loaded        -- test expects [A, B]

All the scenarii described above can be replayed again and again in case of bug as they rely on a seed (the scheduler handling the scheduling of which Suspense should resolve when is based on the seed).

gaearon

gaearon commented on Apr 19, 2020

@gaearon
CollaboratorAuthor

I would probably focus on Suspense itself (and suspending at different priority levels) before spending too much effort on SuspenseList. The core Suspense bugs are more important because you can’t “opt out” of them, like you can by removing SuspenseList.

dubzzz

dubzzz commented on Apr 22, 2020

@dubzzz

I'll start to work on a possible way to detect those kinds of bugs with fuzzing. I'll just need to get through some of the bugs you put just to see what is the recurrent cause of bug in order to cover it (and more)

added 2 commits that reference this issue on Apr 23, 2020

Detect facebook#18657 thanks to fuzzing

Detect facebook#18657 thanks to fuzzing

dubzzz

dubzzz commented on Apr 23, 2020

@dubzzz

I implemented a first version only focusing on Suspense. It already detected what looks to be an issue https://codesandbox.io/s/strange-frost-d4ujl?file=/src/App.js 🤔

By the way, the issue looks similar to the one you reported in #18657. I'll need to rebase my branch to check if it still detects issues on the latest version of master.

The test renders the following component:

<Suspense fallback={<Text text="Loading..." />}>
  <App />
</Suspense>

with App:

function App() {
  const [text, setText] = React.useState(initialText);
  return <AsyncText text={text} readOrThrow={readOrThrow} />;
}

It reorders when AsyncText will resolve for a given text and applies "randomly" updates of the text by calling Scheduler.unstable_runWithPriority.

The code of the test is avalaible here: dubzzz@45b9398

gaearon

gaearon commented on Apr 23, 2020

@gaearon
CollaboratorAuthor

Wow, that’s great! Send a PR? 🙂

I think maybe it’s worth also trying to write a vanilla version per @acdlite’s point. Maybe their difference will convince him the library is worth it? Or maybe the other way around.

added a commit that references this issue on Apr 23, 2020
dubzzz

dubzzz commented on Apr 23, 2020

@dubzzz

@gaearon @acdlite As the test might help you to detect other issues on Suspense, I opened a PR based on fast-check. I know that adding a new library might be problematic so feel free to close it. For now, I have not worked on an implementation framework-free as it will require lots of work to develop the scheduling logic part and might be error-prone.

stale

stale commented on Jul 25, 2020

@stale

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

stale

stale commented on Aug 1, 2020

@stale

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @gaearon@dubzzz

        Issue actions

          Add More Fuzz Tests · Issue #18669 · facebook/react