-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Only download chrome driver #18676
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
Only download chrome driver #18676
Conversation
@@ -6,11 +6,11 @@ | |||
"private": true, | |||
"scripts": { | |||
"selenium-standalone": "selenium-standalone", | |||
"prepare": "selenium-standalone install" | |||
"prepare": "selenium-standalone install --config ..\\..\\..\\Shared\\E2ETesting\\selenium-config.json" |
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.
It's unusual to see Windows-style paths in a package.json
file. Does it work with /
too? If so that would be preferable as we may want this to run on other OSes too.
var psi = new ProcessStartInfo | ||
{ | ||
FileName = "npm", | ||
Arguments = $"run selenium-standalone start -- -- -port {port}", | ||
Arguments = $"run selenium-standalone start -- --config \"{seleniumConfigPath}\" -- -port {port}", |
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.
If we're hardcoding the seleniumConfigPath
into package.json
anyway, why go to all this trouble to pass it dynamically here? Would it be simpler to modify the selenium-standalone
script in package.json
so that it also hardcodes the config path? Then we wouldn't need to pass it from here, and therefore wouldn't need the AssemblyMetadataAttribute
, and wouldn't need the change to E2ETesting.props
, etc.
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.
I think the issue is that knowing the relative path inside of a test is a bit tricky. Using relative paths can mess up depending on how the test is launched and it's not always correct to assume PWD is the test directory. In other places where we need to find something relative, we do something quite similar - have the build tell us where things are.
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.
OK, sounds like a good reason!
This should avoid extra web traffic by only installing the chrome driver. Inspired by a build failure due to transient network issue: