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

Clone setCookie options instead of mutating #2707

Merged
merged 7 commits into from
Nov 7, 2019
Merged

Clone setCookie options instead of mutating #2707

merged 7 commits into from
Nov 7, 2019

Conversation

lilaconlee
Copy link
Contributor

@brian-mann
Copy link
Member

So - this is totally all 👍 but as I mentioned to @jennifer-shehane this is not actually the first issue related to this problem - there are other issues open for this (that i'd have to dig out).

I remember one being about Object.freeze - and there are likely others. The better approach here would be to fix this problem in a single place so that all options received by the commands are actually already cloned - and that cloning them per command is not necessary.

I'll keep this PR open, but won't merge it until more discussion happens and if there's an easy way to fix these in 1 shot I'd prefer it.

@jennifer-shehane
Copy link
Member

I think that this should be merged in today. Regardless - I do agree with brian in this being a larger issue that should be opened and investigated.

@jennifer-shehane
Copy link
Member

Hey @brian-mann, can we merge this fix in?

Also, @lilaconlee, can you open an issue to address the larger issues brian referenced in his comment above?

@lilaconlee
Copy link
Contributor Author

lilaconlee commented Jan 18, 2019

I think if we wanted to merge this in, we'd want to reset it to a previous commit. I tried solving the problem earlier in the process as Brian suggested, but couldn't quite get it.

I'll reset to that commit and make the new issue.

Edit: Issue #3171 created for this

Copy link
Contributor

@flotwig flotwig left a comment

Choose a reason for hiding this comment

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

I think this is fine to go in today, and we can fix other instances of this as they crop up/someone can go through them all as described in #3171.

@flotwig flotwig merged commit 225bcd2 into develop Nov 7, 2019
@cypress
Copy link

cypress bot commented Nov 7, 2019



Test summary

3480 0 46 0


Run details

Project cypress
Status Passed
Commit c17a0f5
Started Nov 7, 2019 9:12 PM
Ended Nov 7, 2019 9:16 PM
Duration 04:06 💡
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

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.

cy.setCookie always sets the same cookie if you pass the same options object
4 participants