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.

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: give proper error message when err.stack is undefined #5313

Merged
merged 9 commits into from
Nov 8, 2019

Conversation

kuceb
Copy link
Contributor

@kuceb kuceb commented Oct 8, 2019

User facing changelog

  • Certain failures related to Cypress commands no longer throw incorrect error 'cannot read property 'replace' of undefined'

Additional details

How has the user experience changed?

When running the following spec in chrome, you used to get incorrect error message, now you get correct one:

describe('issue-1669 undefined err.stack in beforeEach hook', () => {

  beforeEach(() => {
    cy.setCookie('foo', '   bar')
  })

  it('cy.setCookie should fail with correct error', () => {
    expect(true).ok
  })
})

image

PR Tasks

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Oct 8, 2019

Thanks for the contribution! Below are some guidelines Cypress uses when doing PR reviews.

  • Please write [WIP] in the title of your Pull Request if your PR is not ready for review - someone will review your PR as soon as the [WIP] is removed.
  • Please familiarize yourself with the PR Review Checklist and feel free to make updates on your PR based on these guidelines.

PR Review Checklist

Functionality

  • Does the code work? Does it perform its intended function, the logic is correct, etc?
  • Does the code guard against edge cases, invalid input, etc being entered? Are there tests for these?
  • Is any code invoking memory leaks? Has performance been factored in?

Maintainability

  • Is the code readable (too many nested 'if's are a bad sign.)
  • Do names used for variables, methods, etc, describe their function?
  • Is all the code easily understood? Are there relevant comments explaining?
  • Have algorithms been documented in the code? Is there a link to an external document? (flowcharts, w3c, chrome, firefox)

Quality

  • Is there a comment containing a link to the issue this is fixing (in tests and code)?
  • Does the change reimplement code that would be better served by pulling in a well known module from the ecosystem?
  • Is there any redundant or duplicate code?
  • Are tests actually testing the code’s intended functionality? Is there a more comprehensive test that could be written?
  • Are there any irrelevant comments left in the code?

User Experience

  • Is the feature/bugfix self-documenting from within the product (is there a way to explain this feature in product, so it doesn’t rely on resources outside, like docs)?
  • Are there any dead ends? Do we provide the end user with a way to fix their problem?

Internal

  • Has the original issue been tagged with a release in ZenHub?

@kuceb kuceb requested a review from brian-mann November 7, 2019 21:53
@cypress
Copy link

cypress bot commented Nov 7, 2019



Test summary

3488 0 46 0


Run details

Project cypress
Status Passed
Commit e7139ec
Started Nov 8, 2019 8:03 PM
Ended Nov 8, 2019 8:07 PM
Duration 03:44 💡
OS Linux Debian - 9.8
Browser Multiple

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@brian-mann
Copy link
Member

I will review this

@@ -0,0 +1,9 @@
describe('issue-1669 undefined err.stack in beforeEach hook', () => {
beforeEach(() => {
cy.setCookie('foo', ' bar')
Copy link
Contributor

@flotwig flotwig Nov 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bkucera if you merge in develop, I believe this was fixed to err correctly and no longer causes an err.stack === undefined, you may need to come up with a custom command to get the error you want

Also, just curious, is there a reason why is this an e2e test and not just a normal driver test that asserts on the error using cy.on('fail', (err) => {...})?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you catch the error in the 'fail' handler, it has not yet hit the logic that reads off .stack

I'll merge in develop, and try locally without these changes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, it did pass without the src changes, so I changed the test that now fails without these changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeError: Cannot read property 'replace' of undefined" at Object.appendErrMsg
3 participants