-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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: Modify doubleEscape to handle path with extra backslashes and launch edge without browser key #14723
Conversation
Thanks for taking the time to open a PR!
|
Can someone please review this PR? |
@SagarGaniga The server-e2e-tests-chrome failing are unrelated to this PR. A recent change in our stdout warnings is causing some flake in our internal snapshots. Our team is hoping to review this PR next sprint. |
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! Just a few minor things
if (pathStr.indexOf('chrome.exe') > -1) { | ||
return { path, browserKey: 'chrome' } | ||
} | ||
|
||
if (pathStr.indexOf('edge.exe') > -1) { | ||
return { path, browserKey: 'edge' } |
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.
Ha, I guess this does technically work for msedge.exe too
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.
Yeah, keeping that browserKey
edge for msedge.exe too as we use edge
everywhere else.
expect(res.browserKey).to.eq('chrome') | ||
}) | ||
|
||
it('returns path and edge given just path', () => { |
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.
Could also use a test for msedge.exe - that is what #14716 is about
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.
Yes, updated
function doubleEscape (s: string) { | ||
return win32.join(...s.split(win32.sep)).replace(/\\/g, '\\\\') | ||
} | ||
|
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.
could you just import this from windows.ts
?
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.
I think you mean packages/launcher/lib/windows/index.ts
Exported that function, used it in the test case and added its own test cases
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.
Great stuff! Thanks for submitting this PR.
User facing changelog
--browser
for edge browsers on windows, allows users to pass just path to edge.exe and launch the browser.--browser
to launch browsers when the path has extra backslashes on windowsAdditional details
--browser
was breaking on windows for every browser but it was fixed in v5.4.0. But it still has issuesThis PR adds code to handle edge paths and modifies
doubleEscape
function with the help ofpath.win32
module which is capable of handling windows specific path separators inwindows/index.ts
This PR also adds some test cases to make sure these use cases are always tested.
How has the user experience changed?
Users can pass edge and chrome paths with more freedom around escaped chars.
Users can now
cypress run --browser "C:\\foo\\bar\\edge.exe"
cypress run --browser "C:\\\\foo\\\\bar\\\\chrome.exe"
PR Tasks