Support getting current timeouts in sync w3c drivers. #281
Conversation
Uses the W3C "Get Timeouts" API: https://www.w3.org/TR/webdriver1/#get-timeouts There is no such API in the JSON API: https://www.selenium.dev/documentation/legacy/json_wire_protocol/#command-summary Rationale: Allows sync clients to temporarily change a timeout and then restore it back to what it was before (which doesn't work for async clients anyway). Tested: Against geckodriver, and internally against Chrome.
|
@devoncarew do you know the best person to review this? |
| @@ -0,0 +1,22 @@ | |||
| class TimeoutValues { | |||
There was a problem hiding this comment.
@jakemac53 @lrhn - Would you have any concerns about using a record type with named fields in place of this class? The package is already on language version 3.
There was a problem hiding this comment.
No concerns. It means there won't be a nice toString, possibly it will lack the names in production.
But the toString is for debugging only (there is no parse), so that shouldn't be a problem.
I wouldn't introduce a typedef then, just inline the type in the function declaration:
({Duration script, Duration implicit, Duration plageLoad}),
as a multi-value result.
There was a problem hiding this comment.
I wonder if we need to worry about this interface changing. Without the class it could be harder to add more fields, because the record types are unrelated once we add a new named field. With the class (if we make it final class) we can add a new field as long at is isn't required in the constructor.
I had the understanding we wanted to move away from the sync client. The Why are we adding new sync-only features?
Why doesn't it work for async clients? |
@parren-google - can you help me understand this constraint? |
| WebDriverRequest buildGetTimeoutsRequest() { | ||
| // Not supported by JSON protocol: | ||
| // https://www.selenium.dev/documentation/legacy/json_wire_protocol/#command-summary | ||
| throw UnimplementedError(); |
There was a problem hiding this comment.
Do we plan to support it?
Otherwise use UnsupportedError.
There was a problem hiding this comment.
I am not aware of plans to support getting timeouts in the JsonWire webdriver API. Switched to UnsupportedError.
| @@ -0,0 +1,22 @@ | |||
| class TimeoutValues { | |||
There was a problem hiding this comment.
No concerns. It means there won't be a nice toString, possibly it will lack the names in production.
But the toString is for debugging only (there is no parse), so that shouldn't be a problem.
I wouldn't introduce a typedef then, just inline the type in the function declaration:
({Duration script, Duration implicit, Duration plageLoad}),
as a multi-value result.
| @@ -40,6 +41,11 @@ class Timeouts { | |||
| _handler.timeouts.parseSetPageLoadTimeoutResponse); | |||
| } | |||
|
|
|||
| /// Gets the current timeout values. | |||
There was a problem hiding this comment.
Should be a getter?
(Unlss we specifically want it to match the underlying W3C API.)
And the operations above should probably have been setters, but they didn't have getters, which was an argument against that. Should they have getters now?
Does it make sense to get the individual timeouts?)
Would it make sense to set all the timeouts in one batch?
The assymmetry is irksome :)
There was a problem hiding this comment.
I didn't want to change the existing API. We could introduce a new setter for all the timeouts though. The W3C API would support setting them all in one go:
https://www.w3.org/TR/webdriver2/#set-timeouts
https://github.com/google/webdriver.dart/blob/master/lib/src/handler/w3c/timeouts.dart#L21
I can do that if you think it's worth it.
| driver.timeouts.setImplicitTimeout(one); | ||
| driver.timeouts.setPageLoadTimeout(ten); | ||
|
|
||
| expect(driver.timeouts.getAllTimeouts(), |
There was a problem hiding this comment.
What happens if you don't set them timeouts before doing the get?
Does it work? (Do they have default values? Are those default values known?)
There was a problem hiding this comment.
I added a test that gets the default values. However, I don't know if these come from our Dart webdriver code or from geckodriver. If the latter, we'd be exposing the test to changes in these defaults in geckodriver.
| driver = config.createTestDriver(spec: spec); | ||
| }); | ||
|
|
||
| test('set all timeouts', () { |
There was a problem hiding this comment.
The test is testing "getting", so should probably be named as such.
I'd start the test name with a capital letter.
There was a problem hiding this comment.
I forked this from https://github.com/google/webdriver.dart/blob/master/test/sync/timeouts.dart#L32, and so the main purpose is still to set the timeouts.
Sorry, that was a misunderstanding from my side. I thought the async driver used the JSON API. But it can also use W3C, so we can support getting timeouts in the aysnc driver too. |
However, the current |
Uses the W3C "Get Timeouts" API:
https://www.w3.org/TR/webdriver1/#get-timeouts
There is no such API in the JSON API:
https://www.selenium.dev/documentation/legacy/json_wire_protocol/#command-summary
Rationale: Allows sync clients to temporarily change a timeout and then restore it back to what it was before (which doesn't work for async clients anyway).
Tested: Against geckodriver, and internally against Chrome.
Implements #280.