Add smoke test for flight fixture #28350

eps1lon · 2024-02-15T21:58:52Z

Not working in CI yet. Needs Node.js 20

Summary

The test just opens the page for now and asserts that no console errors or uncaught errors happens. This would've caught the breakage that was fixed in #28352

How did you test this change?

CI
yarn build && cd fixtures/flight && yarn && yarn build && yarn build && yarn test

react-sizebot · 2024-02-15T22:02:49Z

Comparing: 59fe6c3...0a7acc5

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	=	176.86 kB	176.86 kB	=	55.14 kB	55.14 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	179.00 kB	179.00 kB	=	55.78 kB	55.78 kB
facebook-www/ReactDOM-prod.classic.js	=	591.78 kB	591.78 kB	=	104.51 kB	104.51 kB
facebook-www/ReactDOM-prod.modern.js	=	575.07 kB	575.07 kB	=	101.51 kB	101.51 kB
test_utils/ReactAllWarnings.js	Deleted	66.50 kB	0.00 kB	Deleted	16.27 kB	0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
test_utils/ReactAllWarnings.js	Deleted	66.50 kB	0.00 kB	Deleted	16.27 kB	0.00 kB

Generated by 🚫 dangerJS against 0a7acc5

sebmarkbage · 2024-02-16T03:54:26Z

You can test it against
#28352
and
#28353

It should still have first one but pass after both.

There's still an issue that you have to run yarn build before yarn dev because of a babel cache poisoning issues.

eps1lon · 2024-02-16T14:03:28Z

#28353

It should still have first one but pass after both.

Not sure what #28353 fixes. Is there a specific interaction that's broken? From opening the fixture and clicking around I can't immediately see what's broken.

eps1lon · 2024-02-16T14:53:45Z

.circleci/config.yml

+          working_directory: fixtures/flight
+          command: |
+            yarn build
+            # Need to build twice for unknown reasons


Otherwise we get https://app.circleci.com/pipelines/github/facebook/react/50479/workflows/7727b75f-e654-4558-9294-a2a710b8ebbd/jobs/783757/steps

Which the fixture also catches

I believe if you set FAST_REFRESH=false, you can skip the build step completely. This is how I verified this locally: rm -rf build ./node_modules/.cache && FAST_REFRESH=false yarn test. My understanding is that this avoids needing to transform the node_modules/react-refresh ES modules (which seems to occur too late on the first run).

On the other hand, this would just mask the issue that users might run into when trying to run the flight fixture locally. Is fast refresh that useful for the fixture? If not, we could also disable it in fixtures/flight/config/env.js, and not only in the CI.

unstubbable · 2024-02-16T15:27:36Z

#28353
It should still have first one but pass after both.

Not sure what #28353 fixes. Is there a specific interaction that's broken? From opening the fixture and clicking around I can't immediately see what's broken.

Without the fix from #28353 I do get hydration mismatches because the chunk script tags (mentioned as "preloads" in the PR) are rendered outside of <html> in the server, and inside of <head> in the client.

<script src="/static/js/client2.chunk.js" async=""></script>
<script src="/static/js/client3.chunk.js" async=""></script>
<script src="/static/js/client8.chunk.js" async=""></script>
<script src="/static/js/client7.chunk.js" async=""></script>
<script src="/static/js/client6.chunk.js" async=""></script>
<script src="/static/js/client0.chunk.js" async=""></script>
<script src="/static/js/client4.chunk.js" async=""></script>
<script src="/static/js/client1.chunk.js" async=""></script>
<script src="/static/js/client9.chunk.js" async=""></script>
<!DOCTYPE html>
<html lang="en">
    <head>

eps1lon · 2024-02-17T14:17:07Z

Without the fix from #28353 I do get hydration mismatches

Could you clarify how this manifests other than having to manually inspect the produced HTML? I get no console warnings which you'd usually get for hydration mismatches.

unstubbable · 2024-02-17T15:47:28Z

Without the fix from #28353 I do get hydration mismatches

Could you clarify how this manifests other than having to manually inspect the produced HTML? I get no console warnings which you'd usually get for hydration mismatches.

That's strange. When I check out 4a54e8a (so before the fix), I get the expected console errors.

rm -rf build ./node_modules/.cache && FAST_REFRESH=false yarn dev:

rm -rf build ./node_modules/.cache && FAST_REFRESH=false yarn start:

unstubbable · 2024-02-17T16:28:00Z

For what it's worth, even with the console errors, and the "Hello World" expectation that was added later, I can't get the tests to fail consistently. Only when I add await page.reload(); after await page.goto('/'); it fails consistently (sometimes with two sets of errors though). Hopefully there's a better fix for this flakiness.

eps1lon · 2024-02-18T12:52:13Z

Can you post the exact instructions how you get the fixture running for you on a fresh clone?

unstubbable · 2024-02-18T14:41:16Z

With the following steps I got the fixture running on a fresh clone:

git clone https://github.com/facebook/react.git
cd react
yarn
RELEASE_CHANNEL=experimental node scripts/rollup/build.js --type=NODE_DEV,NODE_PROD,NODE_ES2015,ESM_PROD
(Because I'm not patient enough for a full build, and I don't know a better way to do this.)
cd fixtures/flight
yarn

adjust the flight package.json because of the custom build step:

diff --git a/fixtures/flight/package.json b/fixtures/flight/package.json
index 3d4574778..d33affd88 100644
--- a/fixtures/flight/package.json
+++ b/fixtures/flight/package.json
@@ -64 +64 @@
-    "predev": "cp -r ../../build/oss-experimental/* ./node_modules/",
+    "predev": "cp -r ../../build/node_modules/* ./node_modules/",

FAST_REFRESH=false yarn dev

And with that, I got a running flight fixture, strangely now without any hydration mismatches. 🙄
Please don't ask me what the difference is to my other clone where I do get the hydration mismatches. 🤷‍♂️

eps1lon · 2024-02-18T14:52:48Z

FAST_REFRESH=false yarn dev

I was missing that bit. Fast Refresh randomly starts working after a couple of failed yarn build && yarn dev.

unstubbable · 2024-02-18T15:16:51Z

And with that, I got a running flight fixture, strangely now without any hydration mismatches. 🙄 Please don't ask me what the difference is to my other clone where I do get the hydration mismatches. 🤷‍♂️

Ah, I didn't realise that #28353 was merged into main. That explains why the hydration mismatches are gone now.

unstubbable

For what it's worth, even with the console errors, and the "Hello World" expectation that was added later, I can't get the tests to fail consistently. Only when I add await page.reload(); after await page.goto('/'); it fails consistently (sometimes with two sets of errors though). Hopefully there's a better fix for this flakiness.

@eps1lon Since this is not addressed yet, I believe there will be false positive test results whenever a console error may appear due to some changes in the code base.

eps1lon · 2024-02-18T18:40:43Z

I believe there will be false positive test results whenever a console error may appear due to some changes in the code base.

The idea is that we prevent these from creeping in since that's an error. console.errors are not false positives. If they are expected, they can be added to the assertion.

I'll try the page.reload(). Thank your for the tip

unstubbable · 2024-02-18T18:46:53Z

I believe there will be false positive test results whenever a console error may appear due to some changes in the code base.

The idea is that we prevent these from creeping in since that's an error. console.errors are not false positives. If they are expected, they can be added to the assertion.

I meant "false positive test results" as in there might be a successful test despite console errors occurring, due to the test exiting before the calls could have been collected.

unstubbable · 2024-02-18T19:06:57Z

For what it's worth, even with the console errors, and the "Hello World" expectation that was added later, I can't get the tests to fail consistently. Only when I add await page.reload(); after await page.goto('/'); it fails consistently (sometimes with two sets of errors though). Hopefully there's a better fix for this flakiness.

@eps1lon Since this is not addressed yet, I believe there will be false positive test results whenever a console error may appear due to some changes in the code base.

I tested the latest version of this PR again, but without d37ca84, and now I do get consistent test failures, as expected. So maybe there's no need to change the test, after all.

eps1lon · 2024-02-21T22:46:48Z

Didn't detect any flakiness while running locally so let's see how CI treats it.

DiffTrain build for commit 81d2a51.

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Feb 15, 2024

eps1lon force-pushed the test/fixture-flight branch from 220c0f7 to bf65cbe Compare February 15, 2024 22:06

eps1lon mentioned this pull request Feb 16, 2024

Add regression test to attribute-behavior fixture #28349

Open

2 tasks

eps1lon force-pushed the test/fixture-flight branch 2 times, most recently from 418f4da to 51401ff Compare February 16, 2024 10:45

eps1lon commented Feb 16, 2024

View reviewed changes

unstubbable mentioned this pull request Feb 16, 2024

Disable fast-refresh in flight fixture tests eps1lon/react#9

Closed

eps1lon force-pushed the test/fixture-flight branch from 1c182d4 to d036d62 Compare February 17, 2024 14:19

eps1lon marked this pull request as ready for review February 18, 2024 15:09

unstubbable reviewed Feb 18, 2024

View reviewed changes

sebmarkbage approved these changes Feb 21, 2024

View reviewed changes

eps1lon and others added 4 commits February 21, 2024 23:34

Add smoke test to flight fixture

e110488

Debug webserver in CI

561495b

Fix CI value

be08cc5

Better webserver debugging

9aa8c4e

Sebastian Silbermann added 7 commits February 21, 2024 23:34

Ensure dependency constraints are fulfilled

3a94495

yarn build is required

99466f8

Add hint for require Node.js version

bdbbce8

Ensure correct Node.js version is used

0320dd3

Ensure build completed

76fc01c

Fix build

0bf35c3

Fix bundle error

0a7acc5

eps1lon force-pushed the test/fixture-flight branch from 03660c7 to 0a7acc5 Compare February 21, 2024 22:34

eps1lon changed the title ~~Add smoke test to flight fixture~~ Add smoke test for flight fixture Feb 21, 2024

eps1lon merged commit 81d2a51 into facebook:main Feb 21, 2024
37 checks passed

eps1lon deleted the test/fixture-flight branch February 21, 2024 22:47

huozhi mentioned this pull request Feb 26, 2024

Upgrade vendored react vercel/next.js#62549

Merged

ztanner mentioned this pull request Mar 19, 2024

(backport) Update React from 60a927d04 to 14898b6a9 vercel/next.js#63442

Closed

EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024

Add smoke test for flight fixture (facebook#28350)

3357bea

bigfootjon pushed a commit that referenced this pull request Apr 18, 2024

Add smoke test for flight fixture (#28350)

4683bd9

DiffTrain build for commit 81d2a51.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add smoke test for flight fixture #28350

Add smoke test for flight fixture #28350

eps1lon commented Feb 15, 2024 •

edited

Loading

react-sizebot commented Feb 15, 2024 •

edited

Loading

sebmarkbage commented Feb 16, 2024

eps1lon commented Feb 16, 2024

eps1lon Feb 16, 2024

unstubbable Feb 16, 2024 •

edited

Loading

unstubbable Feb 16, 2024 •

edited

Loading

unstubbable commented Feb 16, 2024 •

edited

Loading

eps1lon commented Feb 17, 2024

unstubbable commented Feb 17, 2024 •

edited

Loading

unstubbable commented Feb 17, 2024

eps1lon commented Feb 18, 2024

unstubbable commented Feb 18, 2024 •

edited

Loading

eps1lon commented Feb 18, 2024 •

edited

Loading

unstubbable commented Feb 18, 2024 •

edited

Loading

unstubbable left a comment

eps1lon commented Feb 18, 2024

unstubbable commented Feb 18, 2024

unstubbable commented Feb 18, 2024 •

edited

Loading

eps1lon commented Feb 21, 2024

Add smoke test for flight fixture #28350

Add smoke test for flight fixture #28350

Conversation

eps1lon commented Feb 15, 2024 • edited Loading

Summary

How did you test this change?

react-sizebot commented Feb 15, 2024 • edited Loading

Critical size changes

Significant size changes

sebmarkbage commented Feb 16, 2024

eps1lon commented Feb 16, 2024

eps1lon Feb 16, 2024

Choose a reason for hiding this comment

unstubbable Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

unstubbable Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

unstubbable commented Feb 16, 2024 • edited Loading

eps1lon commented Feb 17, 2024

unstubbable commented Feb 17, 2024 • edited Loading

unstubbable commented Feb 17, 2024

eps1lon commented Feb 18, 2024

unstubbable commented Feb 18, 2024 • edited Loading

eps1lon commented Feb 18, 2024 • edited Loading

unstubbable commented Feb 18, 2024 • edited Loading

unstubbable left a comment

Choose a reason for hiding this comment

eps1lon commented Feb 18, 2024

unstubbable commented Feb 18, 2024

unstubbable commented Feb 18, 2024 • edited Loading

eps1lon commented Feb 21, 2024

eps1lon commented Feb 15, 2024 •

edited

Loading

react-sizebot commented Feb 15, 2024 •

edited

Loading

unstubbable Feb 16, 2024 •

edited

Loading

unstubbable Feb 16, 2024 •

edited

Loading

unstubbable commented Feb 16, 2024 •

edited

Loading

unstubbable commented Feb 17, 2024 •

edited

Loading

unstubbable commented Feb 18, 2024 •

edited

Loading

eps1lon commented Feb 18, 2024 •

edited

Loading

unstubbable commented Feb 18, 2024 •

edited

Loading

unstubbable commented Feb 18, 2024 •

edited

Loading