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

fix(cli): pass chromedriver-path arg to webdriver #416

Merged
merged 3 commits into from
Jan 10, 2022

Conversation

Zidious
Copy link
Contributor

@Zidious Zidious commented Jan 5, 2022

We were not passing the --chromedriver-path arg value to webdriver.

Within webdriver.ts config.chromedriverPath would produce undefined even if --chromedriver-path was set which would result in using the default chrome driver and not the one set by the user.

image

If we add chromedriverPath to the destructured args and a user provides an invalid path we do error correctly throw the error as well as use the given Chrome driver:

Invalid Path:

image

Valid Path:

image

Note: --chromedriver-path does not seem to support backwards compatibility in terms of Chrome Driver, it depends on your personal Chrome version. Example, if you're using V96 you cannot use V95 chrome driver, otherwise you will get a session error (as shown in the issue referenced).

Closes issue: #370

@Zidious Zidious marked this pull request as ready for review January 5, 2022 19:50
@Zidious Zidious requested a review from a team January 5, 2022 19:51
@michael-siek
Copy link
Member

We should add a test for this.

@Zidious
Copy link
Contributor Author

Zidious commented Jan 7, 2022

We should add a test for this.

I've added a test for checking when the driver path does not exist. I am not sure if we can test if the driver path does exist and run CLI. would require us to have a Chrome driver executable somewhere.

@Zidious Zidious merged commit 14e5125 into develop Jan 10, 2022
@Zidious Zidious deleted the fix/cli-chromedriverpath branch January 10, 2022 13:09
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.

3 participants