-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Use Chrome Headless instead of Electron #6655
Conversation
@mramato, thanks for the pull request! Maintainers, we have a signed CLA from @mramato, so you can review this at any time.
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
Apparently this is having some issues on Windows, so hold off until I bump it again. |
Nevermind, I'm just a moron who forgot to run This is ready. |
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 good! I think there are a few places the Testing Guide may need updates.
@ggetz did you run the tests locally? I couldn't get them to finish on this branch but they finish in master. I ran |
@hpinkos I was able to run locally, but I'll blow away node_modules and double check. |
@hpinkos, what exactly failed for you? What OS and version of Chrome are you running? Did you do a clean |
@mramato it said it timed out. Windows 10, Chrome 67. I didn't try blowing away node_modules but I did npm install. I'll try again. |
It's probably just my computer that's weird. If it passes on travis and passes for both of you guys it's probably good to go |
Yeah, it just failed again after a clean npm install
I guess my computer is just too slow |
Well, we shouldn't just dismiss your failure, but weird things have been going on with our Unit tests for the past few releases, particularly related to WebGL content. Does it work if you run:
|
@mramato no from the command line. |
Whoops, totally misread that. Yeah, running from the website interface works fine. I'm running with the webgl stub now to see what works |
Yes, everything passed with |
I'm getting the same results as @hpinkos. Everything is passing with |
So I think |
Since Chrome added an officially supported headless mode, there's no reason to use Electron for our unit tests. This will be both more accurate (uses actual Chrome) and also gets rid of the rather large Electron dependency. WebGL is disabled in headless, so we only use headless on CI (because we also disable webGL tests there). When running locally, it just shows an actual Chrome window. Chrome has an issue and plans to address this: https://bugs.chromium.org/p/chromium/issues/detail?id=765284
OK, ready for another look. WebGL is disabled in headless, so we only use headless on CI (because we also disable webGL tests there). When running locally, it just shows an actual Chrome window. Chrome has an issue and plans to address this: https://bugs.chromium.org/p/chromium/issues/detail?id=765284 so once WebGL is available in headless we can choose to run the tests headless too (but now that I think about it, most of the time you probably want to debug with the browser anyway). |
Works great @mramato, thanks! I'll merge as soon as CI passes |
I think the Test Guide still needs to be updated to remove references to Electron. |
I updated the doc. This looks good, thanks again @mramato! |
Electron is no longer used in WebStorm and Chrome is used to run tests, we can't use headless until WebGL is supported.
Since Chrome added an officially supported headless mode, there's no reason to use Electron for our unit tests. This will be both more accurate (uses actual Chrome) and also gets rid of the rather large Electron
dependency.
We also want to start testing FirefoxHeadless on CI as well, but need to wait until #6539 is either worked around or fixed.