-
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
Add Firefox support #1359
Add Firefox support #1359
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Test summaryRun details
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 |
edbb977
to
38338d8
Compare
@bkucera I got new from Twitter that this work picked up again, amazing to see it 🤩! Developers I talk to love Cypress and keep apologizing to me in conversations for not testing Firefox. Since I work on Developer Experiences team at Firefox I be would be happy to chat and to help with any questions, bugs or feature requests. |
Is this still a priority @chrisbreiding @bkucera? last commit is pretty old... |
@joshsleeper @andrewholsted 2 weeks is 6 months in javascript-years, but yes I have been busy with 3.5 release. This PR is very close |
cli/types/index.d.ts
Outdated
@@ -64,6 +64,7 @@ declare namespace Cypress { | |||
path: string | |||
isHeaded: boolean | |||
isHeadless: boolean | |||
family: string |
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.
don't we already define this interface inside of packages/launcher
? or somewhere else?
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.
probably because there is no clean way to share code between packages and this - a lot of types are duplicated because of that. it would be nice to revisit how we do types to untangle this mess.
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.
overall pretty damn good.
i didn't leave comments on a lot of uncommented code since i know it's still WIP
{ | ||
"globals": { | ||
"Cypress": true | ||
} | ||
} |
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.
this is code smell needing to pull in globals - i get the challenge - you want DOM utils to be stateless, and avoid DI or instantiation but there's probably a middle ground here we can facilitate
@@ -36,6 +36,15 @@ const attachMouseDblclickListeners = attachListeners(['dblclick']) | |||
const attachContextmenuListeners = attachListeners(['contextmenu']) |
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.
why are there so many lines changed in this file... ?
.should('have.focus') | ||
|
||
if (Cypress.browser.family === 'firefox') { | ||
cy.wait(0).get('@selectionchange').should('be.called') |
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.
whys this necessary?
all of these conditionals need a comment else its completely unclear and opaque why its being done
This comment has been minimized.
This comment has been minimized.
❤️ the commit velocity in this pull request. I saw that you are working around a few issues already. If you hit any blockers in Firefox, let me know and I can help solve them. |
* firefox headless * update errors * fix video recording in ff headless * fix thos types
… issue-1096-firefox-support
@JenniferFuBook We're working overtime to get this out, as can be seen by our commits in this branch. So, please be patient, fixing a few remaining bugs. 🙏 |
Co-authored-by: Zach Bloomquist <github@chary.us>
… issue-1096-firefox-support
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.
🥇
User facing changelog
Cypress now support running tests in Firefox.
Additional details
Tasks
window
objectsfix not setting selected broswer withnot a bug, only sets default browser when setting custom browserbrowser
launch arge2e/video_spec
)chromeWebSecurity: false
when running firefox (desktop gui, terminal reporter)How has the user experience changed?
beta flag for initial release:
chromeWebSecurity
warning (desktop-gui):before:browser:launch
warning (desktop-gui):warnings in
run
mode:forced GC warning
TODO: add screenshot of forced GC warning
PR Tasks
cypress-documentation
? Add docs for Firefox support cypress-documentation#420type definitions
?cypress.schema.json
?Additional testing @bahmutov
later can add