Closed Bug 1485580 Opened 7 years ago Closed 7 years ago

[wdspec] Parsing timeout values given as floats fail when equal to result of toFixed()

Categories

(Testing :: geckodriver, defect, P2)

Product:
Testing
Component:
geckodriver
Type:
defect

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(
URL
)

Details

Attachments

(3 files, 2 obsolete files)

[wdspec] Improve timeouts tests for "Set Timeout" and "New Session"
16.17 KB, patch
jgraham
: review+
Details | Diff | Splinter Review
[marionette] Update unit tests for invalid timeout values
3.41 KB, patch
jgraham
: review+
Details | Diff | Splinter Review
[geckodriver] Make parsing of float timeout values spec conforming
8.00 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
The following tests make use of the `valid_data` from the `create.py` module: https://searchfox.org/mozilla-central/rev/f2ac80ab7dbde5400a3400d463e07331194dec94/testing/web-platform/tests/webdriver/tests/new_session/support/create.py#9 And that contains a line like: > {"script": 0, "pageLoad": 2.0, "implicit": 2**53 - 1} As given by the spec only integers are allowed. It means that the tests are invalid, and need to be corrected.
Comment on attachment 9003379 [details] [diff] [review] [wdspec] Correct create_alwaysMatch.py and create_firstMatch.py for float not being a valid timeout value Review of attachment 9003379 [details] [diff] [review]: ----------------------------------------------------------------- 2.0 is an integer. Javascript (and hence JSON) doesn't have a distinction between floats and ints, so we have to accept any input that can be exactly represented as an integer.
Attachment #9003379 - Flags: review?(james) → review-
Hm, interesting. Looks like I missed the following part of the spec: > An integer is a Number that is unchanged under the ToInteger operation. Actually `ToInteger()` is obsolete and never made it from the draft into the final ES6 specification as what MDN says: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/toInteger
So I filed a WebDriver spec issue: https://github.com/w3c/webdriver/issues/1295 I think that we want to have this check in Marionette, but also geckodriver. Given that Serde is in use now, we would need a custom deserialization routine. As such I will keep this bug in the geckodriver component.
URL: https://github.com/w3c/webdriver/issu...
Summary: [wdspec] Correct create_alwaysMatch.py and create_firstMatch.py for float not being a valid timeout value → [wdspec] Parsing timeout values given as floats fail when equal to result of toFixed()
Actually Marionette handles that all very well, so I only had to update the unit tests for a better coverage. For geckodriver I got the changes implemented for `Set Timeout`, and soon also for the capabilities in `New Session`. To test all the various cases I completed all the tests for `Set Timeout`. I should have an updated patch soon.
I accidentally pushed an artifact build to try, whereby it needs to be a non-artifact build due to the geckodriver changes. https://treeherder.mozilla.org/#/jobs?repo=try&revision=7613eab46178d698bf3c7d8f2d6c0476c535a96d
Comment on attachment 9003749 [details] [diff] [review] [geckodriver] Make parsing of float timeout values spec conforming Review of attachment 9003749 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/webdriver/src/capabilities.rs @@ +331,2 @@ > ); > + if timeout < 0.0 || timeout.fract() != 0.0 { I wonder if we should have a tolerance for nonzero fractional parts? Probably not necessary for this bug but could be needed in a followup. @@ +341,5 @@ > + if (timeout as u64) > MAX_SAFE_INTEGER { > + return Err(WebDriverError::new( > + ErrorStatus::InvalidArgument, > + format!( > + "'{}' timeouts valus is outside of maximum safe integer: {}", typo "valus" ::: testing/webdriver/src/command.rs @@ +525,5 @@ > + ))); > + } > + if (n as u64) > MAX_SAFE_INTEGER { > + return Err(de::Error::custom(format!( > + "'{}' is outside of maximum safe integer", s/Outside of/Greater than/
Attachment #9003749 - Flags: review?(james) → review+
Attachment #9003748 - Flags: review?(james) → review+
Attachment #9003747 - Flags: review?(james) → review+
(In reply to James Graham [:jgraham] from comment #12) > > + if timeout < 0.0 || timeout.fract() != 0.0 { > > I wonder if we should have a tolerance for nonzero fractional parts? > Probably not necessary for this bug but could be needed in a followup. Handling the epsilon is also something which I mentioned in the created webdriver spec issue yesterday. I decided against using it for now as long as the issue is open. A follow-up sounds fine to me, once the issue has been resolved, and handling the epsilon is required. The other two issues will be fixed with the next update. Thanks.
Comment on attachment 9003791 [details] [diff] [review] [geckodriver] Make parsing of float timeout values spec conforming Updated patch with the two issues fixed. Taking over the r+.
Attachment #9003791 - Flags: review+
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1fd335726c00 [wdspec] Improve timeouts tests for "Set Timeout" and "New Session". r=jgraham https://hg.mozilla.org/integration/mozilla-inbound/rev/4bc3167b0240 [marionette] Update unit tests for invalid timeout values. r=jgraham https://hg.mozilla.org/integration/mozilla-inbound/rev/26632fd3312a [geckodriver] Make parsing of float timeout values spec conforming. r=jgraham
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12675 for changes under testing/web-platform/tests
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: