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

Stop using Scheduler.log to test double invocations #29008

Merged
merged 2 commits into from
May 20, 2024

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented May 6, 2024

Stack:

  1. Allow specifying timeout in tests via third argument #29006
  2. Stop using Scheduler.log to test double invocations  #29008 <-- You're here
  3. Dim console calls on additional Effect invocations due to StrictMode #29007

Summary

In a follow-up, I want to dim console calls for effect invocations due to StrictMode just like we do for render-time invocations due to StrictMode. This will automatically ignore Scheduler.log calls during effect invocations caused by StrictMode (just like it does for render-time invocations due to StrictMode).

That means we have to use other means of testing double invocations during StrictMode. I used the same pattern we used for render-time double invocations: pushing to a log.

Also used the opportunity to reduce the size of some log assertions to make future diffs readable. Git will not be able to correctly identify where a new log lines where added and think they got moved instead.

How did you test this change?

@react-sizebot
Copy link

react-sizebot commented May 6, 2024

Comparing: 6f90365...686bed8

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.js = 6.66 kB 6.66 kB = 1.82 kB 1.82 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 495.71 kB 495.71 kB = 88.78 kB 88.78 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.67 kB 6.67 kB = 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 500.51 kB 500.51 kB = 89.47 kB 89.47 kB
facebook-www/ReactDOM-prod.classic.js = 592.86 kB 592.86 kB = 104.28 kB 104.28 kB
facebook-www/ReactDOM-prod.modern.js = 569.08 kB 569.08 kB = 100.70 kB 100.70 kB
test_utils/ReactAllWarnings.js Deleted 64.35 kB 0.00 kB Deleted 16.05 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 64.35 kB 0.00 kB Deleted 16.05 kB 0.00 kB

Generated by 🚫 dangerJS against 686bed8

@eps1lon eps1lon marked this pull request as ready for review May 6, 2024 22:51
We don't do this to test double render invocations either since we ignore Scheduler.log for the second render.
The plan is to do the same for double invoking effects to reduce test noise.
The longer the list of log lines asserted one, the harder future diffs are to comprehend.
git-diff produces hard to decipher diffs if the StrictEffects behavior is changed slightly that results in more lines added.
It won't be obvious anymore which lines were added and which ones were removed.
@eps1lon eps1lon force-pushed the strictEffects/no-scheduler-log branch from 1d5ee2b to 686bed8 Compare May 20, 2024 21:46
Copy link

vercel bot commented May 20, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 20, 2024 9:48pm

@eps1lon eps1lon merged commit 55dd0b1 into facebook:main May 20, 2024
40 checks passed
@eps1lon eps1lon deleted the strictEffects/no-scheduler-log branch May 20, 2024 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants