wpt icon indicating copy to clipboard operation
wpt copied to clipboard

[WebDriver BiDi] fix `test_release_mouse_sequence_resets_dblclick_state`

Open sadym-chromium opened this issue 1 year ago • 7 comments

  • Move test to tentative, as the expected behavior is not clear yet.
  • Remove parameter, as the double click is tested in test_dblclick_at_coordinates.

sadym-chromium avatar May 16 '24 13:05 sadym-chromium

@sadym-chromium could you maybe explain what is/was broken with the test?

whimboo avatar May 16 '24 13:05 whimboo

@sadym-chromium could you maybe explain what is/was broken with the test?

sure!

  1. The test test_release_mouse_sequence_resets_dblclick_state checks both with and without release, which is confusing.
  2. Moving it to tentative until the https://github.com/w3c/webdriver/issues/1772 is clarified.

sadym-chromium avatar May 16 '24 14:05 sadym-chromium

@sadym-chromium could you maybe explain what is/was broken with the test?

  1. If there is no release actions, why the dblclick should be absent at all?

sadym-chromium avatar May 16 '24 14:05 sadym-chromium

2. Moving it to tentative until the [Clarify how the double click tracking should work in the actions w3c/webdriver#1772](https://github.com/w3c/webdriver/issues/1772) is clarified.

Would you mind renaming the test so that it includes tentative in its name?

whimboo avatar May 17 '24 08:05 whimboo

Would you mind renaming the test so that it includes tentative in its name?

@whimboo done

sadym-chromium avatar May 17 '24 08:05 sadym-chromium

You moved the test below a tentative folder which is not what we usually do for tentative wpt tests. As mentioned in my review please add the word to the test name: webdriver/tests/bidi/input/release_actions/sequence_tentative.py

Otherwise I assume (didn't check the changes directly) it is just moving the existing tests without any further changes?

whimboo avatar May 21 '24 09:05 whimboo

You moved the test below a tentative folder which is not what we usually do for tentative wpt tests. As mentioned in my review please add the word to the test name: webdriver/tests/bidi/input/release_actions/sequence_tentative.py

Got it. Done.

Otherwise I assume (didn't check the changes directly) it is just moving the existing tests without any further changes?

Yes, except release_actions parameter:

@pytest.mark.parametrize(
    "release_actions",
    [True, False],
    ids=["with release actions", "without release actions"],
)
...
    if release_actions:
        await bidi_session.input.release_actions(context=top_context["context"])

sadym-chromium avatar May 21 '24 09:05 sadym-chromium

@whimboo any other concerns?

sadym-chromium avatar May 22 '24 12:05 sadym-chromium