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

[5.0] Implement ChromeDriverCommand #643

Merged
merged 4 commits into from
Apr 29, 2019
Merged

[5.0] Implement ChromeDriverCommand #643

merged 4 commits into from
Apr 29, 2019

Conversation

driesvints
Copy link
Member

@driesvints driesvints commented Apr 26, 2019

This PR implements the functionality offered by https://github.com/staudenmeir/dusk-updater which was written by @staudenmeir.

This command will allow users to update their chrome driver for their OS. It has a flag that will also allow users to update every binary if they want. I've added this to the install command so the driver is updated whenever the install command is run. This means we don't have to update the docs.

I wrote this as an install command so we can remove the binaries in a future version of Dusk and always have them installed with this command. This will save having them to be packaged with the repo. It's also written in such a way that it only installs the binary for the current OS.

Fixes #641

@driesvints
Copy link
Member Author

@staudenmeir I'd greatly appreciate a review from you :)

@staudenmeir
Copy link
Contributor

Not a review, but things I've noticed:

  • The resources/legacy.json file is missing.
  • Can we reuse the OS detection from ChromeProcess (by moving it to a shared place)?
  • Since the code is basically a copy of my package, it should/must include some kind of copyright notice.

@driesvints
Copy link
Member Author

The resources/legacy.json file is missing.

I've added this as an array in the command.

Can we reuse the OS detection from ChromeProcess (by moving it to a shared place)?

Yeah I was thinking that as well. That's probably better indeed. I've pushed a class that re-uses this knowledge.

Since the code is basically a copy of my package, it should/must include some kind of copyright notice.

You're absolutely right. I've updated the license file.

This command will allow users to update their chrome driver for their OS. It has a flag that will also allow users to update every binary if they want. I've added this to the install command so the driver is updated whenever the install command is run. This means we don't have to update the docs.

I wrote this as an install command so we can remove the binaries in a future version of Dusk and always have them installed with this command. This will save having them to be packaged with the repo. It's also written in such a way that it only installs the binary for the current OS.

Fixes #641
@driesvints
Copy link
Member Author

@staudenmeir updated.

@driesvints
Copy link
Member Author

@staudenmeir thanks for all your work on this btw 👍

@staudenmeir
Copy link
Contributor

When I updated my package, I noticed a potential issue with the new ChromeDriver release strategy:
There will now be beta releases for upcoming Chrome versions and latestChromeVersion() might incorrectly select them as the latest version (technically correct, but not really useful).

We can't do anything about that at the moment, we just have to wait for the next release to see what the index page will look like.

@taylorotwell taylorotwell merged commit 899fa16 into laravel:5.0 Apr 29, 2019
@driesvints driesvints deleted the refactor-binaries branch April 29, 2019 13:30
@bobbybouwmann
Copy link
Contributor

bobbybouwmann commented May 9, 2019

@driesvints I did found some errors when running php artisan dusk after reinstalling the binaries

Facebook\WebDriver\Exception\SessionNotCreatedException: session not created: Chrome version must be between 70 and 73

So for me it installed the latest Chrome which is 74, as you also can see on the homepage: http://chromedriver.chromium.org/home

However the Facebook driver doesn't support this version just yet, at least that's what the error is telling me. I've been looking into the facebook\php-webdriver package but couldn't find where this check is done!

A work around for now was downloading an older version manually, but the command currently doesn't work whenever a new version is released and not updated in the php-webdriver package.

@driesvints
Copy link
Member Author

Hey @bobbybouwmann, did you read through #641 to see if any of the answers helps you?

So for me it installed the latest Chrome which is 74, as you also can see on the homepage

The error says Chrome version must be between 70 and 73 so did you try to install version 71 or 72? What's your current chrome browser version? Did you try to upgrade your Chrome browser and quit and restart afterwards? Perhaps even reboot your computer to make sure no processes are lingering anymore?

However the Facebook driver doesn't support this version just yet

I guess I can't help with this and you'll need to use a lower version for now...

@staudenmeir
Copy link
Contributor

However the Facebook driver doesn't support this version just yet, at least that's what the error is telling me.

Do you mean the error that you posted? This error is not caused by an incompatibility with the facebook/webdriver package.

@bobbybouwmann
Copy link
Contributor

bobbybouwmann commented May 11, 2019

@driesvints I had indeed 74 installed, but that was because of the php artisan dusk:chrome-driver command. I installed 72 and everything started working again! Just wanted to give a heads up!

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.

Update Chrome driver to support browser v74
4 participants