-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
fix: log error on reject with string content #25059
fix: log error on reject with string content #25059
Conversation
Thanks for taking the time to open a PR!
|
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 the contribution! The tests you added look good, but could you also add a test in this file? That will give us better e2e coverage of this.
Those tests are a little unusual because they run Cypress within Cypress. The tests in here are the outer tests that verify the results of the inner tests. This file has the inner tests and looks more like a standard Cypress spec. You can follow the pattern of the spec unhandled rejection
test. The tests can be run locally by running yarn cypress:open
within packages/app
.
The only trick to writing the test is that the first argument passed to verify in the outer test (e.g. verify('spec unhandled rejection', { ... })
) needs to match the name of the test in the inner test (e.g. it('spec unhandled rejection', ...)
).
@chrisbreiding tried to add the test you mentioned, bumped into two issues which I need help on:
Not sure where the logic of that is, maybe we need to add this transformation to a higher level in the code? |
Sorry about that. Seems to be an oversight with our eslint configuration. I should have that fixed in #25089, so once it's merged, you can update from origin/develop and it will unblock you.
Great. Looks like the test is doing its job because we do want the error to show up there as well. You're right about needing to move the transformation higher. I think this is where we should do it. If it ensures that the |
The eslint issue is fixed in |
bc28d7f
to
5f2e52c
Compare
@chrisbreiding e2e test up, now failing because of the lack of stack strace on the new error log (if I get it right) |
Co-authored-by: Chris Breiding <chrisbreiding@users.noreply.github.com>
(uncaught exception) undefined: undefined
when promise rejects with a string #24915User facing changelog
Fixes issue where unhadled promise rejectection was being logged as
(uncaught exception) undefined: undefined
Additional details
Change was needed to help debugging failures where user has only access to cy console output.
Steps to test
How has the user experience changed?
PR Tasks
cypress-documentation
?type definitions
?