Closed
Bug 1485580
Opened 6 years ago
Closed 6 years ago
[wdspec] Parsing timeout values given as floats fail when equal to result of toFixed()
Categories
(Testing :: geckodriver, defect, P2)
Testing
geckodriver
Tracking
(firefox63 fixed)
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: whimboo, Assigned: whimboo)
References
()
Details
Attachments
(3 files, 2 obsolete files)
16.17 KB,
patch
|
jgraham
:
review+
|
Details | Diff | Splinter Review |
3.41 KB,
patch
|
jgraham
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•6 years ago
|
||
Assignee | ||
Comment 2•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=91d1a9f349023cb791d2b2ad600d4bb61fe21e3c
Assignee | ||
Updated•6 years ago
|
Attachment #9003379 -
Flags: review?(james)
Comment 3•6 years ago
|
||
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-
Assignee | ||
Comment 4•6 years ago
|
||
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
Assignee | ||
Comment 5•6 years ago
|
||
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.
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()
Assignee | ||
Comment 6•6 years ago
|
||
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.
Assignee | ||
Updated•6 years ago
|
Attachment #9003379 -
Attachment is obsolete: true
Assignee | ||
Comment 7•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3aca8a655cf0afb09134d1d3bc4495508e53f330
Assignee | ||
Comment 8•6 years ago
|
||
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
Assignee | ||
Comment 9•6 years ago
|
||
Attachment #9003747 -
Flags: review?(james)
Assignee | ||
Comment 10•6 years ago
|
||
Attachment #9003748 -
Flags: review?(james)
Assignee | ||
Comment 11•6 years ago
|
||
Attachment #9003749 -
Flags: review?(james)
Comment 12•6 years ago
|
||
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+
Updated•6 years ago
|
Attachment #9003748 -
Flags: review?(james) → review+
Updated•6 years ago
|
Attachment #9003747 -
Flags: review?(james) → review+
Assignee | ||
Comment 13•6 years ago
|
||
(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.
Assignee | ||
Comment 14•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Attachment #9003749 -
Attachment is obsolete: true
Assignee | ||
Comment 15•6 years ago
|
||
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+
Comment 16•6 years ago
|
||
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
Can't merge web-platform-tests PR due to failing upstream checks: Github PR https://github.com/web-platform-tests/wpt/pull/12675 * continuous-integration/travis-ci/pr (https://travis-ci.org/web-platform-tests/wpt/builds/420136488?utm_source=github_status&utm_medium=notification)
Comment 19•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1fd335726c00 https://hg.mozilla.org/mozilla-central/rev/4bc3167b0240 https://hg.mozilla.org/mozilla-central/rev/26632fd3312a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Upstream PR merged
You need to log in
before you can comment on or make changes to this bug.
Description
•