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.

Sign up for GitHub

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

Jump to bottom

fix<compiler>: only call readTestFilter if the filter option is enabled #29720

Merged
merged 2 commits into from
Jun 3, 2024
Merged

fix<compiler>: only call readTestFilter if the filter option is enabled #29720

merged 2 commits into from
Jun 3, 2024

Conversation

TrickyPi
Copy link
Contributor

@TrickyPi TrickyPi commented Jun 3, 2024

Summary

Following the instructions in the compiler/docs/DEVELOPMENT_GUIDE.md, we are stuck on the command yarn snap --watch because it calls readTestFilter even though the filter option is not enabled.

How did you test this change?

have tested it on my local machine.

@vercel Vercel
Copy link

vercel bot commented Jun 3, 2024 •
edited
Loading

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 Jun 3, 2024 5:45am

@react-sizebot
Copy link

react-sizebot commented Jun 3, 2024 •
edited
Loading

Comparing: d77dd31...3194be6

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 +0.11% 1.82 kB 1.82 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 496.48 kB 496.48 kB = 88.87 kB 88.87 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.67 kB 6.67 kB +0.11% 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 501.30 kB 501.30 kB = 89.56 kB 89.56 kB
facebook-www/ReactDOM-prod.classic.js = 593.97 kB 593.97 kB = 104.48 kB 104.49 kB
facebook-www/ReactDOM-prod.modern.js = 570.35 kB 570.35 kB = 100.89 kB 100.89 kB
test_utils/ReactAllWarnings.js Deleted 63.65 kB 0.00 kB Deleted 15.90 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 63.65 kB 0.00 kB Deleted 15.90 kB 0.00 kB

Generated by 🚫 dangerJS against 3194be6

@josephsavona josephsavona merged commit 4082582 into facebook:main Jun 3, 2024
51 checks passed
@josephsavona
Copy link
Contributor

Thanks, great improvement!

github-actions bot pushed a commit that referenced this pull request Jun 3, 2024
@josephsavona
fix<compiler>: only call readTestFilter if the filter option is enabl…
2003e24 
…ed (#29720)

Following the instructions in the compiler/docs/DEVELOPMENT_GUIDE.md, we are stuck on the command `yarn snap --watch` because it calls readTestFilter even though the filter option is not enabled.

DiffTrain build for commit 4082582.
@TrickyPi TrickyPi deleted the xiaopi/tweak-snap branch June 4, 2024 05:34
@josephsavona
Copy link
Contributor

josephsavona commented Jun 4, 2024 •
edited
Loading

Ooops, this broke filter mode. I'm going to have to revert this.

@TrickyPi
Copy link
Contributor Author

TrickyPi commented Jun 5, 2024 •
edited
Loading

Uhh, could you share what problem is？

@josephsavona
Copy link
Contributor

Yeah, this broke filter mode. Running yarn snap —watch and then enabling filter mode by pressing “f” no longer filters tests after this change.

@TrickyPi
Copy link
Contributor Author

TrickyPi commented Jun 5, 2024 •
edited
Loading

Oh, I see! Maybe we can extend the subscribeKeyEvents function to fix this problem, what do you think about it?

image

@eps1lon eps1lon mentioned this pull request Jun 5, 2024
Merged
@josephsavona
Copy link
Contributor

@TrickyPi can you try?

@TrickyPi TrickyPi mentioned this pull request Jun 6, 2024
Merged
@TrickyPi
Copy link
Contributor Author

TrickyPi commented Jun 6, 2024 •
edited
Loading

I filed a PR #29775, we can see the result in the video snapshot.

josephsavona added a commit that referenced this pull request Jun 6, 2024
@josephsavona
compiler: revert #29720 to unbreak filter mode in snap
0197aca 
[ghstack-poisoned]
josephsavona added a commit that referenced this pull request Jun 6, 2024
@josephsavona
Update on "compiler: revert #29720 to unbreak filter mode in snap"
94e2e5e 
[ghstack-poisoned]
josephsavona pushed a commit that referenced this pull request Jun 6, 2024
@TrickyPi
fix<compiler>: reread the testfilter file if filter enabled during th…
70194be 
…e watch process (#29775)

Resolve #29720
In the above PR, I overlooked that we can change the filter mode during the watch process. Now it's fixed.
github-actions bot pushed a commit that referenced this pull request Jun 6, 2024
@josephsavona
fix<compiler>: reread the testfilter file if filter enabled during th…
5f62396 
…e watch process (#29775)

Resolve #29720
In the above PR, I overlooked that we can change the filter mode during the watch process. Now it's fixed.

DiffTrain build for commit 70194be.
josephsavona added a commit that referenced this pull request Jun 6, 2024
@josephsavona
Update on "compiler: revert #29720 to unbreak filter mode in snap"
88b9e8c 
[ghstack-poisoned]
josephsavona added a commit that referenced this pull request Jun 6, 2024
@josephsavona
Update base for Update on "compiler: revert #29720 to unbreak filter …
7f7d339 
…mode in snap"

[ghstack-poisoned]
josephsavona added a commit that referenced this pull request Jun 6, 2024
@josephsavona
Update on "compiler: revert #29720 to unbreak filter mode in snap"
59bc1b7 
[ghstack-poisoned]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
CLA Signed Component: Optimizing Compiler
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@TrickyPi @react-sizebot @josephsavona @facebook-github-bot