Bug/Test: Add fuzzing tests for React.Suspense #18721

dubzzz · 2020-04-23T20:50:32Z

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:

[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

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

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

codesandbox-ci · 2020-04-23T20:52:53Z

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:

Sandbox	Source
dank-mountain-s8mgk	Configuration
elated-violet-oh8uv	PR

sizebot · 2020-04-23T20:56:33Z

	Warnings
⚠️	Failed to fetch build artifacts for base commit: `655affa`

Size changes (stable)

Generated by 🚫 dangerJS against 7128522

sizebot · 2020-04-23T20:58:55Z

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 7128522

gaearon · 2020-04-23T21:57:54Z

What is the process for turning this test into a plain case?

gaearon · 2020-04-23T21:58:11Z

Meaning this particular failure.

acdlite · 2020-04-23T22:09:43Z

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.

acdlite · 2020-04-23T22:12:50Z

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

    
           testResolvedOutput( 
        
             <> 
        
               <Suspense fallback="Loading..."> 
        
                 <Text initialDelay={7200} text="A" /> 
        
               </Suspense> 
        
               <Suspense fallback="Loading..."> 
        
                 <Container> 
        
                   <Text initialDelay={1000} text="B" /> 
        
                   <Text initialDelay={7200} text="C" /> 
        
                   <Text initialDelay={9000} text="D" /> 
        
                 </Container> 
        
               </Suspense> 
        
             </>, 
        
           );

It's also easy for me to read this output and guess what might be going on

acdlite · 2020-04-23T22:16:06Z

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.

dubzzz · 2020-04-24T07:05:57Z

Could you convert this into a hard-coded test failure?

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.

dubzzz · 2020-04-24T07:50:58Z

Could you convert this into a hard-coded test failure?

Actually there is a built-in way to do that. Whenever a test fails, the framework will report you something like { seed: 2045942474, path: "1:0:1:4:0:1:0:1:0:2:0:1:1:0:0", endOnFailure: true }. By using this payload as the second argument of fc.assert you should only run the failing case.

But I agree we can do better.

What is the process for turning this test into a plain case?

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:

    Property failed after 2 tests
    { seed: 2045942474, path: "1:0:1:4:0:1:0:1:0:2:0:1:1:0:0", endOnFailure: true }
    Counterexample: [Scheduler`
    -> [task#2] sequence::Scheduling "1" with priority 1 resolved
    -> [task#1] promise::Request for "" resolved`,"",[{"priority":1,"text":"1"}]]
    Shrunk 14 time(s)

Property failed after 2 tests:
The test fails on second run.
{ seed: 2045942474, path: "1:0:1:4:0:1:0:1:0:2:0:1:1:0:0", endOnFailure: true }:
If you add this payload at the end of the fc.assert(fc.asyncProperty(...), __payload__), fast-check will replay the exact same issue and nothing more. [more on this subject]
Counterexample
What caused the property to fail. Which inputs. I'm working on something to have a ready to copy failing case.
Counterexample looks like an array. Each item correspond to one generator. In other words, the failure happened with:

  - s generated by fc.scheduler() = `Scheduler...`
  - initialText = ""
  - textUpdates = [{"priority":1,"text":"1"}]

Scheduler...: How to read it?
Each task corresponds to an asynchronous operation that have been scheduled using the scheduler built by the framework. Tasks are ordered by the order in which they resolved during the test. The number next to the task (see [task#1]) indicates the order in which tasks have been registered to the scheduler.
Resolved tasks will be displayed with resolved, rejected once with rejected and pending ones with pending. The scheduler will always show all the tasks.

Given that the error above can be seen as:

We render App with initialText = ""
Before the task for text="" resolves we call setText("1") with a priority of 1 - see sequence::Scheduling "1" with priority 1 resolved
Task for text="" resolves - see promise::Request for "" resolved

And no other tasks have been received (ie no task for text="1")

dubzzz · 2020-04-24T08:18:13Z

@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.

packages/react-reconciler/src/__tests__/ReactSuspense-test.property.js

dubzzz · 2020-06-13T18:28:27Z

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:

Property failed after 2 tests
{ seed: -1473093362, path: "1:0", endOnFailure: true }
Counterexample: [schedulerFor()`
-> [task${2}] sequence::Call setText("6") with priority 1 resolved
-> [task${1}] promise::Resolve async request for text "" resolved`,"",[{"priority":1,"text":"6"}]]
Shrunk 1 time(s)
Got error: Error: expect(received).toEqual(expected) // deep equality

- Expected  - 1
+ Received  + 1

  <span>
-   6
+   
  </span>

Stack trace: Error: expect(received).toEqual(expected) // deep equality

- Expected  - 1
+ Received  + 1

  <span>
-   6
+   
  </span>

  156 |                 ? textUpdates[textUpdates.length - 1].text
  157 |                 : initialText;
> 158 |             expect(ReactNoop).toMatchRenderedOutput(<span>{lastText}</span>);
      |                         ^
  159 |           },
  160 |         )
  161 |         .beforeEach(beforeEachAction),

  at packages/react-reconciler/src/__tests__/ReactSuspense-test.property.js:158:25

  Hint: Enable verbose mode in order to have the list of all failing values encountered during the run

  Reproduction link: https://codesandbox.io/api/v1/sandboxes/define?parameters=N4IgZglgNgpgziAXKADgQwMYGs0HMYB0AVnAPYB2SoGFALjObUiMADrkAEHrIAJjCgb9yGCPB6IObTl24gATjEy0JcgAwENagLQwAHoPkQAtg1poo2gMxWMVtGABMADgAsPADTtZcxcu28pMaqPFqaugYwRqaMFta29k5uPN4cAL7saSBpHgoCpPK0BDQAroxR-mjGKLAEtHpMyCA05YzM7MoQFHAcALwcANqp0j48cOa0JXAheWRQAG4wvJ6pXGMYABZLJVAQ5LgAKgCegjNwMACOJQwYMCsyayDmcFgAkstIHI5eD3JQaAAjGBQGYAYQsUA451oB30tAAFKwkSAAGxIngASg4AHcILQNhwUEYCnijhwAIz3UYgUzmXhocyqEY-OS0E53T5jGAwuFUlk8IldIxs1Tkn4s1m8zmolK_DIyHLDVZycYMqYzRRzRYfcWydZbXg7PaHdkzIlBCDnPmPZ5vD6SMXKnj_IEg6UAJXgpAWMA4aDgRxEHEUV3gtA4YAKHHoDW4yPRIGtclpaHpjM-zOpbNO0pD13GSZtUskKUTctS8oAugBudh7PEQCyw2P9Uu18oNACqKDT8D6gyVvwFxOFR1FuqLDRmaLLXCr7Yli442VyabQxAQTRa9DaiDLJhQBXDwA4ZVVANgAH15GUAOp4jYABRHpPSEfkQRVm22sHkPHb7D6IehQcC04wcPWtCNlAzbhq2iYgAB5CwOGl7QrB7ZAUeoHdOG_CQOQMAAMrcrB_bwuhcJYr0AB8HDwmhpFwv2lENBiSFgGUGBQRQoF-PQnpkCU8i3PC5BVDAWLMihwZej6vD9mAFjnAuHAyZq3qLKpMnmsYlq-v0hHYhwz4Wuc8LyH0dEWXJiz9vIGLsakiiTPInCZhw-DhuJpjwlJypcBAYD0QAhBp8lYviH7Gbp-mqT4LnCZwPkwPF6QTrJWowH5UgBZlmlLP2tA3qleXhYsflpfKc7tvK7CcSIPGcOVMCCaQwmiSl_kyGB3qEFApC4PCAAGbULMa-Udb6kaWQAJMAABSREAPIAHIEOMRj7EFRxiRJGJpMNTkyBpU1wAAsmg4n4PIBBeXtpgYgQLWVZk7D1VxTWnj2DLERM2WCiSbIeBwXW5T13R9QQA1DcN4JQLs-xQkxsa4viHDzUta0bcVxo7Q9kmHcdXBnuYF4wNed4Ps-QqkvCgOjiDOW0RwjE8g0BOObV72EQY2FgeGp0ifAl3XVE_ZGRwl0oK95BC7cF1XXgUQbdyiIyiAIMYPxrVelN6szlz7DyyLSs3arCKllrOttfrpZG-Q7DoSYMDtQizN0SUP30ER_3wmKciGyD5JqKHx3O6Ybvwh7-U-rbwvq5iIOOKHajE0uLIriAez8HoG5UM0dBmO05AHthnrKCDJ5TH9v1vmAH7GL4Sjcf-dbVOXLe0AAIst53vp-PB-NxARBG3pcdyBJ74XsxEo7QIOQdBsEgybiti5ZaQD03PAEAA9Gu48fY1XScAAgigMvdVwAuDDGC_I-ztCVv2Ne-798JL02VGqTPhEkU_Ci89jqpF6rUGGI1PTkH4FtXAHAL4oAjFGe-vRMYrXWptPGYBdr3wOkdVSt9NT2T1sLdeytbr3VwapRKbkOAAB44DoHIDRYAmoCApTSHQvejCro0WrBwPee9MrsIktGDY0Ueh7FAv6X0-JfR5jDIVNcEEejkFIOGNA8w0DQEBLAN6jsRC4WDKQdRABRWAMQ4IcECBgEoli7rcnMTASxAAhI47x1YfnUZidsFduK93OgQUmuiKbayUAJExlsTqRKcZY9gT1FDQKiIiGQdC_FFCIlMQQ5BzgRghACTAWBejADoRsckNEAAypBUwTTkX6S-BBGlcLKTRNINFlR0IQQI9pqS97pIIJkxhDBzg9PThnWQWdFDASKPURo1Ai67hAKZQwbI8nQEKg4egllHDRjDNMcgJ5zhLEkNockrgADsVg1AAE4bAom-ISBkGwSwgHJIgNQngOBCGWuQAAYtoqAwkYCSGKtcdI7BQTtXKIoPQVQajAsGHAb8hpfy_IKH5Ya7BtB0QGLaeajg0gv3OKGEQwLEDw0hKxS2GssRowJAzV85JY7aixTivFwBySEsJI3fSiBEBjTsv6QMGBZKhnAjNXZsZSzMqWMNLwCEPADDYCABlIokCOieMWHgM5CWVnYERcRZQsAUmjC7CiGJ2AAHF1FfPkB-eQkhTF2oKCWEoodyQAgGI4UwkRuKIjdWoD1XrvWsADUGqw5JjCKFuBAFlYbPVWGucEeNXrjBPVDe6z1jhvW0FIKYq4FgM2BqzcYf1mbg3JvLVYb1QEYDcSWEW8NSbG0lqxII6xMABBfILbsEUKbs3BAMS2gY1bjDaA4KY319AFIcHHZSFNibB3DojcYAA1BwT0MbtRcHXfOqtzah39qbvQnhzDh0Dvegu7146uBogXQe5dkb11rHvUuo9XAuGnvaf2kNBj37YGjPITACKnX2tdeWmtU6y3ForY-qNdaYCxoba-89aaCDnpzXmntqHoNBovVen1gh63LBQ0ettQj-BdsuCUCwpIMNLsdgR8dk6iPTq4HO5dD6F1Po3QhpDM7d2cYY6hj9X76OXqrde2Qd791vqrTxl9smJMwe9R-7hTDv0Qd_T4BkHBQSICRPweYSJh4qFYOgbAytpisFM9oaNFBRC_iRHAESSJLyXnoOMOA7mkTpMGdk842hPNFHNCso4G5EDkgAKzOEQI4KLPMAASexaCOvEuTDgix5AAlILk4wpB-AQU4AUGB0ZSAcA2Jo2RWw1KWnDKQYKEI1mIzgVowFfYbhQq2Uo4StSas3koGkIbQA

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

dubzzz · 2020-07-16T22:54:20Z

What do you think of this new version?

dubzzz · 2020-08-01T19:22:02Z

Is there any updates? Maybe worth closing this PR if it does not add value to the test suite of React?

dubzzz · 2020-10-28T17:13:48Z

@gaearon @acdlite Before rebasing my PR, I just wanted to make sure that it could be useful. If you believe there is no need for such tests, then maybe I should just close the ticket

gaearon · 2020-10-28T17:24:17Z

Sorry, we're pretty behind on reviews in general. Updating a PR is always helpful.

Quick thoughts:

Thanks for adding a way to reproduce.
How much is the framework buying us here? If we were to write the same thing manually, how many lines would it be?
How many bugs has this uncovered? We can't really merge the test while it exposes bugs since we can't fail the CI. So it seems like fixing the bugs or at least cataloguing them would be a prerequisite to merging.

dubzzz · 2020-10-28T20:05:06Z

Thanks a lot for your quick response. I'll work on a proper answer after the rebase

stale · 2022-01-09T07:41:46Z

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.

facebook-github-bot added the CLA Signed label Apr 23, 2020

dubzzz added 2 commits April 24, 2020 10:15

Quick and dirty nicer output draft

0e0ac2f

Remove unneeded useEffect

238114b

Merge remote-tracking branch 'upstream/master' into fuzz-scenario-a

12a413c

dubzzz mentioned this pull request May 15, 2020

Easier replay of failing scheduler dubzzz/fast-check#595

Closed

dubzzz added 2 commits June 13, 2020 20:16

Better error reporting

78861fd

Merge remote-tracking branch 'upstream/master' into fuzz-scenario-a

7128522

dubzzz commented Jun 13, 2020

View reviewed changes

packages/react-reconciler/src/__tests__/ReactSuspense-test.property.js Show resolved Hide resolved

stale bot added the Resolution: Stale Automatically closed due to inactivity label Jan 9, 2022

sebmarkbage deleted the branch facebook:master October 20, 2022 20:41

sebmarkbage closed this Oct 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug/Test: Add fuzzing tests for React.Suspense #18721

Bug/Test: Add fuzzing tests for React.Suspense #18721

dubzzz commented Apr 23, 2020

codesandbox-ci bot commented Apr 23, 2020 •

edited

sizebot commented Apr 23, 2020 •

edited

sizebot commented Apr 23, 2020 •

edited

gaearon commented Apr 23, 2020

gaearon commented Apr 23, 2020

acdlite commented Apr 23, 2020 •

edited

acdlite commented Apr 23, 2020

acdlite commented Apr 23, 2020 •

edited

dubzzz commented Apr 24, 2020

dubzzz commented Apr 24, 2020

dubzzz commented Apr 24, 2020

dubzzz commented Jun 13, 2020 •

edited

dubzzz commented Jul 16, 2020

dubzzz commented Aug 1, 2020

dubzzz commented Oct 28, 2020

gaearon commented Oct 28, 2020

dubzzz commented Oct 28, 2020

stale bot commented Jan 9, 2022

Bug/Test: Add fuzzing tests for React.Suspense #18721

Bug/Test: Add fuzzing tests for React.Suspense #18721

Conversation

dubzzz commented Apr 23, 2020

Summary

Test Plan

codesandbox-ci bot commented Apr 23, 2020 • edited

sizebot commented Apr 23, 2020 • edited

Size changes (stable)

sizebot commented Apr 23, 2020 • edited

Size changes (experimental)

gaearon commented Apr 23, 2020

gaearon commented Apr 23, 2020

acdlite commented Apr 23, 2020 • edited

acdlite commented Apr 23, 2020

acdlite commented Apr 23, 2020 • edited

dubzzz commented Apr 24, 2020

dubzzz commented Apr 24, 2020

dubzzz commented Apr 24, 2020

dubzzz commented Jun 13, 2020 • edited

dubzzz commented Jul 16, 2020

dubzzz commented Aug 1, 2020

dubzzz commented Oct 28, 2020

gaearon commented Oct 28, 2020

dubzzz commented Oct 28, 2020

stale bot commented Jan 9, 2022

codesandbox-ci bot commented Apr 23, 2020 •

edited

sizebot commented Apr 23, 2020 •

edited

sizebot commented Apr 23, 2020 •

edited

acdlite commented Apr 23, 2020 •

edited

acdlite commented Apr 23, 2020 •

edited

dubzzz commented Jun 13, 2020 •

edited