Skip to content
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

Update test comments with explanations #21857

Merged
merged 1 commit into from Jul 19, 2021

Conversation

@rickhanlonii
Copy link
Member

@rickhanlonii rickhanlonii commented Jul 12, 2021

I understand this well enough to explain these now.

@sizebot
Copy link

@sizebot sizebot commented Jul 12, 2021

Comparing: 87b67d3...02656e8

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 126.85 kB 126.85 kB = 40.63 kB 40.63 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 129.67 kB 129.67 kB = 41.60 kB 41.60 kB
facebook-www/ReactDOM-prod.classic.js = 403.85 kB 403.85 kB = 74.70 kB 74.70 kB
facebook-www/ReactDOM-prod.modern.js = 392.55 kB 392.55 kB = 72.97 kB 72.97 kB
facebook-www/ReactDOMForked-prod.classic.js = 403.85 kB 403.85 kB = 74.71 kB 74.70 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 02656e8

@rickhanlonii rickhanlonii force-pushed the rickhanlonii:rh-update-comments branch from 5cc755c to bff5fe5 Jul 13, 2021
expect(Scheduler).toFlushAndYieldThrough([
'render: 2',
'componentDidUpdate: 2',
'componentDidUpdate (before setState): 2',
'componentDidUpdate (after setState): 2',
// This renders because the scheduled update is default, which is sync.

This comment has been minimized.

@acdlite

acdlite Jul 13, 2021
Member

This isn't right, the "3" update is deferred (deferredUpdates is basically just startTransition, we should remove it)

The reason it appears in this assertion is that toFlushAndYieldThrough will keep rendering until the logs match, and only fails if the expected items are different or if it's unable to yield after the last item.

You should be able to remove the gate condition on line 285. if you wrap the setState({tick: 2}) in startTransition, i.e. this behavior should work in both branches:

    // Increment the tick to 2. This will trigger an update inside cDU. Flush
    // the first update without flushing the second one.
    React.startTransition(() => {
      instance.setState({tick: 2});
    });

    expect(Scheduler).toFlushAndYieldThrough([
      'render: 2',
      'componentDidUpdate: 2',
      'componentDidUpdate (before setState): 2',
      'componentDidUpdate (after setState): 2',
    ]);
    expect(ReactNoop).toMatchRenderedOutput(<span prop={2} />);

    // Now flush the cDU update.
    expect(Scheduler).toFlushAndYield(['render: 3', 'componentDidUpdate: 3']);
    expect(ReactNoop).toMatchRenderedOutput(<span prop={3} />);

This comment has been minimized.

@acdlite

acdlite Jul 13, 2021
Member

Could also use toFlushUntilNextPaint instead

This comment has been minimized.

@rickhanlonii

rickhanlonii Jul 15, 2021
Author Member

Ah, right nice catch.

@rickhanlonii rickhanlonii force-pushed the rickhanlonii:rh-update-comments branch from bff5fe5 to 0396149 Jul 15, 2021
@rickhanlonii rickhanlonii force-pushed the rickhanlonii:rh-update-comments branch from 0396149 to 02656e8 Jul 15, 2021
@rickhanlonii rickhanlonii requested a review from acdlite Jul 15, 2021
@rickhanlonii rickhanlonii merged commit 5579f1d into facebook:main Jul 19, 2021
33 of 34 checks passed
33 of 34 checks passed
@codesandbox
ci/codesandbox Building packages failed.
Details
@facebook-github-tools
Facebook CLA Check Contributor License Agreement is valid!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_build Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_dom_fixtures Your tests passed on CircleCI!
Details
ci/circleci: build_devtools_and_process_artifacts Your tests passed on CircleCI!
Details
ci/circleci: build_devtools_scheduling_profiler Your tests passed on CircleCI!
Details
ci/circleci: get_base_build Your tests passed on CircleCI!
Details
ci/circleci: process_artifacts_combined Your tests passed on CircleCI!
Details
ci/circleci: setup Your tests passed on CircleCI!
Details
ci/circleci: sizebot Your tests passed on CircleCI!
Details
ci/circleci: sync_reconciler_forks Your tests passed on CircleCI!
Details
ci/circleci: yarn_build Your tests passed on CircleCI!
Details
ci/circleci: yarn_build_combined Your tests passed on CircleCI!
Details
ci/circleci: yarn_flow Your tests passed on CircleCI!
Details
ci/circleci: yarn_lint Your tests passed on CircleCI!
Details
ci/circleci: yarn_lint_build Your tests passed on CircleCI!
Details
ci/circleci: yarn_test--r=experimental --env=development Your tests passed on CircleCI!
Details
ci/circleci: yarn_test--r=experimental --env=production Your tests passed on CircleCI!
Details
ci/circleci: yarn_test--r=stable --env=development Your tests passed on CircleCI!
Details
ci/circleci: yarn_test--r=stable --env=development --persistent Your tests passed on CircleCI!
Details
ci/circleci: yarn_test--r=stable --env=production Your tests passed on CircleCI!
Details
ci/circleci: yarn_test--r=www-classic --env=development --variant=false Your tests passed on CircleCI!
Details
ci/circleci: yarn_test--r=www-classic --env=development --variant=true Your tests passed on CircleCI!
Details
ci/circleci: yarn_test--r=www-classic --env=production --variant=false Your tests passed on CircleCI!
Details
ci/circleci: yarn_test--r=www-classic --env=production --variant=true Your tests passed on CircleCI!
Details
ci/circleci: yarn_test--r=www-modern --env=development --variant=false Your tests passed on CircleCI!
Details
ci/circleci: yarn_test--r=www-modern --env=development --variant=true Your tests passed on CircleCI!
Details
ci/circleci: yarn_test--r=www-modern --env=production --variant=false Your tests passed on CircleCI!
Details
ci/circleci: yarn_test--r=www-modern --env=production --variant=true Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_build---project=devtools -r=experimental Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_build--r=experimental --env=development Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_build--r=experimental --env=production Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_build--r=stable --env=development Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_build--r=stable --env=production Your tests passed on CircleCI!
Details
@rickhanlonii rickhanlonii deleted the rickhanlonii:rh-update-comments branch Jul 19, 2021
sthagen added a commit to sthagen/react that referenced this pull request Jul 19, 2021
Update test comments with explanations (facebook#21857)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants