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

Replace PhantomJS with Firefox #86

Merged
merged 2 commits into from May 25, 2017

Conversation

@ef4
Contributor

ef4 commented Dec 4, 2016

Rendered

@twokul

This comment has been minimized.

Show comment
Hide comment
@twokul

twokul Dec 4, 2016

Contributor

@ef4 interesting. I came across this article a while ago which explains how to adopt Electron as a default environment for running tests in CI. I wonder if that's something you considered

Contributor

twokul commented Dec 4, 2016

@ef4 interesting. I came across this article a while ago which explains how to adopt Electron as a default environment for running tests in CI. I wonder if that's something you considered

@mmun

This comment has been minimized.

Show comment
Hide comment
@mmun

mmun Dec 4, 2016

Member

CircleCI's Ubuntu 14.04 build has Chrome 53 and Firefox 47 installed by default.

Member

mmun commented Dec 4, 2016

CircleCI's Ubuntu 14.04 build has Chrome 53 and Firefox 47 installed by default.

@trentmwillis

This comment has been minimized.

Show comment
Hide comment
@trentmwillis

trentmwillis Dec 4, 2016

Member

I am hugely on board with this change! I've had many conversations on this exact topic.

While I personally am holding out for the day that we get easy headless Chrome support, I think using any browser other than Phantom is beneficial for precisely the reasons mentioned in this RFC (particularly the fact that no actual app user uses Phantom).

One concern I do want to bring up is rollout plan. I think the story for applications is relatively straightforward, but the addon story is a bit odd. Specifically, if all addons stopped testing against Phantom, but many consumers still use Phantom, then there is potential for support issues to arise. Would we have any strategy to mitigate this risk? Perhaps ask existing addon maintainers to not immediately drop Phantom support?

Member

trentmwillis commented Dec 4, 2016

I am hugely on board with this change! I've had many conversations on this exact topic.

While I personally am holding out for the day that we get easy headless Chrome support, I think using any browser other than Phantom is beneficial for precisely the reasons mentioned in this RFC (particularly the fact that no actual app user uses Phantom).

One concern I do want to bring up is rollout plan. I think the story for applications is relatively straightforward, but the addon story is a bit odd. Specifically, if all addons stopped testing against Phantom, but many consumers still use Phantom, then there is potential for support issues to arise. Would we have any strategy to mitigate this risk? Perhaps ask existing addon maintainers to not immediately drop Phantom support?

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Dec 4, 2016

Contributor

👍 to this

I do share @trentmwillis's concern, but frankly I think that the benefits still massively outweight the downsides and any addon authors that still want to support Phantom can either run both Phantom and FireFox (or just Phantom) in CI (testem makes this super easy). Basically, a change to the default blueprint like this (IMO) does not constitute a breaking change either way, since at each juncture addon and app authors will have to confirm that they want this change (during ember init / upgrades / etc).

Contributor

rwjblue commented Dec 4, 2016

👍 to this

I do share @trentmwillis's concern, but frankly I think that the benefits still massively outweight the downsides and any addon authors that still want to support Phantom can either run both Phantom and FireFox (or just Phantom) in CI (testem makes this super easy). Basically, a change to the default blueprint like this (IMO) does not constitute a breaking change either way, since at each juncture addon and app authors will have to confirm that they want this change (during ember init / upgrades / etc).

```
before_script:
- export DISPLAY=:99; sh -e /etc/init.d/xvfb start; sleep 3

This comment has been minimized.

@nathanhammond

nathanhammond Dec 5, 2016

Contributor

What about CI on Windows? I consider this to be a non-starter until we have a story for AppVeyor. If you test your addon on Windows it literally doesn't matter if you get to opt in or out, some transitive can easily break you in an irrecoverable manner since you can't simply switch to Firefox.

@nathanhammond

nathanhammond Dec 5, 2016

Contributor

What about CI on Windows? I consider this to be a non-starter until we have a story for AppVeyor. If you test your addon on Windows it literally doesn't matter if you get to opt in or out, some transitive can easily break you in an irrecoverable manner since you can't simply switch to Firefox.

This comment has been minimized.

@nathanhammond

nathanhammond Dec 5, 2016

Contributor

Short version: we need a transition path for each of our "supported" CI platforms.

@nathanhammond

nathanhammond Dec 5, 2016

Contributor

Short version: we need a transition path for each of our "supported" CI platforms.

This comment has been minimized.

@stefanpenner

stefanpenner Dec 5, 2016

Contributor

I believe appveyor is actually in a better situation, as one can easily install all major browsers via chocolatey.

something like the following in appveyor.yml

install:
  - choco install firefox
  - choco install googlechrome
@stefanpenner

stefanpenner Dec 5, 2016

Contributor

I believe appveyor is actually in a better situation, as one can easily install all major browsers via chocolatey.

something like the following in appveyor.yml

install:
  - choco install firefox
  - choco install googlechrome

This comment has been minimized.

@stefanpenner

stefanpenner Dec 5, 2016

Contributor

Hmm, may actually already be bundled (atleast the drivers are)? https://www.appveyor.com/docs/installed-software/#testing

@stefanpenner

stefanpenner Dec 5, 2016

Contributor

Hmm, may actually already be bundled (atleast the drivers are)? https://www.appveyor.com/docs/installed-software/#testing

This comment has been minimized.

@stefanpenner

stefanpenner Dec 5, 2016

Contributor

@nathanhammond its also worth pointing out, I don't think we bundle appveyor.yml in our addon blueprint. That being said, that is likely something we should change independently of this RFC, likely its own RFC, but it also we don't need to consider it a non-starter.

@stefanpenner

stefanpenner Dec 5, 2016

Contributor

@nathanhammond its also worth pointing out, I don't think we bundle appveyor.yml in our addon blueprint. That being said, that is likely something we should change independently of this RFC, likely its own RFC, but it also we don't need to consider it a non-starter.

This comment has been minimized.

@raytiley

raytiley Dec 5, 2016

Member

We use appveyor to test and chrome and firefox are pre-installed. It's under selenium section which is kinda weird: https://www.appveyor.com/docs/installed-software/#selenium

@raytiley

raytiley Dec 5, 2016

Member

We use appveyor to test and chrome and firefox are pre-installed. It's under selenium section which is kinda weird: https://www.appveyor.com/docs/installed-software/#selenium

# Summary
Replace PhantomJS with Firefox as the default browser for continuous integration testing.

This comment has been minimized.

@stefanpenner

stefanpenner Dec 5, 2016

Contributor

should we consider chrome as a default (and make it easy to run FF etc secondary), as it is most likely to mimic the developers setup?

using chrome http://blog.500tech.com/setting-up-travis-ci-to-run-tests-on-latest-google-chrome-version/

@stefanpenner

stefanpenner Dec 5, 2016

Contributor

should we consider chrome as a default (and make it easy to run FF etc secondary), as it is most likely to mimic the developers setup?

using chrome http://blog.500tech.com/setting-up-travis-ci-to-run-tests-on-latest-google-chrome-version/

This comment has been minimized.

@ef4

ef4 Dec 5, 2016

Contributor

The major issue with that linked method is sudo: required. That prevents you from using Travis's faster, newer container-based infrastructure.

@ef4

ef4 Dec 5, 2016

Contributor

The major issue with that linked method is sudo: required. That prevents you from using Travis's faster, newer container-based infrastructure.

This comment has been minimized.

@Turbo87

Turbo87 Jun 26, 2017

Member

this is no longer the case apparently, see rust-lang/crates.io#813 for an example

@Turbo87

Turbo87 Jun 26, 2017

Member

this is no longer the case apparently, see rust-lang/crates.io#813 for an example

@nathanhammond

This comment has been minimized.

Show comment
Hide comment
@nathanhammond

nathanhammond Dec 5, 2016

Contributor

Given above conversation I feel like this RFC must specify method of transition for:

  • Circle
  • Travis
  • AppVeyor
  • (optional, I think) Hudson/Jenkins? (I think these are just .jar things you can run in any environment. Could get hilariously complicated.)

Are there others?

Upon addressing that I'm less opposed.

Contributor

nathanhammond commented Dec 5, 2016

Given above conversation I feel like this RFC must specify method of transition for:

  • Circle
  • Travis
  • AppVeyor
  • (optional, I think) Hudson/Jenkins? (I think these are just .jar things you can run in any environment. Could get hilariously complicated.)

Are there others?

Upon addressing that I'm less opposed.

```
before_script:
- export DISPLAY=:99; sh -e /etc/init.d/xvfb start; sleep 3

This comment has been minimized.

@stefanpenner

stefanpenner Dec 5, 2016

Contributor

@ef4 i think it might be nice to split this command over multiple separate steps:

  • if one fails, the next doesn't happen ; without && will cascade
  • easier to quickly read/understand
  • we can document the steps (such as, the sleep 3)

Is there a way to avoid the sleep 3, feels a tad racy?

@stefanpenner

stefanpenner Dec 5, 2016

Contributor

@ef4 i think it might be nice to split this command over multiple separate steps:

  • if one fails, the next doesn't happen ; without && will cascade
  • easier to quickly read/understand
  • we can document the steps (such as, the sleep 3)

Is there a way to avoid the sleep 3, feels a tad racy?

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Dec 5, 2016

Contributor

Given above conversation I feel like this RFC must specify method of transition for:

Circle
Travis
AppVeyor
(optional, I think) Hudson/Jenkins? (I think these are just .jar things you can run in any environment. Could get hilariously complicated.)
Are there others?

Upon addressing that I'm less opposed.

Is the concern is that by changing the default generated config we will accidentally brake custom CI setups of existing add-ons?

  • one option is to detect and only perform in travis via if (process.env.TRAVIS) { ... }
  • another is to consider custom CI environments as already not subscribing to defaults and it is up to that team to adjust during upgrade (as they likely already need to)
  • We may want to (as a different effort) document common CI configurations, similar to how we document some common deployment options.

Another option, is for testem.js to delegate to some node_module that can be configured, but attempts (optionally or by default) to detect best possible browser on a given host?

Contributor

stefanpenner commented Dec 5, 2016

Given above conversation I feel like this RFC must specify method of transition for:

Circle
Travis
AppVeyor
(optional, I think) Hudson/Jenkins? (I think these are just .jar things you can run in any environment. Could get hilariously complicated.)
Are there others?

Upon addressing that I'm less opposed.

Is the concern is that by changing the default generated config we will accidentally brake custom CI setups of existing add-ons?

  • one option is to detect and only perform in travis via if (process.env.TRAVIS) { ... }
  • another is to consider custom CI environments as already not subscribing to defaults and it is up to that team to adjust during upgrade (as they likely already need to)
  • We may want to (as a different effort) document common CI configurations, similar to how we document some common deployment options.

Another option, is for testem.js to delegate to some node_module that can be configured, but attempts (optionally or by default) to detect best possible browser on a given host?

# How We Teach This
In the guides, replace instructions for installing PhantomJS with instructions for installing Firefox. Since Firefox is a consumer-facing browser with widely-understood installers and behavior, this is one less intimidating thing for newbies to learn.

This comment has been minimized.

@stefanpenner

stefanpenner Dec 5, 2016

Contributor

based on @nathanhammond thoughts, i believe we will need to document not only how to use the default, but:

  • how to use one of the other popular browsers
  • how to use more then one
  • how to once again use phantomjs
@stefanpenner

stefanpenner Dec 5, 2016

Contributor

based on @nathanhammond thoughts, i believe we will need to document not only how to use the default, but:

  • how to use one of the other popular browsers
  • how to use more then one
  • how to once again use phantomjs
@jrjohnson

This comment has been minimized.

Show comment
Hide comment
@jrjohnson

jrjohnson Dec 6, 2016

We made this change about 8 months ago and are very happy with it. Two things of immediate and significant impact:

  1. Test which failed on Travis failed on developer machines. With Phantom we often experienced cases where failures on one platform could not be duplicated on the other which reduced our overall confidence in the entire test suite and made debugging some failures enough of a pain that the tendency was to just remove an otherwise good and working test.

  2. Tests stopped failing when installing Phantom. We would have at least one sometimes ten test runs a week fail because Phantom could not be downloaded. Firefox is already in the default Travis image so this went away entirely.

These are not small annoyances and I think the community would see real benefit from this change.

We made this change about 8 months ago and are very happy with it. Two things of immediate and significant impact:

  1. Test which failed on Travis failed on developer machines. With Phantom we often experienced cases where failures on one platform could not be duplicated on the other which reduced our overall confidence in the entire test suite and made debugging some failures enough of a pain that the tendency was to just remove an otherwise good and working test.

  2. Tests stopped failing when installing Phantom. We would have at least one sometimes ten test runs a week fail because Phantom could not be downloaded. Firefox is already in the default Travis image so this went away entirely.

These are not small annoyances and I think the community would see real benefit from this change.

@ef4

This comment has been minimized.

Show comment
Hide comment
@ef4

ef4 Dec 6, 2016

Contributor

I don't think that writing instructions for every other popular CI system is a reasonable requirement for this change. Especially since there's a one-line, self-explanatory change they will see in their diff:

< "PhantomJS",
> "Firefox",

when they upgrade that can easily be rejected to let them keep the browser they were using before. And because the downside of making the wrong choice is small: you'll see a test failure in CI, not ship a bug to end users.

Contributor

ef4 commented Dec 6, 2016

I don't think that writing instructions for every other popular CI system is a reasonable requirement for this change. Especially since there's a one-line, self-explanatory change they will see in their diff:

< "PhantomJS",
> "Firefox",

when they upgrade that can easily be rejected to let them keep the browser they were using before. And because the downside of making the wrong choice is small: you'll see a test failure in CI, not ship a bug to end users.

@nathanhammond

This comment has been minimized.

Show comment
Hide comment
@nathanhammond

nathanhammond Dec 6, 2016

Contributor

@ef4 This diff won't show up for the users I'm concerned about. The problem is that this change can break Phantom transitively. ember-my-addon depends on ember-somebody-else and ember-somebody-else moves to Firefox for CI and starts including code that breaks in Phantom.

Now, the Phantom-using ember-my-addon just had their CI break on them and they don't know why. They have to use http://package-hint-historic-resolver.herokuapp.com/ as their best option, but realistically they probably don't even know that it exists. They're already annoyed even before beginning to fix and have no idea what went wrong.

Once the problem has been identified (possibly hours of pain) for bonus points, let's say ember-my-addon has a lot of node stuff inside it which needs to be tested via AppVeyor. Now they either have to split the test suite (only run part of the tests on Windows, not a single-line-change) or they must figure out how to move their CI to a system that doesn't break (possibly not a single line change depending upon their system).

Or likely, in my case, I'd just open a PR to the upstream that would make it work in Phantom again since that's pretty straightforward at that point... and now we've no benefit for moving off of Phantom other than upstreaming support for it.

Regardless, considering the person who this is going to affect didn't opt in to this pain we should make it as easy as possible to get them out of it.

Contributor

nathanhammond commented Dec 6, 2016

@ef4 This diff won't show up for the users I'm concerned about. The problem is that this change can break Phantom transitively. ember-my-addon depends on ember-somebody-else and ember-somebody-else moves to Firefox for CI and starts including code that breaks in Phantom.

Now, the Phantom-using ember-my-addon just had their CI break on them and they don't know why. They have to use http://package-hint-historic-resolver.herokuapp.com/ as their best option, but realistically they probably don't even know that it exists. They're already annoyed even before beginning to fix and have no idea what went wrong.

Once the problem has been identified (possibly hours of pain) for bonus points, let's say ember-my-addon has a lot of node stuff inside it which needs to be tested via AppVeyor. Now they either have to split the test suite (only run part of the tests on Windows, not a single-line-change) or they must figure out how to move their CI to a system that doesn't break (possibly not a single line change depending upon their system).

Or likely, in my case, I'd just open a PR to the upstream that would make it work in Phantom again since that's pretty straightforward at that point... and now we've no benefit for moving off of Phantom other than upstreaming support for it.

Regardless, considering the person who this is going to affect didn't opt in to this pain we should make it as easy as possible to get them out of it.

@nathanhammond

This comment has been minimized.

Show comment
Hide comment
@nathanhammond

nathanhammond Dec 6, 2016

Contributor

Adding to this: it's entirely possible that the CI change and Phantom-bug introduction can be separated by months of commits and the upstream consumer of ember-somebody-else doesn't recognize that the testing change is what triggered their eventual breakage.

In conclusion: I'm in favor of adding Firefox but I'm not in favor of removing Phantom until we've been through many release cycles so that it has worked its way into the ecosystem. Either that or we need a better way to help people out of the CI hell we would thrusting them into.

Contributor

nathanhammond commented Dec 6, 2016

Adding to this: it's entirely possible that the CI change and Phantom-bug introduction can be separated by months of commits and the upstream consumer of ember-somebody-else doesn't recognize that the testing change is what triggered their eventual breakage.

In conclusion: I'm in favor of adding Firefox but I'm not in favor of removing Phantom until we've been through many release cycles so that it has worked its way into the ecosystem. Either that or we need a better way to help people out of the CI hell we would thrusting them into.

@nathanhammond

This comment has been minimized.

Show comment
Hide comment
@nathanhammond

nathanhammond Dec 6, 2016

Contributor

Note: if it's as easy to make this change as we say it is we could write a script to open PRs against all community addons and then my concerns are also addressed with that + a bit of time + overlapping test browser versions.

Contributor

nathanhammond commented Dec 6, 2016

Note: if it's as easy to make this change as we say it is we could write a script to open PRs against all community addons and then my concerns are also addressed with that + a bit of time + overlapping test browser versions.

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Dec 6, 2016

Contributor

We had a fairly long conversation about this in #dev-ember-cli this morning. It is my personal opinion that we do not have to couple this to a longer tail of upgrading addons (blueprint changes are definitely not considered "breaking" changes already), but I am willing to accept that some migration period is acceptable as long as it is very concrete.

Contributor

rwjblue commented Dec 6, 2016

We had a fairly long conversation about this in #dev-ember-cli this morning. It is my personal opinion that we do not have to couple this to a longer tail of upgrading addons (blueprint changes are definitely not considered "breaking" changes already), but I am willing to accept that some migration period is acceptable as long as it is very concrete.

The default alternative is to do nothing and keep PhantomJS.
Another alternative would be to pick Chrome, since it is a very popular browser. However, Chrome is not 100% open source, which complicates distribution. It's not built into Travis, and the popular methods of installing it there require users to opt into non-container-based images, which are heavier and slower to boot.

This comment has been minimized.

@blimmer

blimmer Dec 10, 2016

👍 to chrome - it's what I use on all of my Ember projects on Travis-CI. Understand the issues with it not being built into Travis, but maybe they can help us out since other CI platforms pre-install / provide an easy way to use it.

@blimmer

blimmer Dec 10, 2016

👍 to chrome - it's what I use on all of my Ember projects on Travis-CI. Understand the issues with it not being built into Travis, but maybe they can help us out since other CI platforms pre-install / provide an easy way to use it.

This comment has been minimized.

@jrjohnson

jrjohnson Apr 25, 2017

It isn't documented, but Chrome is part of the beta dist: trusty Travis environment (we're using it in our app now with no installation and sudo: false) so I think making this official probably isn't too big of a step.

@jrjohnson

jrjohnson Apr 25, 2017

It isn't documented, but Chrome is part of the beta dist: trusty Travis environment (we're using it in our app now with no installation and sudo: false) so I think making this official probably isn't too big of a step.

This comment has been minimized.

@rwwagner90

rwwagner90 Jun 5, 2017

This is correct. I do this in all my addons and no installations are needed:

sudo: required
dist: trusty
...
before_install:
  - export CHROME_BIN=chromium-browser
  - export DISPLAY=:99.0
  - sh -e /etc/init.d/xvfb start
@rwwagner90

rwwagner90 Jun 5, 2017

This is correct. I do this in all my addons and no installations are needed:

sudo: required
dist: trusty
...
before_install:
  - export CHROME_BIN=chromium-browser
  - export DISPLAY=:99.0
  - sh -e /etc/init.d/xvfb start
@kategengler

This comment has been minimized.

Show comment
Hide comment
@kategengler

kategengler Dec 15, 2016

I think this change is worthwhile. I tell anybody that asks me about a test failure in Phantom to switch to testing in a browser that their end users can actually use.

I have already run into many addons that already cause test failures in Phantom, so I can't see this introducing much more confusion than already exists, especially if the party line is "Don't use phantom". It is easy enough to switch an app to using Chrome or Firefox even before it is part of the default blueprint.

I think this change is worthwhile. I tell anybody that asks me about a test failure in Phantom to switch to testing in a browser that their end users can actually use.

I have already run into many addons that already cause test failures in Phantom, so I can't see this introducing much more confusion than already exists, especially if the party line is "Don't use phantom". It is easy enough to switch an app to using Chrome or Firefox even before it is part of the default blueprint.

@kellyselden

This comment has been minimized.

Show comment
Hide comment
@kellyselden

kellyselden Dec 20, 2016

Member

Came across an issue. Started seeing the usual phantom issue on travis "Can't find variable: Symbol". So I wanted to try out this RFC and used system installed firefox. But I got the same error "Symbol is not defined" because the system installed firefox on travis is an old firefox 31. Using most recent firefox locally works fine, and also installing chrome on travis works fine.

Is there an easy way to say "firefox:latest" on travis? otherwise I believe some of the benefits are lost.

Member

kellyselden commented Dec 20, 2016

Came across an issue. Started seeing the usual phantom issue on travis "Can't find variable: Symbol". So I wanted to try out this RFC and used system installed firefox. But I got the same error "Symbol is not defined" because the system installed firefox on travis is an old firefox 31. Using most recent firefox locally works fine, and also installing chrome on travis works fine.

Is there an easy way to say "firefox:latest" on travis? otherwise I believe some of the benefits are lost.

@stevesims

This comment has been minimized.

Show comment
Hide comment
@stevesims

stevesims Dec 22, 2016

From my perspective the fact that PhantomJS is being used for CI test runs is a good thing.

I do my development using Chrome, a practice that means that I'm in danger of writing code that is Blink+V8 specific. I keep /tests open in a separate browser tab to make sure my tests are passing, but rarely run ember test from the CLI.

Having my CI environment run the test suite against PhantomJS gives me some reassurance that my code should also work against WebKit, and that my code is thus more likely to work on a wider variety of browsers. Sure it's a bit irritating when the tests fail in PhantomJS but it's usually indicative of an issue that users may face when using my apps with a WebKit-based browser.

@kellyselden In my experience well over half of the problems I've encountered with PhantomJS get fixed by including the babel-polypill - I believe that would fix your Symbol problem.

From my perspective the fact that PhantomJS is being used for CI test runs is a good thing.

I do my development using Chrome, a practice that means that I'm in danger of writing code that is Blink+V8 specific. I keep /tests open in a separate browser tab to make sure my tests are passing, but rarely run ember test from the CLI.

Having my CI environment run the test suite against PhantomJS gives me some reassurance that my code should also work against WebKit, and that my code is thus more likely to work on a wider variety of browsers. Sure it's a bit irritating when the tests fail in PhantomJS but it's usually indicative of an issue that users may face when using my apps with a WebKit-based browser.

@kellyselden In my experience well over half of the problems I've encountered with PhantomJS get fixed by including the babel-polypill - I believe that would fix your Symbol problem.

@ef4

This comment has been minimized.

Show comment
Hide comment
@ef4

ef4 Dec 22, 2016

Contributor

@kellyselden you can do this in travis.yml

addons:
    firefox: latest
Contributor

ef4 commented Dec 22, 2016

@kellyselden you can do this in travis.yml

addons:
    firefox: latest
@kellyselden

This comment has been minimized.

Show comment
Hide comment
@kellyselden

kellyselden Dec 22, 2016

Member

In my experience well over half of the problems I've encountered with PhantomJS get fixed by including the babel-polypill - I believe that would fix your Symbol problem.

@stevesims Thank you. I quickly realized that was the problem, but it was a good test case for digging deeper into this RFC.

addons:
firefox: latest

@ef4 This is great. Worthy of including in this RFC in the changes needed section. Do you agree?

Member

kellyselden commented Dec 22, 2016

In my experience well over half of the problems I've encountered with PhantomJS get fixed by including the babel-polypill - I believe that would fix your Symbol problem.

@stevesims Thank you. I quickly realized that was the problem, but it was a good test case for digging deeper into this RFC.

addons:
firefox: latest

@ef4 This is great. Worthy of including in this RFC in the changes needed section. Do you agree?

# Drawbacks
PhantomJS has two primary benefits over other browsers: being headless and being scriptable.

This comment has been minimized.

@les2

les2 Jan 21, 2017

A third benefit is speed?

Random test against one of our production code bases:

ember test --launch AltPhantom  8.93s user 3.81s system 41% cpu 30.988 total
ember test --launch Chrome  33.42s user 9.23s system 123% cpu 34.476 total

I'm not a mathematician but I think 8.93 < 33.42 and that Chrome is about 3X slower than Phantom.

@les2

les2 Jan 21, 2017

A third benefit is speed?

Random test against one of our production code bases:

ember test --launch AltPhantom  8.93s user 3.81s system 41% cpu 30.988 total
ember test --launch Chrome  33.42s user 9.23s system 123% cpu 34.476 total

I'm not a mathematician but I think 8.93 < 33.42 and that Chrome is about 3X slower than Phantom.

@kanongil

This comment has been minimized.

Show comment
Hide comment
@kanongil

kanongil Mar 27, 2017

With ember-cli@2.13 and targets, phantomjs will now balk at non-ES5 compatible targets. As such, a move away from this is more urgent, and this RFC should probably be re-visited.

Sample error output:

not ok 1 PhantomJS 2.1 - Global error: SyntaxError: Unexpected token 'const' at http://localhost:7357/assets/vendor.js, line 77907
    ---
        Log: |
            { type: 'error',
              text: 'SyntaxError: Unexpected token \'const\' at http://localhost:7357/assets/vendor.js, line 77907\n' }
    ...

With ember-cli@2.13 and targets, phantomjs will now balk at non-ES5 compatible targets. As such, a move away from this is more urgent, and this RFC should probably be re-visited.

Sample error output:

not ok 1 PhantomJS 2.1 - Global error: SyntaxError: Unexpected token 'const' at http://localhost:7357/assets/vendor.js, line 77907
    ---
        Log: |
            { type: 'error',
              text: 'SyntaxError: Unexpected token \'const\' at http://localhost:7357/assets/vendor.js, line 77907\n' }
    ...
@kanongil

This comment has been minimized.

Show comment
Hide comment
@kanongil

kanongil Mar 27, 2017

I would like to put in another word for Chrome, which now has a --headless mode that works on Linux and macOS (Canary) without any Xvfb.

Another advantage, is that it currently supports more ES6 features than Firefox.

I would like to put in another word for Chrome, which now has a --headless mode that works on Linux and macOS (Canary) without any Xvfb.

Another advantage, is that it currently supports more ES6 features than Firefox.

@hugoruscitti

This comment has been minimized.

Show comment
Hide comment
@hugoruscitti

hugoruscitti Apr 8, 2017

I think another change to .travis.yml must be fix the firefox version or use the latest version instead:

addons:  
  firefox: latest

by default travis use the firefox version 31.0

I think another change to .travis.yml must be fix the firefox version or use the latest version instead:

addons:  
  firefox: latest

by default travis use the firefox version 31.0

@hjdivad

This comment has been minimized.

Show comment
Hide comment
@hjdivad

hjdivad Apr 13, 2017

Contributor

This RFC was discussed today in the weekly Ember CLI call and we decided to move it to FCP.

Contributor

hjdivad commented Apr 13, 2017

This RFC was discussed today in the weekly Ember CLI call and we decided to move it to FCP.

@jonblack

This comment has been minimized.

Show comment
Hide comment
@jonblack

jonblack Apr 14, 2017

Also worth mentioning the news that Vitaly Slobodin is stepping down as maintainer of phantomjs:

I want to make an announcement.

Headless Chrome is coming - https://www.chromestatus.com/features/5678767817097216
(https://news.ycombinator.com/item?id=14101233)

I think people will switch to it, eventually. Chrome is faster and more stable than PhantomJS. And it doesn't eat memory like crazy.

I don't see any future in developing PhantomJS. Developing PhantomJS 2 and 2.5 as a single developer is a bloody hell.
Even with recently released 2.5 Beta version with new and shiny QtWebKit, I can't physically support all 3 platforms at once (I even bought the Mac for that!). We have no support.
From now, I am stepping down as maintainer. If someone wants to continue - feel free to reach me.

That doesn't mean someone else won't pick up the project, but perhaps it's worth including in the discussion.

Also worth mentioning the news that Vitaly Slobodin is stepping down as maintainer of phantomjs:

I want to make an announcement.

Headless Chrome is coming - https://www.chromestatus.com/features/5678767817097216
(https://news.ycombinator.com/item?id=14101233)

I think people will switch to it, eventually. Chrome is faster and more stable than PhantomJS. And it doesn't eat memory like crazy.

I don't see any future in developing PhantomJS. Developing PhantomJS 2 and 2.5 as a single developer is a bloody hell.
Even with recently released 2.5 Beta version with new and shiny QtWebKit, I can't physically support all 3 platforms at once (I even bought the Mac for that!). We have no support.
From now, I am stepping down as maintainer. If someone wants to continue - feel free to reach me.

That doesn't mean someone else won't pick up the project, but perhaps it's worth including in the discussion.

@bartocc

This comment has been minimized.

Show comment
Hide comment
@bartocc

bartocc Apr 25, 2017

Guys, I see no mention of slimerjs in this RFC. Have you considered it ? It is based on firefox, is has the same API as phantomjs and supports ES2015.

bartocc commented Apr 25, 2017

Guys, I see no mention of slimerjs in this RFC. Have you considered it ? It is based on firefox, is has the same API as phantomjs and supports ES2015.

@kellyselden

This comment has been minimized.

Show comment
Hide comment
@kellyselden

kellyselden Apr 25, 2017

Member

@bartocc I think real browsers are getting good enough at running tests via command line that we would be going backwards if went with pseudo-real browser.

Member

kellyselden commented Apr 25, 2017

@bartocc I think real browsers are getting good enough at running tests via command line that we would be going backwards if went with pseudo-real browser.

@kellyselden

This comment has been minimized.

Show comment
Hide comment
@kellyselden

kellyselden May 25, 2017

Member

This has passed final comment period without any major issues. It is ready to be implemented!

Member

kellyselden commented May 25, 2017

This has passed final comment period without any major issues. It is ready to be implemented!

@kellyselden kellyselden merged commit c1f86ee into master May 25, 2017

@kellyselden kellyselden deleted the firefox branch May 25, 2017

@appleton

This comment has been minimized.

Show comment
Hide comment
@appleton

appleton May 26, 2017

I think I'm 12 hours too late, but could headless Chrome be a better solution here? It won't require xvfb, is supported by the latest stable chrome build and is pretty simple to configure.

If we're going to make a change to the default, I think it's certainly worth considering.

I think I'm 12 hours too late, but could headless Chrome be a better solution here? It won't require xvfb, is supported by the latest stable chrome build and is pretty simple to configure.

If we're going to make a change to the default, I think it's certainly worth considering.

@cibernox

This comment has been minimized.

Show comment
Hide comment
@cibernox

cibernox May 26, 2017

Contributor

@appleton I think that it requires Chrome 59, still in beta.

Contributor

cibernox commented May 26, 2017

@appleton I think that it requires Chrome 59, still in beta.

@appleton

This comment has been minimized.

Show comment
Hide comment
@appleton

appleton May 26, 2017

Apologies, I think it is in stable for Linux and is coming in 59 for Mac OS (which actually contradicts the info here slightly but I'm using the stable branch in Heroku CI with this buildpack successfully already).

Regardless, it'll be in stable very soon so should we consider it as an alternative?

Apologies, I think it is in stable for Linux and is coming in 59 for Mac OS (which actually contradicts the info here slightly but I'm using the stable branch in Heroku CI with this buildpack successfully already).

Regardless, it'll be in stable very soon so should we consider it as an alternative?

@webark

This comment has been minimized.

Show comment
Hide comment
@webark

webark May 26, 2017

the latest chromium has it too (easier to get on a linux box if you need to for internal ci things), but does travis support headless chrome yet?

webark commented May 26, 2017

the latest chromium has it too (easier to get on a linux box if you need to for internal ci things), but does travis support headless chrome yet?

@appleton

This comment has been minimized.

Show comment
Hide comment
@appleton

appleton May 26, 2017

does travis support headless chrome yet?

Looks like yes

does travis support headless chrome yet?

Looks like yes

@kellyselden

This comment has been minimized.

Show comment
Hide comment
@kellyselden

kellyselden May 26, 2017

Member

Regrading the switch to Chrome, there were some issues with that approach in the RFC:
https://github.com/ember-cli/rfcs/pull/86/files#r90848744
https://github.com/ember-cli/rfcs/pull/86/files#diff-5602aa7b333fe18741f01be58b92735aR54

Have they all been resolved over time? What about the open source concern? What would the changes to the .travis.yml look like?

Member

kellyselden commented May 26, 2017

Regrading the switch to Chrome, there were some issues with that approach in the RFC:
https://github.com/ember-cli/rfcs/pull/86/files#r90848744
https://github.com/ember-cli/rfcs/pull/86/files#diff-5602aa7b333fe18741f01be58b92735aR54

Have they all been resolved over time? What about the open source concern? What would the changes to the .travis.yml look like?

@kanongil

This comment has been minimized.

Show comment
Hide comment
@kanongil

kanongil May 28, 2017

@kellyselden Chrome --headless can be enabled in travis like I do here: kanongil/ember-stable-hash@103eea4...travis-chrome-test.

It uses unstable / canary build, since --headless is not supported in the stable build, and testem doesn't handle beta builds. This should change soon enough when v59 goes stable next week.

This also enables it for local development, and works on OS X once Google Chrome Canary is installed.

Edit: Updated for new stable Chrome.

kanongil commented May 28, 2017

@kellyselden Chrome --headless can be enabled in travis like I do here: kanongil/ember-stable-hash@103eea4...travis-chrome-test.

It uses unstable / canary build, since --headless is not supported in the stable build, and testem doesn't handle beta builds. This should change soon enough when v59 goes stable next week.

This also enables it for local development, and works on OS X once Google Chrome Canary is installed.

Edit: Updated for new stable Chrome.

@appleton

This comment has been minimized.

Show comment
Hide comment
@appleton

appleton May 30, 2017

The change I've used to make this work with Testem looks like this (this works with stable chrome under linux already):

module.exports = {
  ...
  "launch_in_ci": [
    "Chrome"
  ],
  'browser_args': {
    'Chrome': [
      '--headless',
      '--disable-gpu',
      '--no-sandbox',
      '--remote-debugging-port=9222'
    ]
  }
};

The change I've used to make this work with Testem looks like this (this works with stable chrome under linux already):

module.exports = {
  ...
  "launch_in_ci": [
    "Chrome"
  ],
  'browser_args': {
    'Chrome': [
      '--headless',
      '--disable-gpu',
      '--no-sandbox',
      '--remote-debugging-port=9222'
    ]
  }
};
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment