-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
test(cypress): Fail Cypress on Console errors #24872
Conversation
Codecov Report
@@ Coverage Diff @@
## master #24872 +/- ##
==========================================
- Coverage 69.00% 68.99% -0.01%
==========================================
Files 1906 1906
Lines 74135 74135
Branches 8207 8207
==========================================
- Hits 51154 51151 -3
- Misses 20860 20863 +3
Partials 2121 2121
Flags with carried forward coverage won't be shown. Click here to find out more. see 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
This adds the plugin to block all console errors, and then adds configuration to let them slide. In follow-up PRs, we can start disabling these configs, one by one, and whittle away at the pile. CC @michael-s-molina for a review, since I think this is where we discussed letting the initial PR sit, and open the door to actual cleanup. |
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.
Thank you @rusackas! This will really help improve the quality of our codebase.
/\[webpack-dev-server\]/, | ||
'The pseudo class ":first-child" is potentially unsafe when doing server-side rendering. Try changing it to ":first-of-type".', | ||
'The pseudo class ":nth-child" is potentially unsafe when doing server-side rendering. Try changing it to ":nth-of-type".', | ||
/Warning: /, |
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.
Is it possible to make this regex more restrictive and add specific warning types? My concern is that any new warning will be ignored.
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.
Yes... that was going to be my next PR. Just trying to get the framework through. I can add it here, though.
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.
LGTM. Thanks for working on this @rusackas!
@jinghua-qa @geido @eschutho @betodealmeida can I get a codeowner review on this? Hoping to merge it and then start backing out the exceptions one by one in subsequent PRs. |
SUMMARY
A lot of console errors have been creeping into the repo, and failing silently. These really only get noticed by random devs/users now when clicking around.
This PR won't systemically resolve these, but it will fail whey Cypress randomly encounter them during E2E tests.
This PR now swallows all console errors that would otherwise fail CI with this plugin installed. This is a new pile of tech debt. Now that we have disables failure on "preexisiting conditions" in this PR, we can stop the addition of further error types (particularly ones that sneak in with package bumps.
The rest of these should be addressed in subsequent PRs, to systemically get rid of these errors, or at least fine-tune the overrides from this PR to be more specific (in individual files/tests, etc).
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
E2E now passes, and you can see all the errors this swallows to do so.
ADDITIONAL INFORMATION