fix: correctly migrate projects with custom integration folder #19929

lmiller1990 · 2022-01-27T09:43:36Z

resolves: https://cypress-io.atlassian.net/browse/UNIFY-989

One of the migration use cases was not handled correctly. We had an example project but didn't have the right assertions to verify it. Now we do.

The correct UI looks like this (screenshot from the test I added). Note it shows src/basic.spec.js correctly (src is the custom integration folder for this project).

We were not handling the case where the config is:

{
  "integrationFolder": "src"
}

We had a regexp hard-coded to use cypress/..., we should use their custom integration folder.

Also, we were including component specs in the E2E auto migration step - that's incorrect, so I fixed it. All of the fixes have tests - finally, the migration UI is getting in shape.

cypress-bot · 2022-01-27T09:43:37Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

cypress · 2022-01-27T10:00:28Z

Test summary

17933 • 0 • 216 • 0 • 0

Run details


Project	cypress
Status	Passed
Commit	`24596ea`
Started	Jan 28, 2022 2:47 PM
Ended	Jan 28, 2022 2:59 PM
Duration	12:17 💡
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

I have a few question before I feel comfortable approving this one.

elevatebart · 2022-01-27T16:13:41Z

packages/launchpad/cypress/e2e/migration.cy.ts

+  // TODO: the current logic is wrong and does not consider
+  // the case of a custom integration folder!


I see this test and I see it using a custom integrationFolder (src).
I see that files are renamed but left in the same folder.
This feels right.

Can you be more explicit? What logic is wrong? What do we need to consider?

Oh, the comment was from before (when it was wrong). This PR fixes the wrong. I will remove the comment.

elevatebart · 2022-01-27T16:16:31Z

packages/data-context/src/util/migrationFormat.ts

+      afterRegexp: getNewHighlightRegexp('e2e'),
+    },
+    usingCustomIntegrationFolder: {
+      beforeRegexp: (folder: string) => `${folder}\/.*?(?<ext>[._-]?[s|S]pec.|[.])(?=[j|t]s[x]?)`,


can folder contain any special characters? . or / could probably make it fail right?

I have no idea, this regexp has been here for a while. We almost certainly need more unit tests around it. This should be a separate PR, there's likely a ton of complexity to be uncovered when working on this regexp, I will follow it up.

I am thinking the regexp approach may not be ideal.

This PR is big, let's do it elsewhere. https://cypress-io.atlassian.net/browse/UNIFY-997

elevatebart · 2022-01-27T16:20:06Z

packages/data-context/src/util/migration.ts

-    findByTestingType(projectRoot, e2eDirPath, 'e2e'),
-  ])
+): Promise<GetRelativeSpecs> {
+  const e2e = await findByTestingType(projectRoot, e2eDirPath, 'e2e')


So we do not rename component specs anymore?

My understanding was that we wanted the rename to avoid collision with jest when moving component tests.

We never did auto rename on component specs - only E2E. Component is always manual.

Basically,

autoRename === e2e

manualRename === CT

https://www.figma.com/file/PAOozuCxtfetqP8fUVvg0y/Cypress-App?node-id=4345%3A32803...

Edit, apparently this is NOT the case, rename is automated, moving is not. 😮‍💨

emilyrohrbough · 2022-01-28T00:27:42Z

packages/data-context/src/sources/MigrationDataSource.ts


-    debug('looked in %s and %s and found %o', compFolder, intFolder, specs)
+    debug('looked in %s and %s and found %o', intFolder, specs)


Suggested change

debug('looked in %s and %s and found %o', intFolder, specs)

debug('looked in %s and found %o', intFolder, specs)

system-tests/projects/migration/README.md

emilyrohrbough · 2022-01-28T00:38:07Z

packages/data-context/src/sources/MigrationDataSource.ts

    const intFolder = await this.integrationFolder()

-    const specs = await getSpecs(this.ctx.currentProject, compFolder || null, intFolder || null)
+    const specs = await getSpecs(this.ctx.currentProject, intFolder || null)


Should we worry about users who have custom testSpec regex pattern in their <10.0 configuration? Currently the pattern(s) will be used to find the specs, whether or not they are in the integration folder: https://github.com/cypress-io/cypress/blob/develop/packages/server/lib/util/specs.ts#L173

EDIT: ~~Also, why did we remove migrating the tests found in the component folder?~~
Nevermind. The naming here didn't seem specific to E2e.

Good catch, fixing this one up.

Also re: migrating component tests, you were right, I'd misunderstood a requirement - the first step of the migration will rename them if you are using the defaults, eg cypress/component/foo.spec.js will become cypress/component/foo.cy.js. In the next step, we ask you to manually move the component specs to be colocated with their component.

This part of the migration (specifically, for component tests) is really not ideal. Although you can opt to leave them in cypress/component, most people like to colocate their component tests. Anyway, that's a product/design decision beyond the scope of this PR - the vast majority of users are E2E, and the workflow works really well for those users.

Co-authored-by: Emily Rohrbough <[email protected]>

* 10.0-release: (25 commits) fix(unify): improve dev server config ergonomics (#19957) feat: add spec pattern modal (#19801) fix: Windows e2e project scaffolding issues (#19938) feat: update @cypress/schematic to use proper e2e config for 10.0.0 (#19827) fix: correctly migrate projects with custom integration folder (#19929) fix: component spec creation with spec pattern (#19862) fix: missed committing yarn.lock after merge conflict fix: correct reference branch / commitSha in performance-reporter (#19941) feat: update navbar UI per Figma (#19926) fix: seed examples files when no e2e directory is created (#19768) chore: remove windy lightBlue warning test: component test updates (#19925) feat: Focus browser from select browser screen and on dashboard login (#19842) test: Honeycomb system-test reporter (#19855) fix(deps): update dependency engine.io to v5.2.1 [security] feat: Retain fileName when working with aliased fixtures and files (#19820) Update release-process.md Update release-process.md Update release-process.md Update release-process.md ...

lmiller1990 added 4 commits January 27, 2022 18:34

repro bug

0fb20a0

write failing case

825457a

remove junk file;

1b4c19e

fix custom integration folder logic

88520c1

lmiller1990 requested review from ImCesar, elevatebart and emilyrohrbough January 27, 2022 09:45

types

b443b31

elevatebart suggested changes Jan 27, 2022

View reviewed changes

update readme

135a8db

emilyrohrbough reviewed Jan 28, 2022

View reviewed changes

system-tests/projects/migration/README.md Outdated Show resolved Hide resolved

add tests

d38859e

emilyrohrbough reviewed Jan 28, 2022

View reviewed changes

lmiller1990 and others added 14 commits January 28, 2022 10:59

adding test for component spec migration

b719fdd

rejig API

11b9413

fix tests

6a51367

fix types

5aee45e

fixing tests

f2a5d47

Update system-tests/projects/migration/README.md

e1c6dd8

Co-authored-by: Emily Rohrbough <[email protected]>

remove old code

af7c9d9

tests

c553c1c

work on tests

5e612e3

fix tests

e666111

more testing

8b290dc

fix workflow

f6c5ff1

edge case

4073689

update

9665c5e

update test

f17c76d

lmiller1990 requested a review from elevatebart January 28, 2022 14:20

remove old test

24596ea

lmiller1990 merged commit 97e9378 into 10.0-release Jan 28, 2022

lmiller1990 deleted the lmiller1990/UNIFY-989 branch January 28, 2022 15:08

sync-by-unito bot mentioned this pull request May 30, 2022

Verify edge cases for folders are covered #21705

Closed

		// TODO: the current logic is wrong and does not consider
		// the case of a custom integration folder!


		debug('looked in %s and %s and found %o', compFolder, intFolder, specs)
		debug('looked in %s and %s and found %o', intFolder, specs)

	debug('looked in %s and %s and found %o', intFolder, specs)
	debug('looked in %s and found %o', intFolder, specs)

fix: correctly migrate projects with custom integration folder #19929

fix: correctly migrate projects with custom integration folder #19929

Uh oh!

Conversation

lmiller1990 commented Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cypress-bot bot commented Jan 27, 2022

Uh oh!

cypress bot commented Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test summary

Run details

Uh oh!

elevatebart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

emilyrohrbough Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lmiller1990 commented Jan 27, 2022 •

edited

Loading

cypress bot commented Jan 27, 2022 •

edited

Loading

emilyrohrbough Jan 28, 2022 •

edited

Loading