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

Get ChromeDriver's latest release in Robo task. #7345

Conversation

connorshea
Copy link
Contributor

@connorshea connorshea commented Jun 6, 2019

Description

This resolves one of the problems in #7344.

The Robo task will now automatically install the latest version of ChromeDriver instead of installing a static version.

Also improves wording/grammar in various places.

Motivation and Context

The ChromeDriver Robo task was stuck at an older version of ChromeDriver, 2.38. This isn't compatible with the latest release of Chrome, so it doesn't work unless you're on an older version of Chrome.

The LATEST_RELEASE URL system is based on how the webdrivers gem installs ChromeDriver, and also based on the ChromeDriver documentation.

How To Test This

Install ChromeDriver with the Robo task (./vendor/bin/robo chromedriver:install) and then make sure it runs properly (./vendor/bin/robo chromedriver:run). It should install ChromeDriver 75. You'll need to delete the existing build/tmp/webdrivers/ directory if you have one, or use ./vendor/bin/robo chromedriver:install --reinstall to delete the existing directory.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Final checklist

  • My code follows the code style of this project found here.
  • My change requires a change to the documentation.
  • I have read the How to Contribute guidelines.

@connorshea connorshea force-pushed the improve-chrome-webdriver-robo-task branch from 682099f to ad5a31d Compare June 6, 2019 22:18
@connorshea connorshea mentioned this pull request Jun 6, 2019
16 tasks
@codecov-io
Copy link

codecov-io commented Jun 7, 2019

Codecov Report

❗ No coverage uploaded for pull request base (hotfix-7.10.x@be18dab). Click here to learn what that means.
The diff coverage is 0%.

@@               Coverage Diff                @@
##             hotfix-7.10.x    #7345   +/-   ##
================================================
  Coverage                 ?    7.49%           
================================================
  Files                    ?     3741           
  Lines                    ?   387063           
  Branches                 ?        0           
================================================
  Hits                     ?    29025           
  Misses                   ?   358038           
  Partials                 ?        0

@connorshea
Copy link
Contributor Author

@Dillon-Brown Updated

Dillon-Brown
Dillon-Brown previously approved these changes Jun 7, 2019
@connorshea connorshea mentioned this pull request Jun 7, 2019
The Robo task will now automatically install the latest version of
ChromeDriver instead of installing a static version.

Also improves wording/grammar in various places.
@connorshea connorshea force-pushed the improve-chrome-webdriver-robo-task branch from a7cc2e4 to 95c4179 Compare June 7, 2019 16:33
@connorshea
Copy link
Contributor Author

Rebased to squash the commits and to get Travis to re-run and hopefully not flake out this time.

Can be used to delete and reinstall the web driver. Also added a bit
more logging and documentation.
@connorshea
Copy link
Contributor Author

I added a --reinstall flag since ChromeDriver 75 came out today and I wanted to update to it without needing to manually delete the tmp directory :) I'll also add it to the documentation.

@Dillon-Brown Dillon-Brown added this to the 7.11.6 milestone Jun 10, 2019
lib/Robo/Plugin/Commands/TestEnvironmentCommands.php Outdated Show resolved Hide resolved
}
} else if ($opts['reinstall']) {
$this->_deleteDir($basePath);
if (mkdir($basePath, 0777, true) === false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (mkdir($basePath, 0777, true) === false) {
if (mkdir($basePath, 0777, true) || is_dir($basePath)) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Dillon-Brown This caused problems because if mkdir worked, the exception would be thrown. It'd also cause is_dir to return true, which also causes the exception.

lib/Robo/Plugin/Commands/TestEnvironmentCommands.php Outdated Show resolved Hide resolved
@connorshea
Copy link
Contributor Author

Updated 👍

Dillon-Brown
Dillon-Brown previously approved these changes Jun 10, 2019
@connorshea connorshea mentioned this pull request Jun 11, 2019
6 tasks
@Dillon-Brown Dillon-Brown added the PR:Community Contribution These are contribution made by the community label Jun 12, 2019
@connorshea
Copy link
Contributor Author

Two notes:

@Mac-Rae Mac-Rae added the Status: Requires Code Review Needs the core team to code review label Jun 17, 2019
Renames the commands to chromedriver:install and chromedriver:run.
@connorshea
Copy link
Contributor Author

connorshea commented Jun 17, 2019

I've split the command into two.

Also, this can probably be squashed when merged.

@connorshea
Copy link
Contributor Author

It seems I got pretty unlucky with flaky tests on this one :P

Nothing I changed should effect the tests, and they match the flaky tests I've catalogued elsewhere.

@connorshea
Copy link
Contributor Author

I think this is ready to go now.

@connorshea
Copy link
Contributor Author

@Dillon-Brown any chance you could review? 😇

@Dillon-Brown Dillon-Brown removed the Status: Requires Code Review Needs the core team to code review label Jul 4, 2019
@cameronblaikie
Copy link
Contributor

Assessed 👍

@cameronblaikie cameronblaikie added the Status:Assessed PRs that have been tested and confirmed to resolve an issue by a core team member label Jul 4, 2019
@Dillon-Brown Dillon-Brown merged commit a0d5f5a into salesagility:hotfix-7.10.x Jul 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR:Community Contribution These are contribution made by the community Status:Assessed PRs that have been tested and confirmed to resolve an issue by a core team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants