-
Notifications
You must be signed in to change notification settings - Fork 409
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
Select correct webdriver version #706
Conversation
Thanks for sending in this PR! Will take a look at this next week, after rust conf. |
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.
This is a great step forward -- thanks @MartinKavik! However, I have some concerns that I think we need to address before merging this. See inline comments below.
src/test/webdriver/chromedriver.rs
Outdated
handle.perform()?; | ||
|
||
let contents = handle.get_ref(); | ||
Ok(String::from_utf8_lossy(&contents.0).into_owned()) |
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.
This is already a fallible method, and if the string contains non-utf-8 bits, I'd rather fail loudly than silently introduce replacement chars:
Ok(String::from_utf8_lossy(&contents.0).into_owned()) | |
let vers = String::from_utf8(contents.0) | |
.context("chromedriver's LATEST_RELEASE is not valid UTF-8")?; | |
Ok(vers) |
src/test/webdriver/chromedriver.rs
Outdated
/// The official algorithm for `chromedriver` version selection: | ||
/// https://chromedriver.chromium.org/downloads/version-selection | ||
fn get_chromedriver_url(target: &str) -> Result<String, failure::Error> { | ||
let chromedriver_version = fetch_chromedriver_version()?; |
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 we should only check once per day, similar to how we throttle checking crates.io for new versions of wasm-pack itself being released.
Additionally, I think we should have a default, fallback version for when devs are offline. We can't make it impossible to run tests when one isn't connected to the internet. For this default version, we would manually keep it up to date with each wasm-pack
release, and the fetch_chromedriver_version
function would return it when the HTTP request otherwise fails. (Of course, the default would have to be downloaded at least once, but this would allow tests to work offline after it had been downloaded at least once).
src/test/webdriver/chromedriver.rs
Outdated
fn fetch_chromedriver_version() -> Result<String, failure::Error> { | ||
let mut handle = curl::easy::Easy2::new(Collector(Vec::new())); | ||
handle.url("https://chromedriver.storage.googleapis.com/LATEST_RELEASE")?; | ||
handle.perform()?; |
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.
These should have .context("...")
s added as well to help debugging / inform users as to what is going wrong when HTTP requests fail.
src/test/webdriver/geckodriver.rs
Outdated
/// https://firefox-source-docs.mozilla.org/testing/geckodriver/Support.html#supported-platforms | ||
fn get_geckodriver_url(target: &str, ext: &str) -> Result<String, failure::Error> { | ||
// JSON example: `{"id":15227534,"tag_name":"v0.24.0","update_url":"/mozzila...` | ||
let latest_tag_json = fetch_latest_geckodriver_tag_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.
Basically all the same comments from the chromedriver
module apply here, including the .context("...")
s.
Changes in the latest commit (I can squash them before merging if you want):
I've tested it with a local project and it seems working even without internet connection (if drivers are already downloaded). Your comments from the previous review should be resolved. |
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.
This looks wonderful! Thanks @MartinKavik for following through with addressing feedback and for your patience.
Closes #611
Changes
webdriver.rs
split intochromedriver.rs
,geckodriver.rs
andsafaridriver.rs
for better code organization.install_geckodriver
andinstall_chromedriver
fetches info about the latest*driver
version so they can find out if we are using the latest*driver
.Testing
curl
calls - should I test it in unit/integration tests somehow? (Is there an example for inspiration?)Possible future improvements
[chrome/gecko]driver
only when testing fails because of old driver and then restart testing.wasm-pack
cache / cratebinary-install
.get_chromedriver_url
andget_geckodriver_url
for more info.