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

Build: Add karma-chrome-launcher support #10898

Merged
merged 1 commit into from Oct 1, 2018

Conversation

ossdev07
Copy link
Contributor

@ossdev07 ossdev07 commented Sep 27, 2018

Removed phantomjs dependency from package.json and karma.conf.js.

Signed-off-by: ossdev07 ossdev@puresoftware.com

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix [template]
[ ] New rule [template]
[X] Changes an existing rule [template]
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)
Just added the "customLauncher" support which allow to use HeadlessChrome with --no-sandbox flag.

Is there anything you'd like reviewers to focus on?
Phantomjs is not supported anymore should use chrome instead.

@jsf-clabot
Copy link

jsf-clabot commented Sep 27, 2018

CLA assistant check
All committers have signed the CLA.

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Sep 27, 2018
Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I'm in support of this change, but I do have a question.

karma.conf.js Show resolved Hide resolved
@kaicataldo
Copy link
Member

One more thing: would you mind formatting the commit message following our guidelines? We ask this of committers so that we can keep the generation of our changelog automated.

@ossdev07 ossdev07 changed the title eslint: Replaced phantomJS by chrome eslint: Add karma-chrome-launcher support Sep 27, 2018
@ossdev07 ossdev07 changed the title eslint: Add karma-chrome-launcher support New: Add karma-chrome-launcher support Sep 27, 2018
Removed phantomjs dependency from package.json and
karma.conf.js.

Signed-off-by: ossdev07 <ossdev@puresoftware.com>
@not-an-aardvark not-an-aardvark added build This change relates to ESLint's build process chore This change is not user-facing and removed triage An ESLint team member will look at this issue soon labels Sep 28, 2018
Copy link
Member

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@not-an-aardvark
Copy link
Member

Note to whoever merges this: Since this just affects the tests and isn't user-facing, the commit prefix should be changed to Build: rather than New:.

@aladdin-add aladdin-add changed the title New: Add karma-chrome-launcher support Build: Add karma-chrome-launcher support Sep 30, 2018
@aladdin-add
Copy link
Member

Thanks for the PR!

Is something like chrome/puppeteer required?(I'm not seeing it in devdeps).

@ossdev07
Copy link
Contributor Author

ossdev07 commented Oct 1, 2018

Hi @aladdin-add

You can add puppeteer in the devdeps or you can manually download chromium.
In both scenerios --no-sandbox must be provided.
Thanks!

@not-an-aardvark not-an-aardvark merged commit 9bc3f7c into eslint:master Oct 1, 2018
not-an-aardvark added a commit that referenced this pull request Oct 12, 2018
btmills added a commit that referenced this pull request Oct 12, 2018
aladdin-add added a commit that referenced this pull request Oct 17, 2018
aladdin-add added a commit to aladdin-add/eslint that referenced this pull request Oct 17, 2018
@ossdev07
Copy link
Contributor Author

Hi @not-an-aardvark

Thanks for accepting this PR.
But the changes suggested here are still not reflected in the master branch.
May I know the reason for this, also if you want any further explanation regarding the changes I
proposed I won't mind discussing that with you.
Thanks!

@not-an-aardvark
Copy link
Member

The commit in this PR was reverted in #10973 because it was breaking the build on a few platforms. For example, the tests on our Jenkins release server failed with the following error:

START:
�[33m12 10 2018 15:09:14.220:WARN [watcher]: �[39mAll files matched by "/var/lib/jenkins/workspace/Releases/ESLint Release/eslint/node_modules/mocha/mocha.js" were excluded or matched by prior matchers.
�[32m12 10 2018 15:09:15.937:INFO [karma]: �[39mKarma v3.0.0 server started at http://0.0.0.0:9876/
�[32m12 10 2018 15:09:15.938:INFO [launcher]: �[39mLaunching browser HeadlessChrome with unlimited concurrency
�[32m12 10 2018 15:09:15.945:INFO [launcher]: �[39mStarting browser ChromeHeadless
�[91m12 10 2018 15:09:15.946:ERROR [launcher]: �[39mNo binary for ChromeHeadless browser on your platform.
  Please, set "CHROME_BIN" env variable.

Finished in 0 secs / 0 secs @ 15:09:15 GMT-0400 (Eastern Daylight Time)


npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! eslint@5.6.1 ci-release: `node Makefile.js ciRelease`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the eslint@5.6.1 ci-release script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

@platinumazure also reported that this change caused the tests to start failing for him locally -- he might be able to give more information about what's going on.

Sorry, I had meant to leave an update on this PR when I reverted the change, but I forgot to do so.

@ossdev07
Copy link
Contributor Author

@not-an-aardvark I understand your concern.
But from the error message which you attached, I can see that npm err is caused because the CHROME_BIN path is not set.
We need to export the chromium binary path for example,

export CHROME_BIN=/usr/bin/chromium-browser (path to your chromium binary).

I do understand that there must be some other constraints as well which are causing build to fail as you mentioned @platinumazure has reported, I won't mind looking into the issues which are causing builds to breaks on the specific platforms.
I would really like to see phantomjs removed from eslint.
Thanks again for your support!

@not-an-aardvark
Copy link
Member

To clarify, does this require Chromium to be installed on the machine independently of ESLint? I was under the impression that all the necessary pieces would get installed by npm.

@ossdev07
Copy link
Contributor Author

@not-an-aardvark Thanks for asking well there are two ways basically by which we can provide chromium binary:

  1. Install Chromium on your machine for example in ubuntu
apt-get install chromium-browser

After that we need to export path to chromium binary as i mentioned like

export CHROME_BIN=/usr/bin/chromium-browser (path to our chromium binary).
  1. We can define puppeteer in package.json as an devdependency.Puppeteer download chromium inside node-modules with npm install. We need to set the path to chromium binary inside karma.conf.js.

I will be happy to support if you need me to do any modification in the PR.

@ossdev07
Copy link
Contributor Author

Ping @not-an-aardvark @aladdin-add

Hey, Sorry to bother you again!
Please, could you tell me what it would take to accept the changes suggested by this PR?
Please do revert if any support is required at my end, I will be happy to help.

@aladdin-add
Copy link
Member

aladdin-add commented Nov 26, 2018

Hopefully the build failing can be fixed by #11027.

@ossdev07
Copy link
Contributor Author

Thanks for the quick update @aladdin-add

not-an-aardvark pushed a commit that referenced this pull request Jan 4, 2019
* Revert "Revert "Build: Use karma-chrome-launcher to run tests (#10898)" (#10973)"

This reverts commit 536611a.

* Build: add devdeps puppeteer@1.9.0

* Chore: set the path to chromium binary
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Mar 31, 2019
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Mar 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion build This change relates to ESLint's build process chore This change is not user-facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants