-
Notifications
You must be signed in to change notification settings - Fork 47
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
Support getting current timeouts in sync w3c drivers. #281
base: master
Are you sure you want to change the base?
Changes from all commits
e884f5f
31148bd
b8f8bda
04f8831
f1ebd90
a0bea2b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
class TimeoutValues { | ||
final Duration script; | ||
final Duration implicit; | ||
final Duration pageLoad; | ||
|
||
TimeoutValues( | ||
{required this.script, required this.implicit, required this.pageLoad}); | ||
|
||
@override | ||
String toString() => | ||
'TimeoutValues(script: $script, implicit: $implicit, pageLoad: $pageLoad)'; | ||
|
||
@override | ||
int get hashCode => Object.hashAll([script, implicit, pageLoad]); | ||
|
||
@override | ||
bool operator ==(Object other) => | ||
other is TimeoutValues && | ||
script == other.script && | ||
implicit == other.implicit && | ||
pageLoad == other.pageLoad; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,9 +13,10 @@ | |
// limitations under the License. | ||
|
||
import '../common/request_client.dart'; | ||
import '../common/timeouts.dart'; | ||
import '../common/webdriver_handler.dart'; | ||
|
||
/// Sets WebDriver timeouts. | ||
/// Sets or gets WebDriver timeouts. | ||
class Timeouts { | ||
final SyncRequestClient _client; | ||
final WebDriverHandler _handler; | ||
|
@@ -40,6 +41,11 @@ class Timeouts { | |
_handler.timeouts.parseSetPageLoadTimeoutResponse); | ||
} | ||
|
||
/// Gets the current timeout values. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be a getter? 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? The assymmetry is irksome :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: I can do that if you think it's worth it. |
||
TimeoutValues getAllTimeouts() => _client.send( | ||
_handler.timeouts.buildGetTimeoutsRequest(), | ||
_handler.timeouts.parseGetTimeoutsResponse); | ||
|
||
@override | ||
String toString() => '$_handler.timeouts($_client)'; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
// Copyright 2015 Google Inc. All Rights Reserved. | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
@TestOn('vm') | ||
library webdriver.options_test; | ||
|
||
import 'package:test/test.dart'; | ||
import 'package:webdriver/src/common/timeouts.dart'; | ||
import 'package:webdriver/sync_core.dart'; | ||
|
||
import '../configs/sync_io_config.dart' as config; | ||
|
||
void runTests({WebDriverSpec spec = WebDriverSpec.Auto}) { | ||
group('TimeOuts', () { | ||
late WebDriver driver; | ||
|
||
setUp(() { | ||
driver = config.createTestDriver(spec: spec); | ||
}); | ||
|
||
test('set all timeouts', () { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test is testing "getting", so should probably be named as such. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
const five = Duration(seconds: 5); | ||
const one = Duration(seconds: 1); | ||
const ten = Duration(seconds: 10); | ||
|
||
driver.timeouts.setScriptTimeout(five); | ||
driver.timeouts.setImplicitTimeout(one); | ||
driver.timeouts.setPageLoadTimeout(ten); | ||
|
||
expect(driver.timeouts.getAllTimeouts(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if you don't set them timeouts before doing the get? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
TimeoutValues(script: five, implicit: one, pageLoad: ten)); | ||
}); | ||
|
||
test('get default timeouts', () { | ||
expect( | ||
driver.timeouts.getAllTimeouts(), | ||
TimeoutValues( | ||
script: const Duration(seconds: 30), | ||
implicit: Duration.zero, | ||
pageLoad: const Duration(minutes: 5))); | ||
}); | ||
}, timeout: const Timeout(Duration(minutes: 2))); | ||
} |
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 noparse
), 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'trequired
in the constructor.