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: Revert cypress.json changes #15499

Merged
merged 3 commits into from
Mar 15, 2021
Merged

Conversation

chrisbreiding
Copy link
Contributor

@chrisbreiding chrisbreiding commented Mar 15, 2021

User facing changelog

  • Cypress no longer write extra config values to cypress.json when adding the project ID

Additional details

This was caused by a combination of using require to obtain the contents of cypress.json and mutating the config object itself. Since node caches required modules, it mutated the in-memory cache of the cypress.json, so subsequent 'reads' had the mutated content with extra values, which then got written back to the file when writing the project ID.

This reverts PRs #15444 and #15263.

PR Tasks

  • Have tests been added/updated?
  • Has the original issue or this PR been tagged with a release in ZenHub?
  • N/A Has a PR for user-facing changes been opened in cypress-documentation?
  • N/A Have API changes been updated in the type definitions?
  • N/A Have new configuration options been added to the cypress.schema.json?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Mar 15, 2021

Thanks for taking the time to open a PR!

Comment on lines +212 to +213
config: _.cloneDeep(settings),
envFile: _.cloneDeep(envFile),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only code changes besides the reverting.

Comment on lines +83 to +95
it('clones settings and env settings, so they are not mutated', function () {
const settings = { foo: 'bar' }
const envSettings = { baz: 'qux' }

this.setup(settings, envSettings)

return config.get(this.projectRoot)
.then(() => {
expect(settings).to.deep.equal({ foo: 'bar' })
expect(envSettings).to.deep.equal({ baz: 'qux' })
})
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New test to make sure cypress settings don't get mutated.

@kuceb
Copy link
Contributor

kuceb commented Mar 15, 2021

nice. @chrisbreiding do you think we should squash+rebase this into three commits (two reverts, one bugfix) and do a merge commit? it would be nice to have the explicit revert commits in develop

@chrisbreiding chrisbreiding requested a review from kuceb March 15, 2021 21:12
@cypress
Copy link

cypress bot commented Mar 15, 2021



Test summary

9388 0 119 3Flakiness 1


Run details

Project cypress
Status Passed
Commit 496e458
Started Mar 15, 2021 9:51 PM
Ended Mar 15, 2021 10:02 PM
Duration 11:30 💡
OS Linux Debian - 10.5
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

cypress/integration/retries.ui.spec.js Flakiness
1 runner/cypress retries.ui.spec > opens attempt on each attempt failure for the screenshot, and closes after test passes

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

@chrisbreiding chrisbreiding force-pushed the revert-cypress-json-changes branch from 43eb3d5 to 4886f23 Compare March 15, 2021 21:23
kuceb
kuceb previously approved these changes Mar 15, 2021
This was referenced Mar 16, 2021
@chrisbreiding chrisbreiding deleted the revert-cypress-json-changes branch April 5, 2022 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants