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

feat: error when updating a 9.X value in 10.X in the pluginsFile #20521

Merged
merged 119 commits into from
Mar 24, 2022

Conversation

elevatebart
Copy link
Contributor

@elevatebart elevatebart commented Mar 7, 2022

NOTE: this PR has been split in 2. This one only add the renamed config options to the array and makes sure the config is validated after setupNodeEvents.
The other one builds contextualization linked in the video below.

User facing changelog

In setupNodeEvents(), when updating options of 9.X that are not in 10.X, users get an error like below

Screen Shot 2022-03-10 at 10 46 05 AM

Additional details

trimmed.mov

The wrapper

To achieve the behavior above, I set all breaking options setters to error.
The following code will throw an error.

setupNodeEvents(on, config) {
  config.componentFolder = 'src'
}

This way, cypress gives users the line and codeframe where the invalid config set is done. This helps avoid the pain of searching for the file and line where the wrong "set" is done.

Missing breaking values

This PR adds integrationFolderand ignoreTestFiles to the breakingOptions list of configs that worked in 9 and are ignored in 10. To make ignoreTestFiles easier, I refactored the testFiles error slightly by using the newName property.

I also added to all the existing errors the management of the stack trace.

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Mar 7, 2022

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented Mar 7, 2022



Test summary

17788 0 217 0Flakiness 0


Run details

Project cypress
Status Passed
Commit 4c71ef6
Started Mar 24, 2022 3:47 PM
Ended Mar 24, 2022 3:59 PM
Duration 12:21 💡
OS Linux Debian - 10.10
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

@elevatebart elevatebart changed the title Elevatebart/feat/error-when-non-migrated-value feat: error when updating a 9.X value in 10.X in the pluginsFile Mar 7, 2022
@elevatebart elevatebart requested review from brian-mann, chrisbreiding, ZachJW34 and estrada9166 and removed request for ZachJW34 March 8, 2022 16:30
@elevatebart elevatebart marked this pull request as ready for review March 8, 2022 17:56
@elevatebart elevatebart requested review from a team as code owners March 23, 2022 22:44
cy.visitLaunchpad()
cy.findByText('E2E Testing').click()
cy.get('h1').should('contain', 'Error Loading Config')
cy.percySnapshot()
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally like to assert the actual error using a cy.get(...).contains(...). Not a blocker, but I think those are more clear and easier to debug than Percy snapshots.

Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

Seems fine, left one non blocking comment on the test (don't like Percy alone, I like the more explicit assertions) but still going to give it a ✔️ .

To confirm - if you delete the property in your video and save the file, the config file re-executes and everything works as expected and you can proceed?

@jennifer-shehane jennifer-shehane removed the request for review from a team March 24, 2022 15:00
@elevatebart elevatebart merged commit 1de1aa5 into 10.0-release Mar 24, 2022
@elevatebart elevatebart deleted the elevatebart/feat/error-when-non-migrated-value branch March 24, 2022 16:52
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.

4 participants