-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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: escape backslashes in downloadFolder for Firefox on Windows #17896 #23006
fix: escape backslashes in downloadFolder for Firefox on Windows #17896 #23006
Conversation
Thanks for taking the time to open a PR!
|
0437c0b
to
7c31095
Compare
Test summaryRun details
View run in Cypress Dashboard ➡️ Flakiness
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @mirobo. Thanks for submitting this pull request to fix your issue! Would you be able to add a test in firefox_spec.ts
that covers the expected behavior? The existing tests should give a good base for adding the downloadsFolder
to the options and you should be able to stub out the os.platform
similar to some of the tests in the launcher.
I'd like to but there are some big issues for me
I'm in timezone GMT+1 (but can also get up during the night) if there is any possibility of a "Cypress development setup fixing session".. |
We currently have some flake occurring in the build, to the point where getting a green build on a first time run is extremely rare. This obviously isn't great, but the good news is we are starting to address the flake this week and hopefully get ourselves to greener builds in the near future 😄 .
Interesting. I wonder if there are issues running in global mode on Windows. What version of Windows are you developing on and have you followed all the windows instructions in the contributing guide? Just want to make sure we can rule some things out. Might be good to try The good news to write server unit tests is they are run with mocha inside a node context, and you can run those in the package directory. To test this, you could right something like:
you can change directories to
Hopefully I can provide enough guidance on the thread to get you going 😄 . I have a windows 10 instance so if you are running into issues I can see if I can reproduce or provide a solution. |
…s://github.com/mirobo/cypress into issue-17896-firefox-doesnt-use-download-folder
Update Just after writing this I pulled from develop again and I was able to run "yarn dev". Holy moly :-) Let's see how long this holds up :D
Yes, I followed the guide NPM 8.11.0 In scripts\binary\build.ts I commented out this because it will never run in Windows:
I added CYPRESS_INTERNAL_VITE_DEV=1 to .npmrc When running "yarn" the script just continues and you wouldn't even see the following error (I googled a lot regarding this error and I couldn't find any solution):
This failure is probably why Cypress only shows "not found". The launchpad-server itself is running and it also shows "not found" when I open the URL in a normal browser. I also added this script to packages/launchpad/package.json to quickly start the vite build.. When running "yarn check --integrity", everything is fine but running "yarn check" gives me 32 errors.. can't be true that there are so many discrepancies after running yarn?
|
Back to a problem I already had and never got resolved: It starts a Cypress.exe in the background but it is said that system-tests should not generally required a Cypress.exe build?
Also why can't I just run some download tests in "system-tests\projects\downloads" with the UI started with "yarn dev"? I think for my issue I could also call some tests as in "system-tests\test\downloads_spec.ts" because to me it only matters that the file is downloaded to the configured directory. Or can't we just dynamically run all tests in "downloads_spec.ts" on Firefox, Electron and Google Chrome? |
What version of Windows are you running? It is still unclear to me if you ran Also to be clear, you did run Any reason you are using
Why do you need to build the binary? This change shouldn't be necessary
This shouldn't make a difference, only if you are making changes to client side bundle aspects of Cypress. This flag is just going to use
There are some nested dependencies that have a few semver violations. This shouldn't be an issue for getting you up and running.
|
Looks like Windows 10 Home, shouldn't be any issues there. Almost looks like an issue with the webapp building
No worries this is expected
we usually execute these through
What is happening is that the
Now that is weird. Going to try a fresh dev setup of Cypress on windows hopefully this weekend. I want to see if I run into a similar problem. Just to check are you executing this through DOS or PowerShell?
Definitely faster for sure! Just figure we can get you up and running without throwing in more variables 😅 .
We will get this ironed out! |
unfortunately didn't get a chance to look at this over the weekend, but hoping this afternoon to see if I yield similar results |
I'll try it also on an older laptop.. not sure how far I'll get :D (i5 2540M with a new SSD and 16 GB RAM) |
Update: It works on the other laptop. I guess the failing connection to Cypress.exe that I posted previously may have something to do with the fact that Cypress 10+ freezes from time to time on my regular PC 🤔. I'll create a separate issue for this including the crash dump. Now I need to figure out why the tests in place don't fail. Are all the Cypress tests only executed on a Unix system and never on MacOS or Windows? That would explain why the tests don't fail in CI.. |
@mirobo I had a few minutes today and added in the test I was talking about in #17896 (comment). Going to work on getting this reviewed through support rotation. |
can no longer be a reviewer as they have contributed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great and tests great, thank you for your contribution!
Huge thanks to @AtofStryker! |
@mirobo glad I could help! This should make its way into the next release. |
User facing changelog
n/a
Steps to test
How has the user experience changed?
no
PR Tasks
cypress-documentation
?type definitions
?