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

Change browser from Chrome to ChromeHeadless #700

Merged
merged 1 commit into from
Nov 28, 2018

Conversation

ossdev07
Copy link
Contributor

Changing the browser from Chrome to ChromeHeadless

As when using chromium binary to launch chrome then getting issues

15 10 2018 09:29:34.292:ERROR [launcher]: Cannot start Chrome

15 10 2018 09:29:34.292:ERROR [launcher]: Chrome stdout:
15 10 2018 09:29:34.292:ERROR [launcher]: Chrome stderr:
15 10 2018 09:29:34.298:INFO [launcher]: Trying to start Chrome again (1/2).
15 10 2018 09:29:35.145:ERROR [launcher]: Cannot start Chrome

15 10 2018 09:29:35.145:ERROR [launcher]: Chrome stdout:
15 10 2018 09:29:35.146:ERROR [launcher]: Chrome stderr:
15 10 2018 09:29:35.146:INFO [launcher]: Trying to start Chrome again (2/2).
15 10 2018 09:29:35.993:ERROR [launcher]: Cannot start Chrome

15 10 2018 09:29:35.994:ERROR [launcher]: Chrome stdout:
15 10 2018 09:29:35.994:ERROR [launcher]: Chrome stderr:
15 10 2018 09:29:35.995:ERROR [launcher]: Chrome failed 2 times (cannot start). Giving up.

But, if using chromeHeadless in place of Chrome then the browser launches

Headless browsers provide automated control of a web page in an environment similar to popular web browsers, but are executed via a command-line interface or using network communication

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

Fixes #[issue number].

Changes proposed:
Changing browser from chrome to chrome headless

Upgrade Path (for changed or removed APIs):

Acceptance Checklist:

  • The commit message follows the guidelines in CONTRIBUTING.md.
  • Documentation (README.md) and examples have been updated as needed.
  • If this is a code change, a spec testing the functionality has been added.
  • If the commit message has [changed] or [removed], there is an upgrade path above.

@coveralls
Copy link

coveralls commented Oct 15, 2018

Coverage Status

Coverage remained the same at 86.56% when pulling 68560e2 on ossdev07:chrome_headless into fc53400 on reactjs:master.

@ossdev07
Copy link
Contributor Author

Hi @diasbruno

Please have a look at the PR.

If any more information required for it, let me know, it will be a delight to see the chrome headless support.

Thanks,

@diasbruno
Copy link
Collaborator

@ossdev07 Sorry for the delay. I'll have a look on all PRs this weekend. Thanks!

@diasbruno
Copy link
Collaborator

@ossdev07 There are no changes to the package.json, so I think you can remove the changes to the package-lock.json.

Maybe we can change the browser using environment variables. The default browser can be ChromeHeadless.

$ USE_BROWSER=Chrome npm run test

What do you think?

@ossdev07
Copy link
Contributor Author

Thanks, @diasbruno for your suggestion,

Yes, there are no changes in the package.json file, so we can remove the changes from the package-lock.json.

As per your guidance, I tried to change the browser using environment variables, but I am getting the same error, which I have previously mentioned.

15 10 2018 09:29:34.292:ERROR [launcher]: Cannot start Chrome

15 10 2018 09:29:34.292:ERROR [launcher]: Chrome stdout:
15 10 2018 09:29:34.292:ERROR [launcher]: Chrome stderr:
15 10 2018 09:29:34.298:INFO [launcher]: Trying to start Chrome again (1/2).
15 10 2018 09:29:35.145:ERROR [launcher]: Cannot start Chrome

15 10 2018 09:29:35.145:ERROR [launcher]: Chrome stdout:
15 10 2018 09:29:35.146:ERROR [launcher]: Chrome stderr:
15 10 2018 09:29:35.146:INFO [launcher]: Trying to start Chrome again (2/2).
15 10 2018 09:29:35.993:ERROR [launcher]: Cannot start Chrome

15 10 2018 09:29:35.994:ERROR [launcher]: Chrome stdout:
15 10 2018 09:29:35.994:ERROR [launcher]: Chrome stderr:
15 10 2018 09:29:35.995:ERROR [launcher]: Chrome failed 2 times (cannot start). Giving up.

Please correct me if I am missing something, I have tried to set the envirnoment variable for chromeheadless in package.json file, and via terminal too.

But, every time i am getting same error.

As, I have mentioned in the PR, if we change the browser from Chrome to ChromeHeadless in karma.conf.js file, in that case, the test cases are passing.

let browsers = ['ChromeHeadless'];

Please share your views reagrding it.

@ossdev07
Copy link
Contributor Author

Ping @diasbruno

@diasbruno
Copy link
Collaborator

One way we can do this:

$ # shell part
$ USE_BROWSER=firefox,chrome npm run tests
let browsers = ['ChromeHeadless'];
if (process.env.USE_BROWSER) {
  browsers =  process.env.USE_BROWSER.split(',');
}

@ossdev07
Copy link
Contributor Author

Hi @diasbruno

Yes, I think it LGTM.

If you want I can update this PR accordingly, or can you handle this at your end?

@diasbruno
Copy link
Collaborator

You can add to this patch.

@ossdev07
Copy link
Contributor Author

@diasbruno As per our discussion, I have updated the patch.
Please review it once

@diasbruno
Copy link
Collaborator

To rollback those changes to package-lock.json you can use:

git checkout master -- package-lock.json,

then you can amend to the commit.

@ossdev07
Copy link
Contributor Author

Hi @diasbruno

Thanks, for your reply.
In the latest commit, commit a65ffa42b3b45d6d1bb90a09abaf2f76cb04aa9a the updated file of karma.conf is present with the suggested changes.

Please, consider it.

@ossdev07
Copy link
Contributor Author

Ping @diasbruno

Hey! Sorry to bother you again.
Could you Please have a look at the changes and consider it for a merge.
I will be happy to help if you need me to perform any further change to the PR.
It will be a delight to see chrome Headless support for react-modal.

@diasbruno
Copy link
Collaborator

No problem, @ossdev07. 👍

The last thing for this PR is to remove the changes to package-lock.json.
You can use the git command above to revert the change.

Let me know if I can provide an assistance.

Change frome chrome to chromeheadless

Signed-off-by: ossdev <ossdev@puresoftware.com>
@ossdev07
Copy link
Contributor Author

Hi @diasbruno

I have reverted the changes.
Please review and consider it for merge.

@diasbruno
Copy link
Collaborator

Looks good.

Thank you for your patience with this issue and good job.

@ossdev07
Copy link
Contributor Author

@diasbruno Thanks, 😃

Any update on the merge.?

@diasbruno diasbruno merged commit 8d8f476 into reactjs:master Nov 28, 2018
@ossdev07
Copy link
Contributor Author

Thanks @diasbruno for accepting the PR 😃

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.

None yet

3 participants