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

Replace PhantomJS by chromiumHeadless and puppeteer #6377

Merged

Conversation

@PierreBesson
Copy link
Contributor

commented Sep 18, 2017

PhantomJS deprecation announcement: https://groups.google.com/forum/m/#!topic/phantomjs/9aI5d-LDuNE
Last release of PhantomJS: v2.1.1 Jan 24, 2016

This is working great in my tests, hopefully we will even notice performance improvements in Travis

  • Travis tests are green
  • Tests are added where necessary
  • Documentation is added/updated where necessary
  • Coding Rules & Commit Guidelines as per our CONTRIBUTING.md document are followed

@PierreBesson PierreBesson changed the title replace PhantomJS by chromiumHeadless and puppeteer Replace PhantomJS by chromiumHeadless and puppeteer Sep 18, 2017

@@ -151,12 +150,8 @@
"webpack:prod": "<%= clientPackageManager %> run cleanup && <%= clientPackageManager %> run webpack:prod:main && <%= clientPackageManager %> run clean-www",
"webpack:test": "<%= clientPackageManager %> run test",
"webpack-dev-server": "node --max_old_space_size=4096 node_modules/webpack-dev-server/bin/webpack-dev-server.js",
"webpack": "node --max_old_space_size=4096 node_modules/webpack/bin/webpack.js",
<%_ if (protractorTests) { _%>
"webpack": "node --max_old_space_size=4096 node_modules/webpack/bin/webpack.js"<% if (protractorTests) { %>,

This comment has been minimized.

Copy link
@deepu105

deepu105 Sep 18, 2017

Member

I think the <%_ variant of template is more readable

This comment has been minimized.

Copy link
@PierreBesson

PierreBesson Sep 18, 2017

Author Contributor

Agree with this. I had to use the other way to deal with trailing commas in the JSON. Do you know another way to do it.
[EDIT] I just realized this has nothing to do with it. I will correct this.

@@ -18,6 +18,11 @@
-%>
const webpackConfig = require('../../../webpack/webpack.test.js');

var ChromiumRevision = require('puppeteer/package.json').puppeteer.chromium_revision;

This comment has been minimized.

Copy link
@deepu105

deepu105 Sep 18, 2017

Member

use const please

'phantomjs.binary.path': require('phantomjs-prebuilt').path,
'phantomjs.ghostdriver.cli.args': ['--loglevel=DEBUG']

chromeOptions: {

This comment has been minimized.

Copy link
@deepu105

deepu105 Sep 18, 2017

Member

formatting seems off

@gmarziou

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2017

I tested on Windows 10 and I got 2 errors

on yarn install:

"ENOENT: no such file or directory, open 'e:\\projets\\issues\\phantom\\node_modules\\generator-jhipster\\node_modules\\ansi-styles\\index.js'"

and karmacommand not found when run yarn run test.

console output
yarn install v1.0.2
info No lockfile found.
[1/5] Validating package.json...
[2/5] Resolving packages...
warning express@2.5.11: express 2.x series is deprecated
warning connect@1.9.2: connect 1.x series is deprecated
[3/5] Fetching packages...
info fsevents@1.1.2: The platform "win32" is incompatible with this module.
info "fsevents@1.1.2" is an optional dependency and failed compatibility check. Excluding it from installation.
[4/5] Linking dependencies...
warning "bootstrap@4.0.0-beta" has unmet peer dependency "popper.js@^1.11.0".
warning "@ngx-translate/http-loader@0.1.0" has incorrect peer dependency "@ngx-translate/core@>=6.0.0".
warning "@ngtools/webpack@1.5.5" has incorrect peer dependency "enhanced-resolve@^3.1.0".
warning "extract-text-webpack-plugin@2.1.2" has incorrect peer dependency "webpack@^2.2.0".
warning "less-loader@4.0.5" has incorrect peer dependency "less@^2.3.1".
warning "sass-loader@6.0.6" has incorrect peer dependency "node-sass@^4.0.0".
warning "stylus-loader@3.0.1" has incorrect peer dependency "stylus@>=0.52.4".
warning "webpack-dev-server@2.4.5" has incorrect peer dependency "webpack@^2.2.0".
warning "ajv-keywords@1.5.1" has incorrect peer dependency "ajv@>=4.10.0".
warning "marked-terminal@1.7.0" has incorrect peer dependency "marked@^0.3.3".
warning "ajv-keywords@2.1.0" has incorrect peer dependency "ajv@>=5.0.0".
warning "react-dom@0.14.9" has incorrect peer dependency "react@^0.14.9".
error An unexpected error occurred: "ENOENT: no such file or directory, open 'e:\\projets\\issues\\phantom\\node_modules\\generator-jhipster\\node_modules\\ansi-styles\\index.js'".
info If you think this is a bug, please open a bug report with the information provided in "e:\\projets\\issues\\phantom\\yarn-error.log".
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
WARNING! Install of dependencies failed!
To install your dependencies manually, run: yarn install

Server application generated successfully.

Run your Spring Boot application:
./mvnw (mvnw if using Windows Command Prompt)

Client application generated successfully.

Start your Webpack development server with:
yarn start

Execution complete

e:\projets\issues\phantom>yarn run test
yarn run test v1.0.2
$ karma start src/test/javascript/karma.conf.js
'karma' n’est pas reconnu en tant que commande interne
ou externe, un programme exécutable ou un fichier de commandes.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

@PierreBesson

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2017

We definitely must check that it works on Windows as well. However I'm not sure if your issue is a yarn problem or a karma + headless chromium issue.

@gmarziou

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2017

As node_modules\karma\bin folder was empty, I ran yarn again and it fixed the issue.
Now the tests pass using headless chrome.

@jdubois

This comment has been minimized.

Copy link
Member

commented Sep 20, 2017

This is a huge change, so I'm rather merge this soon: as we just did a release, that gives us time to test and experiment until the next release

@PierreBesson PierreBesson force-pushed the PierreBesson:migrate-phantomjs-to-chromium-headless branch from e534448 to fe51f9e Sep 20, 2017

@PierreBesson

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2017

I have fixed the formatting problems. Once tests pass this is ready to merge I think. Then we must make sure that it works everywhere but it is surprisingly reliable in my tests.

@gmarziou

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2017

Have you noticed any performance in test duration?

@jdubois

This comment has been minimized.

Copy link
Member

commented Sep 21, 2017

  • works fine on my Mac
  • performance is much better

I restarted the build, and this needs to be tested on Windows, but for me we are good to go (as the next release won't be soon, this leaves time to test)

@jdubois jdubois merged commit 5217850 into jhipster:master Sep 21, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gmarziou

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2017

I will not be able to re-run the test on WIndows as my SSD died this morning. Back to Linux :)

@jdubois

This comment has been minimized.

Copy link
Member

commented Sep 22, 2017

Tested on Windows 7:

  • No problem!!
  • More than 2x as fast!!!

For me we are good to go :-)

@jdubois jdubois added this to the 4.9.0 milestone Sep 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.