Closed Bug 1491751 Opened 1 year ago Closed 1 year ago

"Pause" action primitive should allow duration to be optional

Categories

(Testing :: geckodriver, defect, P1)

defect

Tracking

(firefox-esr60 unaffected, firefox62 unaffected, firefox63 wontfix, firefox64 fixed)

RESOLVED FIXED
mozilla64
Tracking Status
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 --- wontfix
firefox64 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(Blocks 1 open bug, )

Details

(Keywords: regression)

Attachments

(2 files, 6 obsolete files)

As filed as https://github.com/mozilla/geckodriver/issues/1375 the pause action primitive currently doesn't support `undefined` as value for duration. Per the spec it should be allowed:

https://w3c.github.io/webdriver/#dfn-process-a-pause-action
Priority: -- → P3
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Attachment #9009883 - Attachment is obsolete: true
Summary: "Pause" action primitive should support undefined as duration → "Pause" action primitive should allow duration to be optional
Comment on attachment 9009887 [details] [diff] [review]
[geckodriver] "Pause" action primitive has to support undefined duration

Review of attachment 9009887 [details] [diff] [review]:
-----------------------------------------------------------------

r+wc

If you could update the commit message to not refer to JavaScript
concepts, that would be great.  The "duration" field is _optional_,
not "supported undefined".
Attachment #9009887 - Flags: review?(ato) → review+
Comment on attachment 9009890 [details] [diff] [review]
[wdspec] Add tests for null input source

Review of attachment 9009890 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/tests/webdriver/tests/perform_actions/validity.py
@@ +56,5 @@
> +    actions = [{
> +        "type": action_type,
> +        "id": "foobar",
> +        "actions": [{
> +            "type": "pause",

Can we test for a pause action with the duration set to null as
well, please?
Attachment #9009890 - Flags: review?(ato) → review-
Comment on attachment 9009889 [details] [diff] [review]
[wdspec] Create tests for "Release Actions" command

Review of attachment 9009889 [details] [diff] [review]:
-----------------------------------------------------------------

Is this change relevant to fixing the optional pause duration?

Just re-flag me if you think it is.
Attachment #9009889 - Flags: review?(ato)
Comment on attachment 9009888 [details] [diff] [review]
[wdspec] Relocate "Perform Actions" tests, and separate tests by input source

Review of attachment 9009888 [details] [diff] [review]:
-----------------------------------------------------------------

I have the same question about this patch.
Attachment #9009888 - Flags: review?(ato)
Comment on attachment 9009890 [details] [diff] [review]
[wdspec] Add tests for null input source

Review of attachment 9009890 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/tests/webdriver/tests/perform_actions/validity.py
@@ +56,5 @@
> +    actions = [{
> +        "type": action_type,
> +        "id": "foobar",
> +        "actions": [{
> +            "type": "pause",

This is already done in the test `test_pause_invalid_types` you wrote not that long ago.
Attachment #9009890 - Flags: review- → review?(ato)
(In reply to Andreas Tolfsen ❲:ato❳ from comment #10)
> Is this change relevant to fixing the optional pause duration?

It's not, and I can move this out to a separate patch if that is what you want to hear.
Comment on attachment 9009890 [details] [diff] [review]
[wdspec] Add tests for null input source

Review of attachment 9009890 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/tests/webdriver/tests/perform_actions/validity.py
@@ +56,5 @@
> +    actions = [{
> +        "type": action_type,
> +        "id": "foobar",
> +        "actions": [{
> +            "type": "pause",

Perfect.

What I wanted to avoid here is someone implementing the spec thinking
{type: "pause"} and {type: "pause", duration: null} are equivalent.
Attachment #9009890 - Flags: review?(ato) → review+
(In reply to Henrik Skupin (:whimboo) from comment #13)

> (In reply to Andreas Tolfsen ❲:ato❳ from comment #10)
> 
> > Is this change relevant to fixing the optional pause duration?
> 
> It's not, and I can move this out to a separate patch if that is
> what you want to hear.

Sorry, I didn’t mean any offence.

I was just trying to ascertain if the patches were somehow related
to the optional pause duration change, or if they were preparation
work for the fourth patch to add tests for the missing duration
parameter.  The patches themselves look good, but if they are indeed
unrelated then splitting that work off to a separate bug makes
sense.
No problem at all. I was somehow in such a refactoring mode that I wanted to make it look nice together with this patch. I will move the commits to bug 1492469.
Attachment #9009888 - Attachment is obsolete: true
Attachment #9009889 - Attachment is obsolete: true
Attachment #9009890 - Attachment is obsolete: true
Comment on attachment 9010280 [details] [diff] [review]
[wdspec] Add tests for null input source and missing duration for pause primitive

Rebased the patch after removing the refactoring patches. Taking over r+ from ato.
Attachment #9010280 - Flags: review+
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/57bb41e8a02a
[geckodriver] "Pause" action primitive should allow duration to be optional. r=ato
https://hg.mozilla.org/integration/mozilla-inbound/rev/35e63e22c367
[wdspec] Add tests for null input source and missing duration for pause primitive. r=ato
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13078 for changes under testing/web-platform/tests
Attachment #9010367 - Attachment is obsolete: true
Attachment #9010367 - Flags: review?(ato)
Attachment #9010366 - Attachment is obsolete: true
Attachment #9010366 - Flags: review?(ato)
https://hg.mozilla.org/mozilla-central/rev/57bb41e8a02a
https://hg.mozilla.org/mozilla-central/rev/35e63e22c367
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Upstream PR merged
No need to uplift given that this didn't affect our tests in CI, and a new geckodriver release is already out.
You need to log in before you can comment on or make changes to this bug.