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: migrating from <10 to current with typescript #24675

Merged
merged 4 commits into from Nov 14, 2022

Conversation

ryanthemanuel
Copy link
Collaborator

@ryanthemanuel ryanthemanuel commented Nov 14, 2022

User facing changelog

Users can migrate from Cypress versions < 10 to 11 without hanging

Additional details

The prettier dependency that we use to format code in the 10 migration is not properly being rewritten during the snapshot doctoring process. I am forcing the portions of that dependency that we rely on to be force norewrite so that this no longer happens.

Steps to test

Migrate a project that uses typescript from 9 -> 11.

How has the user experience changed?

The app no longer hangs when migrating from 9 -> 11

PR Tasks

  • [na] Have tests been added/updated?
  • [na] Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • [na] Has a PR for user-facing changes been opened in cypress-documentation?
  • [na] Have API changes been updated in the type definitions?

@cypress-bot
Copy link

cypress-bot bot commented Nov 14, 2022

Thanks for taking the time to open a PR!

@ryanthemanuel ryanthemanuel marked this pull request as ready for review November 14, 2022 14:48
@emilyrohrbough
Copy link
Member

Would a system test have caught this error?

'node_modules/prettier/parser-flow.js',
'node_modules/prettier/parser-meriyah.js',
'node_modules/prettier/parser-typescript.js',
'node_modules/prettier/third-party.js',
Copy link
Member

Choose a reason for hiding this comment

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

how do we know this won't change under the hood with new versions of prettier or if we use new imports from prettier?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately, we don't have a mechanism to verify this today. cypress in cypress testing does not use snapshots due to how the child cypress server is being launched. This is something I want to investigate a solution to ASAP, but for now I think it's worth getting this fix in.

Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be manually verified at release then for each release?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we would only need to verify it if we update the version of prettier that is being used: https://github.com/cypress-io/cypress/blob/develop/packages/data-context/package.json#L45

@cypress
Copy link

cypress bot commented Nov 14, 2022



Test summary

548 8 42 64Flakiness 12


Run details

Project cypress
Status Failed
Commit 74310d0
Started Nov 14, 2022 7:09 PM
Ended Nov 14, 2022 7:24 PM
Duration 15:35 💡
OS Windows 10.0.17763
Browser Chrome 107

View run in Cypress Dashboard ➡️


Failures

Run group: app-e2e (Windows, Chrome )
cypress\e2e\runner\sessions.ui.cy.ts Failed
1 ... > setup has failing Cypress command
cypress\e2e\cypress-in-cypress.cy.ts Failed
1 Cypress in Cypress > handles automation disconnects in component
cypress\e2e\settings.cy.ts Failed
1 App: Settings > visits settings page
cypress\e2e\runner\reporter-ct-vite.uncaught-errors.cy.ts Failed
1 uncaught errors
cypress\e2e\cypress-origin-communicator.cy.ts Failed
1 Cypress In Cypress Origin Communicator > primary origin memory leak prevention > cleans up the primaryOriginCommunicator events when navigating away from the /specs to /runs
cypress\e2e\reporter_header.cy.ts Failed
1 Reporter Header > Specs Shortcut > selects the correct spec in the Specs List
cypress\e2e\specs_list_flaky.cy.ts Failed
1 App: Spec List - Flaky Indicator > shows the "Flaky" badge on specs considered flaky
Run group: launchpad-e2e (Windows, Chrome )
cypress\e2e\top-nav-launchpad.cy.ts Failed
1 ... > global mode > shows "continue" button after login if project selected

Flakiness

cypress\e2e\migration.cy.ts Flakiness
1 Full migration flow for each project > completes journey for migration-component-testing
cypress\e2e\specs.cy.ts Flakiness
1 ... > shows create first spec page with scaffold and create empty spec options
cypress\e2e\scaffold-component-testing.cy.ts Flakiness
1 scaffolding component testing > vuecli4vue2 > scaffolds component testing for Vue CLI 4 w/ Vue 2 project
2 scaffolding component testing > vuecli5vue3 > scaffolds component testing for Vue CLI 5 w/ Vue 3 project
3 scaffolding component testing > react-vite-ts-unconfigured > scaffolds component testing for React and Vite
This comment includes only the first 5 flaky tests. See all 12 flaky tests in the 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

@mjhenkes mjhenkes self-requested a review November 14, 2022 17:27
Copy link
Member

@mjhenkes mjhenkes left a comment

Choose a reason for hiding this comment

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

I'll approve this in the spirit of getting the fix out however it would be nice to have tests to insure that we don't regress here in the future.

@ryanthemanuel ryanthemanuel merged commit fbf2b0b into develop Nov 14, 2022
111 of 121 checks passed
@ryanthemanuel ryanthemanuel deleted the ryanm/fix/migrating-to-11-from-9-typescript branch November 14, 2022 20:03
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.

Migration in V11 not working have to go over 10
4 participants