Skip to content
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

Switch from PhantomJS to Chrome Headless via Puppeteer #269

Merged
merged 17 commits into from
May 20, 2018

Conversation

stdavis
Copy link
Contributor

@stdavis stdavis commented May 17, 2018

Closes #257

This PR tears out PhantomJS (no longer maintained) and replaces it with Chrome Headless via the Puppeteer project. Our office got tired of fighting with PhamtomJS.

Here are the highlights of the changes:

  • Removed all references to phantomjs (not quite there yet)
  • Upgrade from jshint to eslint (required for the async/await code IIRC)
  • Cleaned up a bunch of apparently unused jasmine files
  • Upgraded min node version from 0.10.0 to 7.6.0 (required for async/await)

Let me and/or @steveoh know if you think that this has a chance to be merged and what we can do to make that happen.

Thanks for the great project!

stdavis and others added 8 commits May 17, 2018 08:09
@jsf-clabot
Copy link

jsf-clabot commented May 17, 2018

CLA assistant check
All committers have signed the CLA.

.travis.yml Outdated
@@ -3,8 +3,10 @@ sudo: false
language: node_js

node_js:
- "4"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can probably drop 4, it's EOL

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably drop 6 then also?

README.md Outdated
@@ -1,4 +1,4 @@
# grunt-contrib-jasmine v1.1.0 [![Build Status: Linux](https://travis-ci.org/gruntjs/grunt-contrib-jasmine.svg?branch=master)](https://travis-ci.org/gruntjs/grunt-contrib-jasmine) [![Build Status: Windows](https://ci.appveyor.com/api/projects/status/5985958by5rhnh31/branch/master?svg=true)](https://ci.appveyor.com/project/gruntjs/grunt-contrib-jasmine/branch/master)
# grunt-contrib-jasmine v1.2.0 [![Build Status: Linux](https://travis-ci.org/gruntjs/grunt-contrib-jasmine.svg?branch=master)](https://travis-ci.org/gruntjs/grunt-contrib-jasmine) [![Build Status: Windows](https://ci.appveyor.com/api/projects/status/5985958by5rhnh31/branch/master?svg=true)](https://ci.appveyor.com/project/gruntjs/grunt-contrib-jasmine/branch/master)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can just v2 this probably due to major change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a major change but it doesn’t break the current api.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll revert this and let the versioning get handled by the merging folks.

appveyor.yml Outdated
@@ -5,12 +5,16 @@ version: "{build}"
# What combinations to test
environment:
matrix:
- nodejs_version: "6"
- nodejs_version: "4"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same, drop 4

package.json Outdated
@@ -9,25 +9,25 @@
"repository": "gruntjs/grunt-contrib-jasmine",
"license": "MIT",
"engines": {
"node": ">=0.10.0"
"node": ">=7.6.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs to be >=6 , possible?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wellll......

Caution: Puppeteer requires at least Node v6.4.0, but the examples below use async/await which is only supported in Node v7.6.0 or greater.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see what happened, the ci files were auto generated from internal. So by adding this we would probably say that this supports node >=8 only

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

tasks/jasmine.js Outdated

await page.exposeFunction('jasmine.done_fail', function(url) {
grunt.log.error();
grunt.warn('PhantomJS unable to load "' + url + '" URI.', 90);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this still says phantomjs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted

stdavis added a commit to agrc/electrofishing that referenced this pull request May 17, 2018
temporarily until gruntjs/grunt-contrib-jasmine#269 is merged

Other misc test cleanup
@steveoh
Copy link
Contributor

steveoh commented May 17, 2018

@vladikoff do you have any ideas for the The http2 module is an experimental API? It's failing in the current v8 master branch for the same reason.

stdavis added a commit to agrc/electrofishing that referenced this pull request May 17, 2018
* Remove NaN values form number fields in grids

Closes #119

* Fix [other] option bug in grid selects

Related to #75

* Prevent entering an existing option in the "Other" dialog

Closes #75

* switch to agrc/grunt-contrib-jasmine

temporarily until gruntjs/grunt-contrib-jasmine#269 is merged

Other misc test cleanup

* Upgrade permissions on travis to help with Chrome Headless

travis-ci/travis-ci#8836

* fix tests
@stdavis
Copy link
Contributor Author

stdavis commented May 17, 2018

I was able to get the travis build running for our fork's repo: https://travis-ci.org/agrc/grunt-contrib-jasmine

I can't think of anything that would be different other than the cache. Perhaps you want to manually clear the cache in travis for this project?

As for the AppVeyor failure, it may be related to this: gruntjs/grunt-contrib-connect#235. Not sure if you want to add the environment variable workaround or something different.

@steveoh
Copy link
Contributor

steveoh commented May 18, 2018

Yeah, @vladikoff maybe try deleting the build caches

@stdavis
Copy link
Contributor Author

stdavis commented May 18, 2018

I think that I have all referenced to PhantomJS replaced with Headless Chrome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants