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.

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

[js] Fix timeouts' url #2185

Merged
merged 3 commits into from
Jun 7, 2016
Merged

[js] Fix timeouts' url #2185

merged 3 commits into from
Jun 7, 2016

Conversation

JohanLorenzo
Copy link
Contributor

@JohanLorenzo JohanLorenzo commented May 31, 2016

Steps to reproduce

  1. Follow steps in this branch https://github.com/JohanLorenzo/geckodriver-bugs/tree/cant-set-timeout

Results

  1) timeouts "before all" hook:
     Uncaught UnknownCommandError: POST /session/68afbb72-991b-49ed-b09b-7d4767978232/timeouts/async_script did not match a known command

GeckoDriver (wires) is more strict about the URLs that you provide to it. They should comply with http://w3c.github.io/webdriver/webdriver-spec.html\#set-timeout . In this case we should not have /timeouts/async_script, but only /timeouts.

r? @jleyba

@jleyba
Copy link
Contributor

jleyba commented May 31, 2016

Does this break compatibility with the Selenium server and ChromeDriver?

@JohanLorenzo
Copy link
Contributor Author

JohanLorenzo commented May 31, 2016

Sorry, I haven't manually checked (I was expecting automated tests to check the APIs behavior). I'll get back to you with testing results.

Edit on June 6th: I'll bring testing results by tomorrow.

Edit on June 7th: I tested Chrome, and Chrome behind Selenium server. They both pass. See content of the tests here https://github.com/JohanLorenzo/geckodriver-bugs/tree/cant-set-timeout . I'll work on adding these tests in the PR.

@JohanLorenzo
Copy link
Contributor Author

JohanLorenzo commented Jun 7, 2016

@jleyba Tests done manually and new ones added. r?

Travis failed due to an error with buck. I don't think it's related to my PR.

@jleyba jleyba merged commit 4e77374 into SeleniumHQ:master Jun 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants