Closed Bug 1485580 Opened Last year Closed Last year

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

Categories

(Testing :: geckodriver, defect, P2)

defect

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

()

Details

Attachments

(3 files, 2 obsolete files)

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.
Attachment #9003379 - Flags: review?(james)
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.
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.
Attachment #9003379 - Attachment is obsolete: true
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.
Attachment #9003749 - Attachment is obsolete: true
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.