-
Notifications
You must be signed in to change notification settings - Fork 46.9k
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
[tests] Disallow unasserted console.log #28708
Conversation
@@ -35,6 +35,9 @@ describe('ReactStrictMode', () => { | |||
}); | |||
|
|||
it('should appear in the client component stack', async () => { | |||
// TODO REMOVE AFTER TESTING THIS FAILS ON PR | |||
console.log('hit'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm expecting the test to fail just to confirm the CI envar is set properly, then I'll remove this.
Comparing: 95e6f03...b98343b Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also get a more uniform behavior and easier code if we always disallow console.log
, and just use console.debug
for local debugging.
Don't have a strong preference through. If tests pass and you remove the temporary code to test that this behaves correctly in CI, sounds good to me.
Yeah it would be super annoying to have to use console.debug in DEV |
Isn't |
Followup from facebook#28693 and facebook#28680. In CI, we fail the test for any unasserted console.log. In DEV, we don't fail, but you can still use the matchers and we'll assert on them.
Followup from #28693 and #28680.
In CI, we fail the test for any unasserted console.log. In DEV, we don't fail, but you can still use the matchers and we'll assert on them.