search

mozilla / geckodriver

WebDriver for Firefox
https://firefox-source-docs.mozilla.org/testing/geckodriver/
Mozilla Public License 2.0
7.19k stars 1.52k forks source link

Script timeout should accept a null value #1514

Open DrMarcII opened 5 years ago

DrMarcII commented 5 years ago

System

See https://w3c.github.io/webdriver/#timeouts

Although on my cursory examination of the spec, it is unclear what null means exactly (I suspect, but am not sure, it means unlimited time).

To recreate send: { 'script': null } to the POST /sesssion/:id/timeouts endpoint.

The response value is: {'error': 'invalid argument', 'message': 'null is not a positive integer at line 1 column 48'}

andreastt commented 5 years ago

This should work in 0.24.0:

Allow use of an indefinite script timeout for the Set Timeouts command, thanks to reimu.

It would be great if you could confirm that it works for you.

DrMarcII commented 5 years ago

With 0.24.0 I get the following: { 'error': 'invalid argument', 'message': 'Expected [object String] "script" to be a positive integer, got [object Null] null' }

And I was wrong, the spec does make it clear what null means: "A null value implies that scripts should never be interrupted, but instead run indefinitely."

andreastt commented 5 years ago

I can’t seem to reproduce that:

% curl -d '{"script": null}' http://localhost:4444/session/e19cab7f-ea18-4dcd-ac15-cb53acc9ee52/timeouts
1551547897030   webdriver::server   DEBUG   -> POST /session/e19cab7f-ea18-4dcd-ac15-cb53acc9ee52/timeouts {"script": null}
1551547897033   Marionette  DEBUG   0 -> [0,2,"WebDriver:SetTimeouts",{"script":null}]
1551547897035   Marionette  DEBUG   0 <- [1,2,null,{"value":null}]
1551547897035   webdriver::server   DEBUG   <- 200 OK {"value":null}
DrMarcII commented 5 years ago

Looks like the error I was seeing in 0.24.0 was from Marionette in Firefox 65.0. When run with Firefox 66.0b3 it appears to accept null for script.

andreastt commented 5 years ago

Oh sorry, that should’ve been called out in the change log… and probably an update is needed to https://firefox-source-docs.mozilla.org/testing/geckodriver/Support.html.

whimboo commented 5 years ago

I wouldn't bump the minimum version of Firefox due to that change, which is just an addition similar to the new window command. But mentioning it in the release notes is clearly a miss.