feat: Enable create from Vue component for projects with custom spec patterns #23298

astone123 · 2022-08-11T22:16:59Z

Closes Add 'create from Vue component' card to projects with custom spec patterns #23071

User facing changelog

This is a part of the create from Vue component effort. It shouldn't have its own changelog entry

Additional details

In the last create from Vue component PR, I excluded projects with custom spec patterns to separate out complexity. In order to release the feature, this also needs to work for projects with custom spec patterns.

Steps to test

Open a Vue project that has a custom spec pattern defined for CT
If your project has specs, click on the "+ New Spec" button
Verify that you can see the "Create from component" card
Click on the card and then choose a Vue component from the list.
Verify that the spec file that is created matches your project's spec pattern and that the component you selected is imported correctly into the spec file (best way to verify this is to run the new spec and ensure that the spec mounts correctly).

How has the user experience changed?

Users will now see the "Create from component" generator card when adding a new spec to a Vue project using component testing when the project has a custom spec pattern defined in its configuration.

PR Tasks

Have tests been added/updated?
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?

…patterns

cypress-bot · 2022-08-11T22:17:02Z

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.

astone123 · 2022-08-11T22:19:13Z

system-tests/projects/no-specs-custom-pattern-vue/babel.config.js

@@ -0,0 +1,5 @@
+module.exports = {


The diff looks really big because I added a new project to system-tests. I needed a project that had a custom spec pattern but was a Vue project.

You should be safe to ignore any changed files under system-tests/projects/

packages/data-context/src/actions/ProjectActions.ts

ZachJW34

Gave it a thorough test and it worked great! Just a few comments

packages/app/cypress/e2e/specs.cy.ts

packages/data-context/src/actions/ProjectActions.ts

packages/data-context/src/codegen/spec-options.ts

- Remove test project - Break tests into new spec file - Move getDefaultSpecFileName to util module

…ec-pattern

packages/app/cypress/e2e/create-from-component.cy.ts

mike-plummer · 2022-08-12T18:48:26Z

packages/data-context/src/codegen/spec-options.ts


- return componentPath.startsWith('.') ? componentPath : `./${componentPath}`
+ return `./${this.parsedPath.base}`


Would ./ cause any issues for Windows?

Would using path.join('./', componentPath)resolve any issue with os specifics?

This is for the component import path in the component e.g. import App from '<relative-path>'.js. These should always be forward slashes, but still something we should check on Windows since we might be putting backslashes in the path where they shouldn't be.

Something similar here: https://github.com/cypress-io/cypress/blob/develop/packages/data-context/src/sources/migration/codegen.ts#L269-L275

Yeah this should be fine on Windows because this is the path we're using to import the component in the tests. Mark tested my last PR on Windows that had similar logic and he said it worked well.

I'll try this out on my Windows VM before I merge it, anyone feel free to do the same

Empty-generator is what's happening now, so that sounds OK.

Gave it a try on my Windows and a spec that is generated using a custom spec-pattern

@ZachJW34 yeah I think this is happening because when I use path in spec-options it takes the OS into account, so the path it gives me has backslashes in it on Windows 🤔

Kind of hacky, but what do we think about just replacing \ with / for these relative paths? I still need to use Node path to get the relative path of the component module compared to the spec directory, but I don't want it to use OS-specific slashes because this is a relative import being written to a file

Maybe we can use this function

cypress/packages/data-context/src/util/file.ts

Lines 3 to 5 in c40a120

export function toPosix (file: string, sep: string = path.sep) {

return file.split(sep).join(path.posix.sep)

}

Thanks Lachlan, this should fix the issue on Windows

Yeah the replacement makes sense, we have a few examples of posixify-ing paths in the codebase

cypress · 2022-08-12T18:56:47Z

Test summary

37840 • 0 • 615 • 0 • 7

Run details


Project	cypress
Status	Passed
Commit	`a2b4ff7`
Started	Aug 18, 2022 12:50 PM
Ended	Aug 18, 2022 1:05 PM
Duration	15:07 💡
OS	Linux Debian - 11.3
Browser	Multiple

View run in Cypress Dashboard ➡️

Flakiness

	cypress/e2e/commands/net_stubbing.cy.ts		5
1	network stubbing > intercepting request > can delay with deprecated delayMs param
2	network stubbing > intercepting request > can delay with deprecated delayMs param
3	network stubbing > intercepting request > can delay with deprecated delayMs param
4	network stubbing > intercepting request > can delay with deprecated delayMs param
5	network stubbing > intercepting request > can delay with deprecated delayMs param
This comment includes only the first 5 flaky tests. See all 7 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

lmiller1990

I found an inconsistency: it's using " for the component import but ' for the describe/it blocks. Feels a bit bad, quick fix? 🙏

packages/data-context/src/codegen/spec-options.ts

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

packages/data-context/src/sources/migration/utils.ts

- Use object for getPathFromSpecPattern params - Use single quotes for file import in generated spec - Remove unnecessary async keyword

… github.com:cypress-io/cypress into astone123/23071-create-from-vue-custom-spec-pattern

ZachJW34

Found a bug while creating a spec from a component that already has an existing spec when using a custom spec pattern. The copy gets appended after the .cy, causing the spec to not match.

Screen.Recording.2022-08-15.at.10.08.27.AM.mov

astone123 · 2022-08-15T15:50:04Z

@ZachJW34 it looks like that issue already exists currently for create empty spec. If you try to create a file with the same name, we append "copy" and it ends up not matching the spec pattern. We don't give a warning there either.

I wonder if we could push this bug fix off into a separate issue since it already exists and is common logic between the two cards.

ZachJW34 · 2022-08-15T16:04:53Z

@ZachJW34 it looks like that issue already exists currently for create empty spec. If you try to create a file with the same name, we append "copy" and it ends up not matching the spec pattern. We don't give a warning there either.

I wonder if we could push this bug fix off into a separate issue since it already exists and is common logic between the two cards.

What is the reproduction for the empty spec bug? I gave it a try and it seemed to be working. I ran through empty-spec generation twice for src/ComponentName.cy.js (the default) and got:

src/ComponentName.cy.js
src/ComponentName-copy-1.cy.js

This is the expected output and it matches the specPattern. For the create-from-component, the output was src/App.cy-copy-1.js where the cy and copy are swapped.

Also, I posted a comment regarding Windows import paths: #23298 (comment)

astone123 · 2022-08-15T16:08:24Z

@ZachJW34 Oh right, you wouldn't be able to see the bug with that spec pattern. I was using 'src/specs-folder/*-cypress-spec.js' as my spec pattern, and with both methods of generation I saw the issue.

So in order to see it with empty, you need a custom suffix for your file names. I see that it's a different issue for create from component though, so I'll check it out

lmiller1990 · 2022-08-15T19:34:07Z

My changes were responded to!

lmiller1990 · 2022-08-15T23:29:16Z

I found another edge case:

Go to frontend-shared
Change specPattern to **/*.test.js
Go to open mode
It should show no specs found (since you changed the pattern)
Create a spec for Alert.vue
It creates a spec in Alert.test.js (top level) but the UI shows "Created in src/components/Alert.test.js.

lmiller1990 · 2022-08-15T23:29:29Z

Dismissing my review since I found a bug.

lmiller1990

#23298 (comment)

May need a mini brief around the edge cases, I am not sure the expected filename and path is clear (above behavior is expected based on current code, but certainly not what you'd want).

lmiller1990 · 2022-08-16T02:38:07Z

I found an edge case. Here's my expected output when creating a new spec:

specPattern	existing specs	component	expected new spec
`src/*/.js`	n/a	src/Foo.vue	src/Foo.cy.js
`*/.js`	n/a	src/Foo.vue	src/Foo.cy.js
`*/.js`	src/Bar.cy.js	src/Foo.vue	src/Foo.cy.js
`*/.js`	src/Foo.cy.js	src/Foo.vue	src/Foo-copy-1.cy.js

Case 2 is problematic. I think it's because we use https://github.com/cypress-io/cypress/pull/23298/files#diff-c351c3ec7e69c56e8c57fdf49babbece37086dbb43b9fc07a386f3cd33e508c9R52. When I created a spec in a new project where the spec pattern is **/*.cy.ts, I chose src/Alert.vue for the spec, and it's creating it at the top level - Alert.cy.tsx, which is not really what you'd expect.

…3/23071-create-from-vue-custom-spec-pattern

lmiller1990 · 2022-08-17T00:00:59Z

packages/data-context/test/unit/codegen/spec-options.spec.ts

+ sinon.restore()
+ });
+
+ [{ testName: 'src/specs-folder/*.cy.{js,jsx}', componentPath: 'ComponentName.vue', specs: [], pattern: 'src/specs-folder/*.cy.{js,jsx}', expectedPath: 'src/specs-folder/ComponentName.cy.js' },


Nice way to cover all the cases

lmiller1990 · 2022-08-17T07:55:32Z

Just checking if we need a re-review or should this go back to in progress? Also might be good to clarify the test plan for the various edge cases (eg what we will do for **/*.test.js for CT projects without any specs, etc). 🙏

astone123 · 2022-08-17T21:15:51Z

Okay, after looking at this some more I updated the assertions for the unit tests and fixed a couple things with extensions. This doesn't match the table that @lmiller1990 put in an earlier comment, but these do match the default file name for the empty spec generator.

I'm hoping that this is good enough for now, as the plan for this wasn't to change the underlying defaultSpecFileName logic that we use. If we want to change our default spec name logic then I think we should follow up with another issue, or we should update this generator to allow the user to input their own file path for the spec.

To get an idea of the different cases that we have test coverage for and which file paths they result in, check out this test block

@ZachJW34 can you verify that you can create duplicate files and the -copy doesn't get added to your file extension now?

ZachJW34

My comments have been addressed!

One last thing I'd like to throw out, would it make sense to create the sibling path first and check to see if it matches the specPattern before calling the getDefaultSpecFileName? This would reduce the likelihood that a spec gets generated away from it's sibling component when using a specPattern like **/*.cy.js

Just a thought, going to ✅

lmiller1990 · 2022-08-18T00:42:03Z

I also verified, copy naming bug is gone, I think this is good for now - I do expect this feature to receive lots of updates moving forward, we can address/rejig/rethink more when we get the next requirement/stories. Nice job, this one sure turned out to have some hidden complexity.

lmiller1990

LGTM

Will leave merging to @astone123 since I think there's one more potential (optional) change that @ZachJW34 pointed out that you may or may not want to do - I'm thinking we ship this and focus on Studio/other stuff for now, though?

…ec-pattern

astone123 · 2022-08-18T14:59:53Z

My comments have been addressed!

One last thing I'd like to throw out, would it make sense to create the sibling path first and check to see if it matches the specPattern before calling the getDefaultSpecFileName? This would reduce the likelihood that a spec gets generated away from it's sibling component when using a specPattern like **/*.cy.js

Just a thought, going to ✅

Yeah, I started on this but it ended up being more complicated than I originally thought, so I decided that for now we should just go with the behavior of the existing spec generator card. Will open an issue to improve this

astone123 added 3 commits August 11, 2022 14:09

feat: Enable create from Vue component for projects with custom spec …

d83f789

…patterns

feat: Update existing tests

b452cbd

feat: Add new tests

b29e7c6

astone123 requested review from a team as code owners August 11, 2022 22:17

astone123 requested review from lmiller1990, marktnoonan, mike-plummer and rockindahizzy August 11, 2022 22:17

astone123 self-assigned this Aug 11, 2022

astone123 commented Aug 11, 2022

View reviewed changes

astone123 requested a review from ZachJW34 August 11, 2022 22:19

astone123 commented Aug 11, 2022

View reviewed changes

packages/data-context/src/actions/ProjectActions.ts Outdated Show resolved Hide resolved

lmiller1990 removed the request for review from a team August 12, 2022 07:01

ZachJW34 reviewed Aug 12, 2022

View reviewed changes

astone123 added 4 commits August 12, 2022 14:08

feat: Feedback

f6d9c90

- Remove test project - Break tests into new spec file - Move getDefaultSpecFileName to util module

feat: Revert changes to .eslintignore

937b281

feat: Remove erroredCodegenCandidate

52c022b

Merge branch 'develop' into astone123/23071-create-from-vue-custom-sp…

7fd942a

…ec-pattern

mike-plummer reviewed Aug 12, 2022

View reviewed changes

lmiller1990 suggested changes Aug 15, 2022

View reviewed changes

packages/data-context/src/codegen/spec-options.ts Outdated Show resolved Hide resolved

packages/data-context/src/sources/ProjectDataSource.ts Outdated Show resolved Hide resolved

packages/data-context/src/sources/migration/utils.ts Show resolved Hide resolved

astone123 added 2 commits August 15, 2022 09:13

feat: Feedback

e51215e

- Use object for getPathFromSpecPattern params - Use single quotes for file import in generated spec - Remove unnecessary async keyword

Merge branch 'astone123/23071-create-from-vue-custom-spec-pattern' of…

634bf21

… github.com:cypress-io/cypress into astone123/23071-create-from-vue-custom-spec-pattern

astone123 requested a review from lmiller1990 August 15, 2022 13:15

feat: Revise create-from-component tests

cd152d6

astone123 requested review from mike-plummer and ZachJW34 August 15, 2022 15:01

ZachJW34 suggested changes Aug 15, 2022

View reviewed changes

lmiller1990 approved these changes Aug 15, 2022

View reviewed changes

astone123 mentioned this pull request Aug 15, 2022

10.5.0 changelog cypress-io/cypress-documentation#4668

Merged

feat: Fix relative import on Windows

ce4c883

lmiller1990 self-requested a review August 15, 2022 23:29

lmiller1990 suggested changes Aug 15, 2022

View reviewed changes

astone123 added 2 commits August 16, 2022 15:16

feat: Add unit tests to describe multiple spec pattern cases

ed8041d

Merge branch 'develop' of github.com:cypress-io/cypress into astone12…

f0136a1

…3/23071-create-from-vue-custom-spec-pattern

lmiller1990 reviewed Aug 17, 2022

View reviewed changes

feat: Update tests and fix extension

04d8656

astone123 requested review from ZachJW34 and lmiller1990 August 17, 2022 21:15

ZachJW34 approved these changes Aug 17, 2022

View reviewed changes

lmiller1990 approved these changes Aug 18, 2022

View reviewed changes

Merge branch 'develop' into astone123/23071-create-from-vue-custom-sp…

a2b4ff7

…ec-pattern

astone123 merged commit 0318262 into develop Aug 18, 2022

astone123 deleted the astone123/23071-create-from-vue-custom-spec-pattern branch August 18, 2022 15:00

cypress-bot bot mentioned this pull request Aug 18, 2022

Add 'create from Vue component' card to projects with custom spec patterns #23071

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Enable create from Vue component for projects with custom spec patterns #23298

feat: Enable create from Vue component for projects with custom spec patterns #23298

astone123 commented Aug 11, 2022 •

edited

Loading

cypress-bot bot commented Aug 11, 2022

astone123 Aug 11, 2022

ZachJW34 left a comment

mike-plummer Aug 12, 2022

rockindahizzy Aug 12, 2022 •

edited

Loading

ZachJW34 Aug 12, 2022

lmiller1990 Aug 15, 2022

astone123 Aug 15, 2022

marktnoonan Aug 15, 2022

astone123 Aug 15, 2022

lmiller1990 Aug 15, 2022

astone123 Aug 15, 2022

ZachJW34 Aug 15, 2022

cypress bot commented Aug 12, 2022 •

edited

Loading

lmiller1990 left a comment

ZachJW34 left a comment

astone123 commented Aug 15, 2022

ZachJW34 commented Aug 15, 2022 •

edited

Loading

astone123 commented Aug 15, 2022

lmiller1990 commented Aug 15, 2022

lmiller1990 commented Aug 15, 2022

lmiller1990 commented Aug 15, 2022

lmiller1990 left a comment

lmiller1990 commented Aug 16, 2022

lmiller1990 Aug 17, 2022

lmiller1990 commented Aug 17, 2022

astone123 commented Aug 17, 2022

ZachJW34 left a comment

lmiller1990 commented Aug 18, 2022

lmiller1990 left a comment

astone123 commented Aug 18, 2022


		return componentPath.startsWith('.') ? componentPath : `./${componentPath}`
		return `./${this.parsedPath.base}`

	export function toPosix (file: string, sep: string = path.sep) {
	return file.split(sep).join(path.posix.sep)
	}

feat: Enable create from Vue component for projects with custom spec patterns #23298

feat: Enable create from Vue component for projects with custom spec patterns #23298

Conversation

astone123 commented Aug 11, 2022 • edited Loading

User facing changelog

Additional details

Steps to test

How has the user experience changed?

PR Tasks

cypress-bot bot commented Aug 11, 2022

Choose a reason for hiding this comment

ZachJW34 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rockindahizzy Aug 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cypress bot commented Aug 12, 2022 • edited Loading

Test summary

Run details

Flakiness

lmiller1990 left a comment

Choose a reason for hiding this comment

ZachJW34 left a comment

Choose a reason for hiding this comment

astone123 commented Aug 15, 2022

ZachJW34 commented Aug 15, 2022 • edited Loading

astone123 commented Aug 15, 2022

lmiller1990 commented Aug 15, 2022

lmiller1990 commented Aug 15, 2022

lmiller1990 commented Aug 15, 2022

lmiller1990 left a comment

Choose a reason for hiding this comment

lmiller1990 commented Aug 16, 2022

Choose a reason for hiding this comment

lmiller1990 commented Aug 17, 2022

astone123 commented Aug 17, 2022

ZachJW34 left a comment

Choose a reason for hiding this comment

lmiller1990 commented Aug 18, 2022

lmiller1990 left a comment

Choose a reason for hiding this comment

astone123 commented Aug 18, 2022

astone123 commented Aug 11, 2022 •

edited

Loading

rockindahizzy Aug 12, 2022 •

edited

Loading

cypress bot commented Aug 12, 2022 •

edited

Loading

ZachJW34 commented Aug 15, 2022 •

edited

Loading