-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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 chrome driver path problem in Windows #416
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add comments/document to descript this magic line.
Thanks!
src/puppet-web/browser-driver.ts
Outdated
@@ -81,6 +81,8 @@ export class BrowserDriver { | |||
} | |||
*/ | |||
|
|||
require('chromedriver') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add comments above this line to memo the details?
Becasue it looks wired if without any description.
Anyway, good catch and nice patch. Thank you!
Comments have been added. But the test failed. I will try to figure it out later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Please let me know when you are ready. |
I don't know why they were timeout. |
So don't worry about the failed CI. Before the PR can be merged, we need at least 3 approvements from @Chatie/contributor. I had already approved, so you need 2 more. After that, we can do merge. :) Thank you very much! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code looks good for me.
@Chatie/contributor anyone who still using windows could check and approve this PR? |
After install from souce in Windows,
npm run demo
throws:I solved the problem in following steps:
chromedriver
installed correctly: these is no error whennpm install
andPROJECT_ROOT\node_modules\chromedriver\lib\chromedriver\chromedriver.exe
exists.npm run demo
succeeded if addingPROJECT_ROOT\node_modules\chromedriver\lib\chromedriver
dir to PATH env and failed if removingPROJECT_ROOT\node_modules\chromedriver\lib\chromedriver
dir from PATH env.So i am sure the problem is related to
PATH
im my circumstance. After some search, I found that https://www.npmjs.com/package/chromedriver#running-with-selenium-webdriver.When
require('chromedriver')
, it will do https://github.com/giggio/node-chromedriver/blob/master/lib/chromedriver.js#L2. SoPATH
problem solved.