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: create new spec file from desktop-gui #15335

Merged
merged 24 commits into from Mar 23, 2021
Merged

feat: create new spec file from desktop-gui #15335

merged 24 commits into from Mar 23, 2021

Conversation

panzarino
Copy link
Contributor

@panzarino panzarino commented Mar 5, 2021

User facing changelog

Adds the ability to create a new spec file directly from the desktop gui

Additional details

Dialog needs to be tested on Windows and Linux. I had some issues when trying to run myself - while I'm confident it probably works well, its not a bad idea to test on the native system before merging.

We use the system dialog to prompt for file location rather than a custom file picker since there are a number of benefits (eg. create folder, duplicate detection) that come with using the system dialog rather than a custom implementation (and there's no need to reinvent the wheel). The only downside (for less experienced users) is that the user is able to create a file outside of their Cypress project, which means it may not show up in their spec list. The system dialog provides no way to limit the user to selecting certain directories, so the best we can do is show is put them in their integration folder by default and show a message if they choose to add a file outside of their spec list (which some advanced users might actually want to do).

After creating a file in the integrations folder, the new file will scroll into view and get highlighted for a second.

Do docs need to be written for this? I feel like it's fairly self explanatory but we do document basically everything (and could give us a place to explain in a bit more detail why only recognized spec files in the integrations folder will show up).

This should pair nicely with the changes introduced in #15144, especially once Studio hits GA. With Studio, a user is able to download Cypress for the first time and write a test without ever needing to open their filesystem or IDE.

How has the user experience changed?

Creating a new file

ezgif-4-609b97b5ef41

Message when file is created in a different directory

Screen Shot 2021-03-09 at 16 29 02

Content of created file

// spec_name.js created with Cypress
//
// Start writing your Cypress tests below!
// If you're unfamiliar with how Cypress works,
// check out the link below and learn how to write your first test:
// https://on.cypress.io/writing-first-test

PR Tasks

  • Have tests been added/updated?
  • Has the original issue or this PR been tagged with a release in ZenHub?
  • Has a PR for user-facing changes been opened in cypress-documentation?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Mar 5, 2021

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented Mar 5, 2021



Test summary

9405 0 119 3Flakiness 0


Run details

Project cypress
Status Passed
Commit 2d33038
Started Mar 23, 2021 4:08 PM
Ended Mar 23, 2021 4:21 PM
Duration 12:47 💡
OS Linux Debian - 10.5
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

@panzarino panzarino marked this pull request as ready for review March 10, 2021 06:28
@github-actions
Copy link
Contributor

Internal Jira issue: TR-704

Copy link
Member

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

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

You may want to get @flotwig to test in Windows/Linux. I think he's setup to test both.

I guess it's also technically possible they could create a file that would be filtered out of the testFiles regex, so that's another reason a new file may not show up in the list. I dunno if it's worth mentioning in the warning.

Oh there's a little format dropdown, that is cool.

The user experience looks good to me on the Mac. 👍

I'll approve and leave it up to you if you also want flotwig to manually test.

@panzarino
Copy link
Contributor Author

@jennifer-shehane Yeah I noticed that issue with testFiles while doing some manual testing myself - I attempted to address this in the error message with "or is not recognized as a spec file" but we could be more explicit there. I've made some very minor cosmetic changes since your review if you want to take a quick look but don't feel obligated.

@flotwig It'd be great if you could do a simple test on Windows & Linux just to make sure that the user flow is pretty similar. Any images or gifs that you could provide would be super appreciated. Thanks so much!

@flotwig
Copy link
Contributor

flotwig commented Mar 16, 2021

Merged in develop since it fixes building on Windows 😛

Copy link
Contributor

@flotwig flotwig left a comment

Choose a reason for hiding this comment

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

I noticed an issue when running in Windows - it says my spec file was created outside of the spec tree, but it is clearly not (pardon the palette, GitHub's file size limit is 10mb): o

Strangely, the same thing doesn't happen on Linux: 2021-03-16 new spec file desktop-gui linux

@flotwig
Copy link
Contributor

flotwig commented Mar 16, 2021

I wonder if the filePath on Windows is somehow not what we expect for Linux and macOS?

Here is the console.logged yielded value of dialog.openSaveDialog on Windows:

{
  canceled: false,
  filePath: 'C:\\Users\\zbloo\\AppData\\Local\\Temp\\foo\\cypress\\integration\\untitled.spec.js'
}

@@ -77,6 +83,10 @@ export class SpecsStore {
})
}))

if (this.newSpecAbsolutePath && !_.find(this._files, { absolute: this.newSpecAbsolutePath })) {
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, i bet the paths in this._files are either double-escaped Windows paths, or POSIX paths, that's why this is not working as expected on Windows

@panzarino panzarino requested a review from flotwig March 22, 2021 21:09
@panzarino
Copy link
Contributor Author

@flotwig Nice catch - should be fixed now. Since I tested this on Mac (w/ stubbed tests) could you do one last manual verification that it works properly on an actual Windows system?

Copy link
Contributor

@flotwig flotwig left a comment

Choose a reason for hiding this comment

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

seems to be working great now:
2021-03-22 new spec file desktop-gui

@panzarino panzarino merged commit 3700fe7 into develop Mar 23, 2021
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Apr 5, 2021

Released in 7.0.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v7.0.0, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Apr 5, 2021
@flotwig flotwig deleted the GROW-173 branch January 24, 2022 18:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants