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: it.skip no longer causes hooks to be assigned to the wrong test #8113

Merged
merged 9 commits into from
Aug 4, 2020

Conversation

panzarino
Copy link
Contributor

@panzarino panzarino commented Jul 29, 2020

User facing changelog

  • Fixes an issue where placing an it.skip on the last test before a nested suite with a before hook would cause the remainder of tests not to show up
  • Prevents error "Cannot set property 'err' of undefined" error occurs in place of other errors during automatic test-reruns on file save.

How has the user experience changed?

Given a test that looks like the following:

describe("UPPER", () => {
  it.skip("TEST1", () => {})

  describe("LOWER", () => {
    before(() => {
      cy.wait(1000);
    })

    it("TEST2", () => {})
  })
})

cypress open

Before:

Screen Shot 2020-07-29 at 1 02 54 PM

After:

Screen Shot 2020-07-29 at 1 02 10 PM

cypress run

Before:

Screen Shot 2020-07-29 at 1 06 01 PM

After:

Screen Shot 2020-07-29 at 1 06 51 PM

PR Tasks

  • Have tests been added/updated?
  • Has the original issue been tagged with a release in ZenHub?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jul 29, 2020

Thanks for taking the time to open a PR!

@sainthkh
Copy link
Contributor

Closes #8096 might accidentally close a valid PR.

@cypress
Copy link

cypress bot commented Jul 29, 2020



Test summary

7929 0 130 2


Run details

Project cypress
Status Passed
Commit a5ac913
Started Aug 3, 2020 5:46 PM
Ended Aug 3, 2020 5:53 PM
Duration 06:43 💡
OS Linux Debian - 10.1
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

@panzarino
Copy link
Contributor Author

@sainthkh Thanks for pointing that out, meant to reference 8086, sorry for the confusion.

@panzarino panzarino marked this pull request as ready for review July 29, 2020 17:09
Copy link
Member

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

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

Manually verified that this fixes the original issue.

packages/driver/src/cypress/runner.js Outdated Show resolved Hide resolved
@panzarino
Copy link
Contributor Author

Looks like when .skip is used (at least in this example), _runner.on('test', ... is not being fired, so setTest() is never called. This makes me wonder whether we should fix this so that setTest() sets the correct test or just remove all usages of setTest() and getTest(). @bkucera said he would look into it.

@kuceb
Copy link
Contributor

kuceb commented Aug 3, 2020

So the issue was setting hook.ctx.currentTest back to getTest() when we meant to delete hook.ctx.currentTest and find it from runnables list again, not sure how that wasn't caught. So i undid one of the deletions of getTest() and only left necessary 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
4 participants