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.

Sign up for GitHub

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

Jump to bottom

fix: More reliable (and correct) download #17

Merged
merged 1 commit into from
Feb 8, 2016
Merged

fix: More reliable (and correct) download #17

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/chromedriver/version.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 9 additions & 4 deletions lib/selenium/download.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions src/chromedriver/version.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()

{platform, bitness} = getArchitecture()
downloadUrl = "https://chromedriver.storage.googleapis.com/#{version}/chromedriver_#{platform}#{bitness}.zip"
Expand Down
11 changes: 7 additions & 4 deletions src/selenium/download.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,17 @@ module.exports = (binPath, tempPath, url, version, callback) ->

tempFileName = "selenium_#{version}.jar"
tempFilePath = "#{tempPath}/#{tempFileName}"
if fs.existsSync tempFilePath
copy tempFilePath, binFilePath, callback
validTempFilePath = "#{tempPath}/#{tempFileName}.valid"

if fs.existsSync validTempFilePath
copy validTempFilePath, binFilePath, callback
else
downloadFile url, tempPath, tempFileName, (error, hash) ->
return callback error if error?

validate tempFilePath, hash, (error) ->
return callback error if error?

copy tempFilePath, binFilePath, callback

copy tempFilePath, validTempFilePath, (error) ->
return callback error if error?
copy tempFilePath, binFilePath, callback
10 changes: 9 additions & 1 deletion test/test.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
fs = require 'fs'
{execFile} = require 'child_process'

assert = require 'assert'
rmrf = require 'rimraf'

Expand Down Expand Up @@ -30,7 +32,13 @@ seleniumDownload.update BIN_PATH, (error) ->
assert fs.existsSync(BIN_PATH + '/chromedriver')
assert fs.existsSync(BIN_PATH + '/selenium.jar')

clearFileSystem()
console.log 'make sure it did not download an invalid jar'
execFile 'java', [
# -h means "show usage/help". Selenium has no --version :(
'-jar', BIN_PATH + '/selenium.jar', '-h'
], (error, stdout) ->
throw error if error?
clearFileSystem()
Copy link
Collaborator

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 ?

Copy link
Contributor Author

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?

Copy link
Collaborator

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.


process.on 'uncaughtException', (error) ->
clearFileSystem()
Expand Down