-
-
Notifications
You must be signed in to change notification settings - Fork 346
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
chore: deal with jest console warnings #3410
base: master
Are you sure you want to change the base?
chore: deal with jest console warnings #3410
Conversation
1 failed test on run #5361 ↗︎
Details:
src/integration/settings.spec.ts • 1 failed test • ci-chrome
Review all test suite changes for PR #3410 ↗︎ |
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.
Thanks for your patience waiting to hear back on this one. Does it solve all the console logging stuff?
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.
Disagree with this change. logger is doing what it should be here.
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.
Rather than mocking logger, the tests should be anticipating the call to logger happening.
https://stackoverflow.com/questions/49096093/how-do-i-test-a-jest-console-log
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.
Rather than mocking logger, the tests should be anticipating the call to logger happening.
https://stackoverflow.com/questions/49096093/how-do-i-test-a-jest-console-log
Hey @Robert-LC hope you're well, will you have a chance to have a look at this again soon? |
PR Checklist
PR Type
Description
I dealt with most of the Jest console warnings as well as the unnecessary logging appearing.
For
logger
itself, I expected the test log entries of debug, info, warn, fatal instead of displaying them.For
Message.store.tsx
I removed the logger.error calls because the test looks for Errors thrown so no need to cloud the logging, if the tests will fail in that regard.For files that called logger.info, I switched to logger.debug instead to hide the calls in the tests, but keep them around for debugging. Let me know if you think this what the right choice for those items.
There were a few tests like in
user.store.test.tsx
where it was testing for if two users were returned form the same id and it would return a warning, but if it wouldn't cause a failure of the test.So for those type of scenarios, I decided to mock the warning to hide from the test suite, since its not failing on expect. Let me know if you want something different in that scenario.
Lastly, while trying to figure out the best approach, I noticed that in
functions/src/test/firebase/logger.ts
there was at one point a top level mock that spied on all firebase logger methodsGit Issues
Closes #3303