-
Notifications
You must be signed in to change notification settings - Fork 25
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: More reliable (and correct) download #17
Conversation
@@ -63,6 +63,8 @@ module.exports = (callback) -> | |||
version = FALLBACK_CHROMEDRIVER_VERSION | |||
console.log "[testium] Unable to determine latest version of selenium chromedriver; using #{version}" | |||
console.error (error.stack || error) | |||
else | |||
version = "#{version}".trim() |
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.
What is version here? Can we do version.trim()
or version.toString().trim()
instead? Those are more clear to me.
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 version.trim()
should be fine. This is just me being uber-careful about calling string methods "just in case" (e.g. null.toString().trim()
would throw). Don't think it's warranted here.
* The chrome version info may contain a trailing new line. This broke our url generation. * The broken chrome url lead to the process crashing which lead to a broken tmp file. And once that tmp file existed, it was blindly copied. We're now waiting for the download to complete and then copy the file to a new name once it was actually validated. There's still a small window where we may end up in a broken state but it's progress.
24e3424
to
7599b95
Compare
Actually, I reverted most of the selenium code changes. Pulling out the method didn't provide value in the last iteration. |
'-jar', BIN_PATH + '/selenium.jar', '-h' | ||
], (error, stdout) -> | ||
throw error if error? | ||
clearFileSystem() |
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.
Is this after error so that you can inspect the file system state in the error case? If so, what about the other error case at https://github.com/groupon/selenium-download/pull/17/files#diff-3679dd81e69928644563abcacc1e00ddR44 ?
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.
Not sure I understand - in the success case it will clear the file system and in the failure case as well. I treated this as "insert an additional assertion about the files" so I inserted the new code between the existing assertions and the cleanup. Do you mean that it should delete the file before throwing the error?
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.
Oh, so it will. Yeah, this is fine.
fix: More reliable (and correct) download
@EndangeredMassa Looks like I don't have access to this on npm. Could you release this change (and/or add me as an owner :))? |
@EndangeredMassa Ping? :) |
Added. Sorry for the delay! |
This broke our url generation.
a broken tmp file. And once that tmp file existed, it was blindly
copied. We're now waiting for the download to complete and then
copy the file to a new name once it was actually validated.
There's still a small window where we may end up in a broken
state but it's progress.