-
Notifications
You must be signed in to change notification settings - Fork 79
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
This modifications fix the use of regex with glob on windows. #89
base: master
Are you sure you want to change the base?
Conversation
Thank you for submitting this pull request, however I do not see a valid CLA on file for you. Before we can merge this request please visit https://yahoocla.herokuapp.com/ and agree to the terms. Thanks! 😄 |
The CLA was sent. |
Thanks! I'll review this by the end of tomorrow.
|
@@ -9,6 +9,7 @@ var TestSpecification = require("./test-specification"); | |||
var TestServer = require("./http/test-server"); | |||
|
|||
var WebDriverCollection = require("./webdriver-collection"); | |||
var path = require('path'); |
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 use double-quotes. Thank you!
CLA is valid! |
@@ -97,7 +97,7 @@ var Hub = module.exports = function Hub(options) { | |||
|
|||
this._setupEvents(); | |||
|
|||
this.mountpoint = "/"; | |||
this.mountpoint = '/'; |
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.
Same here.
This looks good. Please match existing quote style and I'll get this merged in and released. Thanks so much for fixing this! 😃 |
Hello @reid , all the changes were done and pushed. |
Hello @reid , Were you able to check the modifications? |
Thanks for the ping @jrubstein! I'll review this today. |
Hey @jrubstein, your changes passed tests locally with a old set of npm dependencies. I have attempted to test with a clean The hang is likely due to a new dependency being loaded that breaks the test suite. I will look into this over the weekend, but feel free to help if you can! Otherwise, I'll get back to you about what I find next week. |
@reid do you have any updates regarding this pull request? |
Getting the test by URL and finding the base file of the test to inject inject.js were done using the path as string. Since windows uses '' instead of '/' the script was never injected.