1328726 - Add web-platform wdspec tests for key actions

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

* Test (sequences of) single-character key actions
* Test release actions command

Comment 1

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

These will be addressed in follow-up bugs:
* test non-printing chars, non-ascii chars, grapheme clusters: geckodriver 0.12 actions endpoint only supports single-character values for key actions, so we can't yet test non-printing characters by sending unicode code points, for example.
* test pauses (and test action chains properly): need to implement a wait utility to check the outcomes of asynchronous behaviour

Comment 2

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Attachment: Bug 1328726 - Style fix ups in Marionette key actions implementation;

These are leftover from review of Bug 1320389. I must have forgotten to
re-push to MozReview.

Review commit: https://reviewboard.mozilla.org/r/102340/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102340/

Comment 3

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Attachment: Bug 1328726 - (wdclient) Add actions endpoints, key actions API;

Review commit: https://reviewboard.mozilla.org/r/102342/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102342/

Comment 4

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Attachment: Bug 1328726 - (wdclient) Add element property command, use value key;

Review commit: https://reviewboard.mozilla.org/r/102344/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102344/

Comment 5

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Attachment: Bug 1328726 - Add server fixture to pytestrunner;

Review commit: https://reviewboard.mozilla.org/r/102346/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102346/

Comment 6

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Attachment: Bug 1328726 - Add web-platform wdspec tests for key actions;

Note the terrible use of sleep, which should be replaced by polling, to
deal with asynchronous events.

Review commit: https://reviewboard.mozilla.org/r/102348/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102348/

Comment 7

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8823857 [details]
Bug 1328726 - Style fix ups in Marionette key actions implementation;

https://reviewboard.mozilla.org/r/102340/#review102736

Comment 8

[James Graham [:jgraham]]

Comment on attachment 8823858 [details]
Bug 1328726 - (wdclient) Add actions endpoints, key actions API;

https://reviewboard.mozilla.org/r/102342/#review102914

::: testing/web-platform/tests/tools/webdriver/webdriver/client.py:78
(Diff revision 1)
> +    def __init__(self, session):
> +        self.session = session
> +
> +    @command
> +    def perform_actions(self, actions):
> +        body = {"actions": actions}

This feels pretty low level. Maybe that's all you need, but I wonder if it's worth providing a more felshed-out API closer in style to the rest of the file?

Comment 9

[James Graham [:jgraham]]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

https://reviewboard.mozilla.org/r/102348/#review102910

This seems like a good start, but there is so much more that should be tested e.g.

 * Parsing of actions messages (I know this is strictly unrelated to this bug, but we are missing it from the testsuite).
 * keyUp of keys that did not previously have keydown
 * Modifier keys like shift, ctrl, etc. pressing and releasing at different points in a sequence
 * Correct support for non-printable keys (all the ones with rnadom PUA assigned unicode characters)
 * Sending keys like é in the composed (one codepoint) and decomposed (multi-codepoint) forms
 * Other codepoints that don't appear in the ascii set, particularly e.g. emoji

I don't know if that strictly needs to block this landing, but I do know we need more comprehensive testing in this area.

::: testing/web-platform/meta/webdriver/actions.py.ini:1
(Diff revision 1)
> +[actions.py]

This file shouldn't be necessary (the wpt manifest is autogenerated, the .ini files are only for tests that fail).

::: testing/web-platform/tests/test_keys_wdspec.html:26
(Diff revision 1)
> +  </div>
> +
> +  <div>
> +    <p>KeyReporter</p>
> +    <input type="text" id="keys" size="80"
> +           onkeyup="appendMessage('up: ' + event.code)"

This doesn't seem to have any information about which key was pressed, or modifiers + etc. That seems like it will be rather important for writing other tests.

::: testing/web-platform/tests/webdriver/actions.py:50
(Diff revision 1)
> +    assert key_reporter.property("value").get("value") == ""
> +    assert event_reporter.text.get("value").strip() == ""
> +
> +
> +def test_printable_key_action_sends_keypress(
> +        session, key_reporter, event_reporter, make_key_chain):

I'm not a huge fan of this indentation style. I would prefer everything on one line, or one parameter per like like

    def foo(a,
            b):

::: testing/web-platform/tests/webdriver/actions.py:53
(Diff revision 1)
> +
> +def test_printable_key_action_sends_keypress(
> +        session, key_reporter, event_reporter, make_key_chain):
> +    actions = make_key_chain([
> +        {
> +            "type": "keyDown",

I would be very tempted to make these into tuples for brevity.

::: testing/web-platform/tests/webdriver/actions.py:58
(Diff revision 1)
> +            "type": "keyDown",
> +            "value": u"c",
> +        },
> +    ])
> +    session.actions.perform_actions(actions)
> +    assert "press" in event_reporter.text.get("value").strip()

Is there some specific reason not to validate against the exact string that we expect rather than looking for substrings only?

::: testing/web-platform/tests/webdriver/actions.py:83
(Diff revision 1)
> +            "value": u"a",
> +        },
> +    ])
> +    session.actions.perform_actions(actions)
> +    # reset event_reporter
> +    session.execute_script("window.displayMessage(' ');")

`window.` here shouldn't be needed?

::: testing/web-platform/tests/webdriver/actions.py:99
(Diff revision 1)
> [e-mail]("k,code", [
> +    (u"a", "KeyA"),
> +    (u"\"", "Quote"),
> +    (u",", "Comma"),
> +])
> +def test_single_printable_key_sends_correct_events(

I would argue that the previous tests don't have much value compared to this one.

::: testing/web-platform/tests/webdriver/actions.py:130
(Diff revision 1)
> +            "type": "keyDown",
> +            "value": u"b",
> +        },
> +    ])
> +    session.actions.perform_actions(actions)
> +    sleep(1)

Relying on a fixed sleep like this seems like a good way to have intermittent tests. In the cases where we expect a value we should poll the server for a non-empty field and have a timeout after which we fail the test.

Comment 10

[James Graham [:jgraham]]

Comment on attachment 8823860 [details]
Bug 1328726 - Add server fixture to pytestrunner;

https://reviewboard.mozilla.org/r/102346/#review102950

::: testing/web-platform/harness/wptrunner/executors/pytestrunner/fixtures.py:134
(Diff revision 1)
>              conn.request(method, path)
>              yield conn.getresponse()
>          finally:
>              conn.close()
>  
> +class Server(object):

This feels rather over-confusing. All you seem to really want is function that closes over the provided `url_getter` function. I don't know if it works with pytest to write that directly, or if you can write it as a class with a `__call__` method for example.

I also think that such a function should be documented to describe the parameter (it wasn't immediately clear that the input was a function to return the server base url).

Comment 11

[James Graham [:jgraham]]

Comment on attachment 8823860 [details]
Bug 1328726 - Add server fixture to pytestrunner;

https://reviewboard.mozilla.org/r/102346/#review102952

Comment 12

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8823858 [details]
Bug 1328726 - (wdclient) Add actions endpoints, key actions API;

https://reviewboard.mozilla.org/r/102342/#review103418

Comment 13

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8823858 [details]
Bug 1328726 - (wdclient) Add actions endpoints, key actions API;

https://reviewboard.mozilla.org/r/102342/#review102914

> This feels pretty low level. Maybe that's all you need, but I wonder if it's worth providing a more felshed-out API closer in style to the rest of the file?

I agree.  I think it should have a more comprehensive API for the common paths to make tests shorter to write.  Remember that this client will be used for other WPT testing that may not want to define the entire action chain as a JSON dictionary.

For the action tests specifically, you could call `session.send_command(METHOD, PATH, BODY [, key=EXPECTED_KEY])` whenever you need to construct the dict literals to test the finer grained aspects of actions.

The Marionette actions API is probably not the best example to follow, but gives you an idea of what a user might expect to see from action chains.  It was, I believe, inspired by the APIs in the various Selenium client bindings: http://marionette-client.readthedocs.io/en/master/reference.html#actions

Some sort of builder pattern to construct the action chain would make a lot of sense:

```python
Actions(session)
    .move(element)
    .move_by_offset(10, 10)
    .mouse_down()
    .move(other_element)
    .mouse_up()
    .perform()
```

Comment 14

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8823859 [details]
Bug 1328726 - (wdclient) Add element property command, use value key;

https://reviewboard.mozilla.org/r/102344/#review103420

::: testing/web-platform/tests/tools/webdriver/webdriver/client.py:460
(Diff revision 1)
>      @command
> +    def property(self, name):
> +        return self.session.send_command("GET", self.url("property/%s" % name))
> +
> +    @command
>      def attribute(self, name):
>          return self.session.send_command("GET", self.url("attribute/%s" % name))

I think maybe these may be missing `key="value"`?  Feel free to resolve this if that’s not the case.

Comment 15

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8823859 [details]
Bug 1328726 - (wdclient) Add element property command, use value key;

https://reviewboard.mozilla.org/r/102344/#review103424

Comment 16

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8823860 [details]
Bug 1328726 - Add server fixture to pytestrunner;

https://reviewboard.mozilla.org/r/102346/#review103426

::: testing/web-platform/harness/wptrunner/executors/pytestrunner/runner.py:36
(Diff revision 1)
>      """Run Python test at ``path`` in pytest.  The provided ``session``
>      is exposed as a fixture available in the scope of the test functions.
>  
>      :param path: Path to the test file.
>      :param session: WebDriver session to expose.
> +    :param url_getter: function to get server url from test environment, given a protocol.

Capital F and fmt(1) it.

Comment 17

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

https://reviewboard.mozilla.org/r/102348/#review102910

> Relying on a fixed sleep like this seems like a good way to have intermittent tests. In the cases where we expect a value we should poll the server for a non-empty field and have a timeout after which we fail the test.

I suggest importing the `Wait` utility from Marionette.  But it surprises me a little bit that the keys- and events aren’t dispatched in time this returns.  Also, isn’t the actions API meant to be blocking?

Comment 18

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

https://reviewboard.mozilla.org/r/102348/#review103428

::: testing/web-platform/tests/test_keys_wdspec.html:6
(Diff revision 1)
> +<!doctype html>
> +<meta charset=utf-8>
> +
> +<head>
> +    <title>Test Keys</title>
> +    <script type="text/javascript">

`type` is not needed.

::: testing/web-platform/tests/test_keys_wdspec.html:8
(Diff revision 1)
> +            document.getElementById('events').innerHTML = "<p>" + message + "</p>";
> +        }
> +
> +        function appendMessage(message) {
> +            document.getElementById('events').innerHTML += message + " ";

Can we please not mix single- and double quotes?

::: testing/web-platform/tests/webdriver/actions.py:3
(Diff revision 1)
> +# TODO implement wait utility
> +from time import sleep

I would suggest we donate the Wait utility I wrote for Marionette to WPT in due course.

::: testing/web-platform/tests/webdriver/actions.py:14
(Diff revision 1)
> [e-mail]
> +def event_reporter(session, test_keys_page, request):
> +    paragraph = session.find.css("#events", all=False)
> +    return paragraph

I wonder if it would help the tests to turn this into a a more complex Python representation to allow tests to query the key reporter?

I haven’t throught at great length about this but it seems tideous to have to get the text, get it’s value, strip it for whitespace, &c.

::: testing/web-platform/tests/webdriver/actions.py:36
(Diff revision 1)
> +        body[0]["actions"] = a if a is not None else []
> +        return body

If you decide to keep `make_key_chain` after you introduce a more comprehensive API to wdclient, I would suggest first constructing the actions JSON object and then wrapping it in an array before returning.  The `body[0] =` annotation is a bit clunky.

::: testing/web-platform/tests/webdriver/actions.py:39
(Diff revision 1)
> +
> +    def maker(a=None):
> +        body[0]["actions"] = a if a is not None else []
> +        return body
> +    # clean up between tests
> +    request.addfinalizer(session.actions.release_actions)

You can put this in a function `teardown_method`.  This isn’t a natural fit for `make_key_chain`.

::: testing/web-platform/tests/webdriver/actions.py:45
(Diff revision 1)
> +    assert key_reporter.property("value").get("value") == ""
> +    assert event_reporter.text.get("value").strip() == ""

I would like these assertions to be nicer.  This relates to the issue I raised earlier about providing some sort of Python abstraction on top of the key reporter.

A random idea:

```python
def test_empty_actions_chain(session, keys, events):
    Actions(session).perform()
    assert len(keys) == 0
    assert len(events) == 0
```

We could empty the key- and event reporter after each test, possibly also reload the document?

Making the reporters indexed would also have other advantages: As you construct the action chains you have a clear picture of what DOM events and keys should be dispatched.  An indexed event reporter would let you iterate of the events or even compare them to a predefined dictionary.

```python
def test_press_key(session, keys, events):
    Actions(session).key_down("a").key_up("a").perform()
    assert keys[0] == "a"
    assert events[0].type == "keydown"
    assert events[0].code == keys[0]
    assert events[0].ctrlKey == False
    assert events[0].shiftKey == False
    …
    assert events[1].type == "keypress"
    …
    assert events[2].type == "keyup"
```

::: testing/web-platform/tests/webdriver/actions.py:82
(Diff revision 1)
> +    # reset event_reporter
> +    session.execute_script("window.displayMessage(' ');")

I think resetting the event- and key reporter for each test would be a good idea.  Could do this in a `teardown_method` function.

Comment 19

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823858 [details]
Bug 1328726 - (wdclient) Add actions endpoints, key actions API;

https://reviewboard.mozilla.org/r/102342/#review102914

> I agree.  I think it should have a more comprehensive API for the common paths to make tests shorter to write.  Remember that this client will be used for other WPT testing that may not want to define the entire action chain as a JSON dictionary.
> 
> For the action tests specifically, you could call `session.send_command(METHOD, PATH, BODY [, key=EXPECTED_KEY])` whenever you need to construct the dict literals to test the finer grained aspects of actions.
> 
> The Marionette actions API is probably not the best example to follow, but gives you an idea of what a user might expect to see from action chains.  It was, I believe, inspired by the APIs in the various Selenium client bindings: http://marionette-client.readthedocs.io/en/master/reference.html#actions
> 
> Some sort of builder pattern to construct the action chain would make a lot of sense:
> 
> ```python
> Actions(session)
>     .move(element)
>     .move_by_offset(10, 10)
>     .mouse_down()
>     .move(other_element)
>     .mouse_up()
>     .perform()
> ```

I agree with your feedback, and I'll certainly make some changes here as I address your comments on the tests, but more than that feels like scope bloat.

I think it's reasonable to flesh out the API in a separate bug. Top priority is to implement an initial, comprehensive-enough set of tests.

Comment 20

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8823858 [details]
Bug 1328726 - (wdclient) Add actions endpoints, key actions API;

https://reviewboard.mozilla.org/r/102342/#review102914

> I agree with your feedback, and I'll certainly make some changes here as I address your comments on the tests, but more than that feels like scope bloat.
> 
> I think it's reasonable to flesh out the API in a separate bug. Top priority is to implement an initial, comprehensive-enough set of tests.

That’s fair.  Then `session.send_command` should provide you with what you need to construct full JSON dicts and pass them to geckodriver.

Comment 21

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823859 [details]
Bug 1328726 - (wdclient) Add element property command, use value key;

https://reviewboard.mozilla.org/r/102344/#review103420

> I think maybe these may be missing `key="value"`?  Feel free to resolve this if that’s not the case.

Yep, it was missing. I added it in a couple of other places, too.

Comment 22

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823860 [details]
Bug 1328726 - Add server fixture to pytestrunner;

https://reviewboard.mozilla.org/r/102346/#review102950

> This feels rather over-confusing. All you seem to really want is function that closes over the provided `url_getter` function. I don't know if it works with pytest to write that directly, or if you can write it as a class with a `__call__` method for example.
> 
> I also think that such a function should be documented to describe the parameter (it wasn't immediately clear that the input was a function to return the server base url).

Yes, I wrote it this way to respect constraints from pytest. As far as I know, a pytest fixture can only be defined by a function, the fixture function's name provides the name of the fixture for use in tests, which is why I'm not using `__call__`, and fixture functions only accept other fixtures or a special `request` object as parameters, which is why I'm not annotating `where_is` with `@pytest.fixture`.

Will document.

Comment 23

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

https://reviewboard.mozilla.org/r/102348/#review102910

> This doesn't seem to have any information about which key was pressed, or modifiers + etc. That seems like it will be rather important for writing other tests.

The information is in `event.code`, which looks like "ShiftRight" or "KeyA". In any case, I do also need to check `event.key` for non-ascii characters, for example. 

Dropping this since it overlaps with issues related to writing more tests.

Comment 24

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

https://reviewboard.mozilla.org/r/102348/#review103428

> I think resetting the event- and key reporter for each test would be a good idea.  Could do this in a `teardown_method` function.

The document is reloaded between each test function (the scope of `test_keys_page` fixture is `function`, the default scope). This should be enough for resetting the reporters.

Comment 25

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

(In reply to Maja Frydrychowicz (:maja_zf) from comment #1)
> These will be addressed in follow-up bugs:
> * test non-printing chars, non-ascii chars, grapheme clusters: geckodriver
> 0.12 actions endpoint only supports single-character values for key actions,
> so we can't yet test non-printing characters by sending unicode code points,
> for example.

This got fixed in geckodriver 0.13 so I'll include these tests in this bug, too.

Comment 26

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

https://reviewboard.mozilla.org/r/102348/#review102910

Agreed. 

    Parsing of actions messages (I know this is strictly unrelated to this bug, but we are missing it from the testsuite).
Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1329833

    keyUp of keys that did not previously have keydown
Will add.

    Modifier keys like shift, ctrl, etc. pressing and releasing at different points in a sequence
    Correct support for non-printable keys (all the ones with rnadom PUA assigned unicode characters)
    Sending keys like é in the composed (one codepoint) and decomposed (multi-codepoint) forms
    Other codepoints that don't appear in the ascii set, particularly e.g. emoji
Will add some coverage for all of the above now that we have https://github.com/mozilla/webdriver-rust/pull/58, except... can't test multi-codepoint forms until https://github.com/mozilla/webdriver-rust/issues/59 is fixed.

Comment 27

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

https://reviewboard.mozilla.org/r/102348/#review102910

> I suggest importing the `Wait` utility from Marionette.  But it surprises me a little bit that the keys- and events aren’t dispatched in time this returns.  Also, isn’t the actions API meant to be blocking?

I've fixed actions so that they're blocking.

Comment 28

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

https://reviewboard.mozilla.org/r/102348/#review103428

> You can put this in a function `teardown_method`.  This isn’t a natural fit for `make_key_chain`.

I can't put it in a teardown_method/teardown_function because I need access to the `session` fixture. We should also avoid mixing xunit-style setup/teardown functions with pytest fixtures.

My thinking here is that `make_key_chain` is always used to perform actions, so it should be responsible for releasing those actions.

Comment 29

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Attachment: Bug 1328726 - Make performActions and releaseActions blocking;

Now the server response isn't sent until the command's Promise is
resolved.

Review commit: https://reviewboard.mozilla.org/r/104104/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104104/

Comment 30

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823858 [details]
Bug 1328726 - (wdclient) Add actions endpoints, key actions API;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102342/diff/1-2/

Comment 31

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823859 [details]
Bug 1328726 - (wdclient) Add element property command, use value key;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102344/diff/1-2/

Comment 32

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823860 [details]
Bug 1328726 - Add server fixture to pytestrunner;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102346/diff/1-2/

Comment 33

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102348/diff/1-2/

Comment 34

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

https://reviewboard.mozilla.org/r/102348/#review104824

I've addressed many of the issues raised (or my interpretation of them -- see replies) and I'd appreciate another quick round of feedback before I add a few more tests. I'm out on Thursday, back on Friday. Thanks!

::: testing/web-platform/tests/webdriver/actions.py:3
(Diff revision 2)
> +import pytest
> +
> +from support.keys import Keys

Note that Keys is from https://github.com/SeleniumHQ/selenium/blob/a1b3b478ec458941c03e02e6bc4fea4332b1645e/py/selenium/webdriver/common/keys.py, with aliases removed.

::: testing/web-platform/tests/webdriver/actions.py:52
(Diff revision 2)
> +    assert len(get_keys(key_reporter)) == 0
> +    assert len(get_events(session)) == 0
> +
> +
> +# TODO - the harness bails with TIMEOUT before all these subtests complete
> +# @pytest.mark.parametrize("name,expected", ALL_EVENTS.items())

This tests the events for all the "special" keys used in the Selenium client. Unfortunately, all the subtests count as one big test as far as the harness timeout is concerned. I fiddled around with trying to override the timeout via manifest, say, but additional changes in wptrunner may be needed to get that working so I'm holding back for now.

::: testing/web-platform/tests/webdriver/support/keys.py:91
(Diff revision 2)
> +    META = u'\ue03d'
> +    COMMAND = u'\ue03d'
> +
> +ALL_KEYS = getmembers(Keys, lambda x: type(x) == unicode)
> +
> +ALL_EVENTS = {

I generated this data based on Firefox behaviour. I only use it for `test_webdriver_special_key_sends_keydown`

Comment 35

[James Graham [:jgraham]]

Comment on attachment 8823860 [details]
Bug 1328726 - Add server fixture to pytestrunner;

https://reviewboard.mozilla.org/r/102346/#review105164

Comment 36

[James Graham [:jgraham]]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

https://reviewboard.mozilla.org/r/102348/#review105166

::: testing/web-platform/tests/test_keys_wdspec.html:7
(Diff revisions 1 - 2)
>  <meta charset=utf-8>
>  
>  <head>
>      <title>Test Keys</title>
> -    <script type="text/javascript">
> +    <script>
> +        window.allEvents = {events: []};

No need for `window.` here since `window` is the global object.

::: testing/web-platform/tests/test_keys_wdspec.html:19
(Diff revisions 1 - 2)
>          }
> +
> +        function recordEvent(type, event) {
> +            e = {"code": event.code, "key": event.key, "type": type};
> +            allEvents.events.push({
> +              "code": event.code,

Eventually you will need to know about alt, shoft, ctrl, etc.

::: testing/web-platform/tests/webdriver/actions.py:122
(Diff revisions 1 - 2)
> +        ("keyUp", value)
> +    )
> +    session.actions.perform_actions(actions)
> +    expected = [
> +        {"code": code, "key": key, "type": "down"},
> +        {"code": code, "key": key, "type": "up"},

Do we know how standard not sending keypress is in this case?

::: testing/web-platform/tests/webdriver/support/keys.py:24
(Diff revision 2)
> +The Keys implementation.
> +"""
> +
> +from inspect import getmembers
> +
> +class Keys(object):

The spec has more special key codes than Selenium has historically supported, probably worth updating this list.

Comment 37

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

https://reviewboard.mozilla.org/r/102348/#review105166

> Do we know how standard not sending keypress is in this case?

Non-printable character means no keypress (according to the standard anyway). Chrome/Opera/Safari seem to respect this, Firefox sends keypress on some non-printable keys like arrows.

> The spec has more special key codes than Selenium has historically supported, probably worth updating this list.

I guess you're talking about the ones listed in the first table here? https://w3c.github.io/webdriver/webdriver-spec.html#h-keyboard-actions

Comment 38

[James Graham [:jgraham]]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

https://reviewboard.mozilla.org/r/102348/#review105166

> Non-printable character means no keypress (according to the standard anyway). Chrome/Opera/Safari seem to respect this, Firefox sends keypress on some non-printable keys like arrows.

Bleh. OK. I wonder if we can make it optional for the cases where Fx is wrong (because the *webdriver* standard intentionally doesn't have a strong opinion on whether you get this stuff right, just that you should be consistent with whatever you do in non-webdrvier scenarios).

> I guess you're talking about the ones listed in the first table here? https://w3c.github.io/webdriver/webdriver-spec.html#h-keyboard-actions

Yes.

Comment 39

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Happy try run with extra logging and long timeout: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9088ac363795f001374aa72684d5959b769f566f

Comment 40

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102348/diff/2-3/

Comment 41

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823857 [details]
Bug 1328726 - Style fix ups in Marionette key actions implementation;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102340/diff/1-2/

Comment 42

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8826059 [details]
Bug 1328726 - Make performActions and releaseActions blocking;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104104/diff/1-2/

Comment 43

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823858 [details]
Bug 1328726 - (wdclient) Add actions endpoints, key actions API;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102342/diff/2-3/

Comment 44

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823859 [details]
Bug 1328726 - (wdclient) Add element property command, use value key;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102344/diff/2-3/

Comment 45

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823860 [details]
Bug 1328726 - Add server fixture to pytestrunner;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102346/diff/2-3/

Comment 46

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102348/diff/3-4/

Comment 47

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Attachment: Bug 1328726 - Add wdspec test for emoji in key actions;

Review commit: https://reviewboard.mozilla.org/r/105316/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105316/

Comment 48

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102348/diff/4-5/

Comment 49

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102348/diff/5-6/

Comment 50

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8827688 [details]
Bug 1328726 - Add wdspec test for emoji in key actions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105316/diff/1-2/

Comment 51

[James Graham [:jgraham]]

Comment on attachment 8827688 [details]
Bug 1328726 - Add wdspec test for emoji in key actions;

https://reviewboard.mozilla.org/r/105316/#review106286

I think I need to be convinced that this special handling of surrogates is not just a bug in marionette.

::: testing/web-platform/tests/test_keys_wdspec.html:24
(Diff revision 2)
> +         * Example: given "\ud83d" return "\\\\ud83d".
> +         *
> +         * Otherwise JSON.stringify will convert it to U+FFFD (REPLACEMENT CHARACTER)
> +         * when returning a value from executeScript, for example.
> +         */
> +        function escapeSurrogateHalf(key) {

This feels like a workaround for more general brokenness here; we shouldn't be leaking details of UCS2 into event.key.

::: testing/web-platform/tests/webdriver/actions.py:151
(Diff revision 2)
> +    )
> +    session.actions.perform_actions(actions)
> +    full_events = get_events(session)
> +    expected = [
> +        {"code": code, "type": "down", "key": value},
> +        # We get two keypress events (one for each surrogate half)

So this doesn't seem like the intent of the spec. Can we test what different browsers do if you actually bind a key to a specific emoji? (I tried to do this under Linux but got stuck working out how to specify that keybinding).

Comment 52

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8826059 [details]
Bug 1328726 - Make performActions and releaseActions blocking;

https://reviewboard.mozilla.org/r/104104/#review106350

Comment 53

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8827688 [details]
Bug 1328726 - Add wdspec test for emoji in key actions;

https://reviewboard.mozilla.org/r/105316/#review106286

I see the same behaviour when Marionette is not involved. Details below.

> This feels like a workaround for more general brokenness here; we shouldn't be leaking details of UCS2 into event.key.

I see this as preventing the details of character encodings at different levels of the stack (os, gecko, marionette server, wdclient) from interfering with what we see in our tests. In other words, we need this to be able to inspect the actual key value recorded in the keypress event, rather than the "REPLACEMENT CHARACTER" that was ending up in the JSON reponse. It just so happens that I encountered surrogate pairs because I was testing on mac (see other comment), but I think it may be useful in general.

> So this doesn't seem like the intent of the spec. Can we test what different browsers do if you actually bind a key to a specific emoji? (I tried to do this under Linux but got stuck working out how to specify that keybinding).

After some digging I think the surrogate pairs situation is actually specific to mac and windows here [1]. Regardless, the event properties are inconsistent across browsers [2].  I think the best we can do for now is just check the value of the input field, and just the event types: a keydown followed by a keyup.

[1] Try `len(u"\U0001F60D")` in python on linux vs mac vs win.
[2] I used xmodmap in a linux VM to bind an emoji character to a key, then observed behaviour of https://w3c.github.io/uievents/tools/key-event-viewer.html in different browsers.

Comment 54

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823857 [details]
Bug 1328726 - Style fix ups in Marionette key actions implementation;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102340/diff/2-3/

Comment 55

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8826059 [details]
Bug 1328726 - Make performActions and releaseActions blocking;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104104/diff/2-3/

Comment 56

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823858 [details]
Bug 1328726 - (wdclient) Add actions endpoints, key actions API;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102342/diff/3-4/

Comment 57

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823859 [details]
Bug 1328726 - (wdclient) Add element property command, use value key;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102344/diff/3-4/

Comment 58

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823860 [details]
Bug 1328726 - Add server fixture to pytestrunner;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102346/diff/3-4/

Comment 59

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102348/diff/6-7/

Comment 60

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8827688 [details]
Bug 1328726 - Add wdspec test for emoji in key actions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105316/diff/2-3/

Comment 61

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102348/diff/7-8/

Comment 62

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8827688 [details]
Bug 1328726 - Add wdspec test for emoji in key actions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105316/diff/3-4/

Comment 63

[James Graham [:jgraham]]

Comment on attachment 8827688 [details]
Bug 1328726 - Add wdspec test for emoji in key actions;

https://reviewboard.mozilla.org/r/105316/#review106286

> After some digging I think the surrogate pairs situation is actually specific to mac and windows here [1]. Regardless, the event properties are inconsistent across browsers [2].  I think the best we can do for now is just check the value of the input field, and just the event types: a keydown followed by a keyup.
> 
> [1] Try `len(u"\U0001F60D")` in python on linux vs mac vs win.
> [2] I used xmodmap in a linux VM to bind an emoji character to a key, then observed behaviour of https://w3c.github.io/uievents/tools/key-event-viewer.html in different browsers.

OK, so there are several pieces here. Let me state my understanding so that we're on the same page.

*Python* is confusing because you can compile it in two modes, roughly UCS2 mode or UCS4 mode. In the latter all codepoints are stored as 4 bytes so there isn't a difference between BMP and non-BMP characters. In the former, which is the default on OSX for some reason, non-BMP characters are stored in a UTF-16 style surrogate pair, but both halves of the surrogate are individually addressable. This is basically broken, and leads to the differing `len` you see on OSX vs Linux (Windows is, of course, special because there are multiple possible ways to have Python installed and they might all have different behaviours).

*Javascript* is just always broken. It stores strings as UCS2 so you always see non-BMP characters as two halves of a surrogate pair.

*JSON* is also pretty dumb. It seems to either encode non-BMP as two escape sequences representing the two parts of a surrogate pair or as a single UTF8 encoded character. In our case we seem to be prefering the two-escapes encoding.

*Rust* stores everything as UTF8 internally and iterates over codepoints rather than bytes.

So I would expect that when you send an action using WebDriver and then get the result, the following steps occur:

In python it's stored as either a single 4 byte character or 2 2 byte characters. Then it's transformed to 2 escape sequences for a low and a high surrogate in JSON. In rust those two escapes are converted to a UTF8 string, and then converted either back into two escapes, or left as a UTF8 string to send to marionette. It will decode whatever as a pair of UCS2 surrogates (which may change representation again if they are stored as a non-JS string in the browser). Going the other way, roughly the opposite should happen.

So the pipeline seems like it should be non-BMP safe as long as we don't try and do something like take a single "character" at a time to work with in Python or JS, as that may end up with us operating on half of a surroagate pair at a time, which would cause badness to happen.

Does that seem correct? What am I missing?

Comment 64

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8823858 [details]
Bug 1328726 - (wdclient) Add actions endpoints, key actions API;

https://reviewboard.mozilla.org/r/102342/#review107070

::: testing/web-platform/tests/tools/webdriver/webdriver/client.py:72
(Diff revision 4)
> +class Actions(object):
> +    def __init__(self, session):
> +        self.session = session
> +
> +    @command
> +    def perform_actions(self, actions):
> +        body = {"actions": actions}
> +        return self.session.send_command("POST", "actions", body)
> +
> +    @command
> +    def release_actions(self):
> +        return self.session.send_command("DELETE", "actions")

Did we conclude not flesh out a more complete user-friendly `Actions` class API?

If you decided not to do that for this changeset, I would remove this class and use `session.send_command` in the tests so that we don’t expose a public API that will break once we figure out the higher-level API.

Comment 65

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

https://reviewboard.mozilla.org/r/102348/#review107072

::: testing/web-platform/tests/webdriver/actions.py:7
(Diff revision 8)
> +
> +from support.keys import Keys
> +
> +
> +def get_events(session):
> +    """ Get list of key events recorded in |test_keys_page| """

Can you make the docstrings in this file PEP8 conforming?

::: testing/web-platform/tests/webdriver/actions.py:8
(Diff revision 8)
> +from support.keys import Keys
> +
> +
> +def get_events(session):
> +    """ Get list of key events recorded in |test_keys_page| """
> +    return session.execute_script("return window.allEvents;")["events"]

Faster to return the events element directly, e.g.

```python
execute_script("return window.allEvents.events")
```

::: testing/web-platform/tests/webdriver/actions.py:11
(Diff revision 8)
> +def get_keys(input):
> +    """ Get printable characters entered into |input|."""
> +    return input.property("value") or ""

Use `is not None` here because it might return an empty string if `input.property("value")` is falsy.

::: testing/web-platform/tests/webdriver/actions.py:34
(Diff revision 8)
> [e-mail]
> +def make_key_chain(request, session):
> +    chain = {
> +          "type": "key",
> +          "id": "keyboard",
> +          "actions": []
> +    }
> +
> +    def maker(*args):
> +        chain["actions"] = [{"type": t, "value": v} for t, v in args]
> +        return [chain]
> +    # clean up between tests
> +    request.addfinalizer(session.actions.release_actions)
> +    return maker

As far as I can tell, `make_key_chain` doesn’t need to be a fixture at all?  It could just be a helper function?

The only dependency on other fixtures in it is the `request.addfinalizer` line, which could be done in a `teardown_function`.

::: testing/web-platform/tests/webdriver/actions.py:52
(Diff revision 8)
> +    assert len(get_keys(key_reporter)) == 0
> +    assert len(get_events(session)) == 0

I think it would be nicer if these were methods on the `key_reporter`, but this approach is fine too.

::: testing/web-platform/tests/webdriver/actions.py:66
(Diff revision 8)
> +# TODO - the harness bails with TIMEOUT before all these subtests complete
> +# The timeout is per file, so move to separate file with longer timeout?
> +# Need a way to set timeouts in py files (since can't do html meta)

I noticed that when I implemented the pytest runner in wptrunner.  Need to dig a bit deeper into pytest to figure out how to disable that timeout.

::: testing/web-platform/tests/webdriver/actions.py:99
(Diff revision 8)
> +    (u"\u00E0", ""),
> +    (u"\u0416", ""),
> +    (u"@", "Digit2"),
> +    (u"\uF6C2", ""),  # PUA

I wonder if we should predefine these Unicode characters so they’re more easily readable?

::: testing/web-platform/tests/webdriver/support/keys.py:32
(Diff revision 8)
> +    Set of special keys codes.
> +
> +    See also https://w3c.github.io/webdriver/webdriver-spec.html#h-keyboard-actions
> +    """
> +
> +    NULL = u'\ue000'

Can we please use double quotes throughout.

Comment 66

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8827688 [details]
Bug 1328726 - Add wdspec test for emoji in key actions;

https://reviewboard.mozilla.org/r/105316/#review106286

> OK, so there are several pieces here. Let me state my understanding so that we're on the same page.
> 
> *Python* is confusing because you can compile it in two modes, roughly UCS2 mode or UCS4 mode. In the latter all codepoints are stored as 4 bytes so there isn't a difference between BMP and non-BMP characters. In the former, which is the default on OSX for some reason, non-BMP characters are stored in a UTF-16 style surrogate pair, but both halves of the surrogate are individually addressable. This is basically broken, and leads to the differing `len` you see on OSX vs Linux (Windows is, of course, special because there are multiple possible ways to have Python installed and they might all have different behaviours).
> 
> *Javascript* is just always broken. It stores strings as UCS2 so you always see non-BMP characters as two halves of a surrogate pair.
> 
> *JSON* is also pretty dumb. It seems to either encode non-BMP as two escape sequences representing the two parts of a surrogate pair or as a single UTF8 encoded character. In our case we seem to be prefering the two-escapes encoding.
> 
> *Rust* stores everything as UTF8 internally and iterates over codepoints rather than bytes.
> 
> So I would expect that when you send an action using WebDriver and then get the result, the following steps occur:
> 
> In python it's stored as either a single 4 byte character or 2 2 byte characters. Then it's transformed to 2 escape sequences for a low and a high surrogate in JSON. In rust those two escapes are converted to a UTF8 string, and then converted either back into two escapes, or left as a UTF8 string to send to marionette. It will decode whatever as a pair of UCS2 surrogates (which may change representation again if they are stored as a non-JS string in the browser). Going the other way, roughly the opposite should happen.
> 
> So the pipeline seems like it should be non-BMP safe as long as we don't try and do something like take a single "character" at a time to work with in Python or JS, as that may end up with us operating on half of a surroagate pair at a time, which would cause badness to happen.
> 
> Does that seem correct? What am I missing?

That all sounds right to me, except the last bit: _"as long as we don't try and do something like take a single "character" at a time to work with in Python or JS, as that may end up with us operating on half of a surroagate pair at a time, which would cause badness to happen'_

Using Firefox directly (no Marionette), I see two keypress events for non-BMP characters on mac: one for each surrogate half, which is recorded in event.key. The keydown and keyup events refer to the whole character, so this is only for keypress. 

From that I conclude that we're not doing anything in the Marionette/Webdriver stack to force taking one "character" at a time. The events generated by the browser can refer to surrogate halves, so I'm proposing a way for our tests to be able to examine that event data accurately.

Comment 67

[James Graham [:jgraham]]

Comment on attachment 8827688 [details]
Bug 1328726 - Add wdspec test for emoji in key actions;

https://reviewboard.mozilla.org/r/105316/#review106286

> That all sounds right to me, except the last bit: _"as long as we don't try and do something like take a single "character" at a time to work with in Python or JS, as that may end up with us operating on half of a surroagate pair at a time, which would cause badness to happen'_
> 
> Using Firefox directly (no Marionette), I see two keypress events for non-BMP characters on mac: one for each surrogate half, which is recorded in event.key. The keydown and keyup events refer to the whole character, so this is only for keypress. 
> 
> From that I conclude that we're not doing anything in the Marionette/Webdriver stack to force taking one "character" at a time. The events generated by the browser can refer to surrogate halves, so I'm proposing a way for our tests to be able to examine that event data accurately.

Woah, really? OK that's pure crazy.

Thanks for sticking with me and explaining that enough times that I finally got it. I guess in that case we have to deal with this case :/

Comment 68

[James Graham [:jgraham]]

Comment on attachment 8827688 [details]
Bug 1328726 - Add wdspec test for emoji in key actions;

https://reviewboard.mozilla.org/r/105316/#review107112

::: testing/web-platform/tests/test_keys_wdspec.html:30
(Diff revision 4)
> +          if (typeof key !== 'undefined' && key.length === 1) {
> +            var charCode = key.charCodeAt(0);
> +            var highSurrogate = charCode >= 0xD800 && charCode <= 0xDBFF;
> +            var surrogate = highSurrogate || (charCode >= 0xDC00 && charCode <= 0xDFFF);
> +            if (surrogate) {
> +              key = "\\\\u" + charCode.toString(16);

Just for the sake of fewer `\\\\` I wonder if we could encode these as U+XXXX?

::: testing/web-platform/tests/webdriver/actions.py:139
(Diff revision 4)
> [e-mail]("value", [
> +    (u"\U0001F604"),
> +    (u"\U0001F60D"),
> +])
> +def test_single_emoji_records_correct_key(session,
> +                                           key_reporter,

Looks like the indentation is slightly off here.

Comment 69

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

https://reviewboard.mozilla.org/r/102348/#review107072

> As far as I can tell, `make_key_chain` doesn’t need to be a fixture at all?  It could just be a helper function?
> 
> The only dependency on other fixtures in it is the `request.addfinalizer` line, which could be done in a `teardown_function`.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1328726#c28

> I noticed that when I implemented the pytest runner in wptrunner.  Need to dig a bit deeper into pytest to figure out how to disable that timeout.

Filed https://github.com/w3c/wptrunner/issues/229

Comment 70

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823858 [details]
Bug 1328726 - (wdclient) Add actions endpoints, key actions API;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102342/diff/4-5/

Comment 71

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823859 [details]
Bug 1328726 - (wdclient) Add element property command, use value key;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102344/diff/4-5/

Comment 72

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823860 [details]
Bug 1328726 - Add server fixture to pytestrunner;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102346/diff/4-5/

Comment 73

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102348/diff/8-9/

Comment 74

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8827688 [details]
Bug 1328726 - Add wdspec test for emoji in key actions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105316/diff/4-5/

Comment 75

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823858 [details]
Bug 1328726 - (wdclient) Add actions endpoints, key actions API;

https://reviewboard.mozilla.org/r/102342/#review107070

> Did we conclude not flesh out a more complete user-friendly `Actions` class API?
> 
> If you decided not to do that for this changeset, I would remove this class and use `session.send_command` in the tests so that we don’t expose a public API that will break once we figure out the higher-level API.

I cobbled something together today to address this. See latest revision. Good enough to start with?

Comment 76

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823857 [details]
Bug 1328726 - Style fix ups in Marionette key actions implementation;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102340/diff/3-4/

Comment 77

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8826059 [details]
Bug 1328726 - Make performActions and releaseActions blocking;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104104/diff/3-4/

Comment 78

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823858 [details]
Bug 1328726 - (wdclient) Add actions endpoints, key actions API;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102342/diff/5-6/

Comment 79

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823859 [details]
Bug 1328726 - (wdclient) Add element property command, use value key;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102344/diff/5-6/

Comment 80

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823860 [details]
Bug 1328726 - Add server fixture to pytestrunner;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102346/diff/5-6/

Comment 81

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102348/diff/9-10/

Comment 82

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8827688 [details]
Bug 1328726 - Add wdspec test for emoji in key actions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105316/diff/5-6/

Comment 83

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8823859 [details]
Bug 1328726 - (wdclient) Add element property command, use value key;

https://reviewboard.mozilla.org/r/102344/#review107850

Can you change the commit message to not include special characters?

Comment 84

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

https://reviewboard.mozilla.org/r/102348/#review107072

> Filed https://github.com/w3c/wptrunner/issues/229

FWIW I didn’t mean for this to be an issue.

> I wonder if we should predefine these Unicode characters so they’re more easily readable?

This is probably not very important right now.

Comment 85

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

https://reviewboard.mozilla.org/r/102348/#review107852

::: testing/web-platform/tests/test_keys_wdspec.html:29
(Diff revision 10)
> +            "meta": event.metaKey,
> +            "shift": event.shiftKey,
> +            "repeat": event.repeat,
> +            "type": event.type
> +          });
> +          appendMessage(`${event.type}(code:${event.code}, key:${event.key}, which:${event.which})`);

Template literals aren’t supported in IE, but I guess this is fine since Microsoft’s implementation is for Edge.

::: testing/web-platform/tests/test_keys_wdspec.html:34
(Diff revision 10)
> +          appendMessage(`${event.type}(code:${event.code}, key:${event.key}, which:${event.which})`);
> +        }
> +
> +        function resetEvents() {
> +            allEvents.events.length = 0;
> +            displayMessage('');

Let us not mix single quotes and double quotes.

::: testing/web-platform/tests/test_keys_wdspec.html:55
(Diff revision 10)
> +  </div>
> +
> +</body>
> +</html>
> +
> +

Superfluous newline.

::: testing/web-platform/tests/webdriver/actions.py:11
(Diff revision 10)
> +def get_events(session):
> +    """Return list of key events recorded in the test_keys_page fixture."""
> +    return session.execute_script("return allEvents.events;") or []
> +
> +
> +def get_keys(input):

Give `input` variable another name since it is a built-in in Python.

::: testing/web-platform/tests/webdriver/actions.py:12
(Diff revision 10)
> +    """Return list of key events recorded in the test_keys_page fixture."""
> +    return session.execute_script("return allEvents.events;") or []
> +
> +
> +def get_keys(input):
> +    """Get printable characters entered into |input|.

Backtick so as to support Sphinx/Python docstrings.

::: testing/web-platform/tests/webdriver/actions.py:24
(Diff revision 10)
> +    else:
> +        return rv
> +
> +
> +def filter_dict(source, d):
> +    """Filter |source| dict to only contain same keys as |d| dict.

Backtick.

::: testing/web-platform/tests/webdriver/actions.py:34
(Diff revision 10)
> +    return {k: source[k] for k in d.keys()}
> +
> +
> [e-mail]
> +def key_reporter(session, test_keys_page, request):
> +    """ Represents focused input element on |test_keys_page| """

Whitespace at beginning of docstring and use backticks in place of pipe character.

Should probably also expand this docstring to highlight that getting the key reporter clicks and shifts focus to it.

::: testing/web-platform/tests/webdriver/actions.py:35
(Diff revision 10)
> +    input = session.find.css("#keys", all=False)
> +    input.click()
> +    return input

Same as above: Give this variable a different name.

Comment 86

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

https://reviewboard.mozilla.org/r/102348/#review103428

> I can't put it in a teardown_method/teardown_function because I need access to the `session` fixture. We should also avoid mixing xunit-style setup/teardown functions with pytest fixtures.
> 
> My thinking here is that `make_key_chain` is always used to perform actions, so it should be responsible for releasing those actions.

You need to access `session` because you add a finaliser, but if you hadn’t needed to do that `make_key_chain` wouldn’t need to be a fixture.  Apart from ensuring to release actions when the test is done, which isn’t explicit from the function’s name, all this function does is to construct a JSON structure.

> My thinking here is that make_key_chain is always used to perform actions, so it should be responsible for releasing those actions.

It’s not used to _perform_ actions at all: it’s only used to construct the dict body passed to `session.actions.perform_actions`.

I don’t think this is super-important so I will drop the issue, but I am concerned about the tight coupling and implicitness of `make_key_chain`.

> I would like these assertions to be nicer.  This relates to the issue I raised earlier about providing some sort of Python abstraction on top of the key reporter.
> 
> A random idea:
> 
> ```python
> def test_empty_actions_chain(session, keys, events):
>     Actions(session).perform()
>     assert len(keys) == 0
>     assert len(events) == 0
> ```
> 
> We could empty the key- and event reporter after each test, possibly also reload the document?
> 
> Making the reporters indexed would also have other advantages: As you construct the action chains you have a clear picture of what DOM events and keys should be dispatched.  An indexed event reporter would let you iterate of the events or even compare them to a predefined dictionary.
> 
> ```python
> def test_press_key(session, keys, events):
>     Actions(session).key_down("a").key_up("a").perform()
>     assert keys[0] == "a"
>     assert events[0].type == "keydown"
>     assert events[0].code == keys[0]
>     assert events[0].ctrlKey == False
>     assert events[0].shiftKey == False
>     …
>     assert events[1].type == "keypress"
>     …
>     assert events[2].type == "keyup"
> ```

Thanks for following up on this.  I find the new assertions much easier to read.

> The document is reloaded between each test function (the scope of `test_keys_page` fixture is `function`, the default scope). This should be enough for resetting the reporters.

Yes, that sounds good enough to me.

Comment 87

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8827688 [details]
Bug 1328726 - Add wdspec test for emoji in key actions;

https://reviewboard.mozilla.org/r/105316/#review107078

::: testing/web-platform/tests/webdriver/actions.py:10
(Diff revision 4)
>  
>  def get_events(session):
> -    """ Get list of key events recorded in |test_keys_page| """
> -    return session.execute_script("return window.allEvents;")["events"]
> +    """Get list of key events recorded in |test_keys_page|."""
> +    events = session.execute_script("return window.allEvents;")["events"]
> +    # |key| values in |allEvents| may be escaped (see |escapeSurrogateHalf| in
> +    # |test_keys_page|), so this converts them back into unicode literals.

test_keys_wdspec.html?

::: testing/web-platform/tests/webdriver/actions.py:13
(Diff revision 6)
> +        if e["key"].startswith(u'U+'):
> +            key = e["key"]
> +            hex_suffix = key[key.index('+') + 1:]
> +            e["key"] = unichr(int(hex_suffix, 16))

Please don’t mix quotes.

::: testing/web-platform/tests/webdriver/actions.py:137
(Diff revision 6)
> +    (u"\U0001F60D"),
> +])
> +def test_single_emoji_records_correct_key(session, key_reporter, key_chain, value):
> +    # Not using key_chain.send_keys() because we always want to treat value as
> +    # one character here. |len(value)| varies by platform for non-BMP characters,
> +    # so we don't want to interate over value.

iterate?

Comment 88

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8823858 [details]
Bug 1328726 - (wdclient) Add actions endpoints, key actions API;

https://reviewboard.mozilla.org/r/102342/#review107848

::: testing/web-platform/tests/tools/webdriver/webdriver/client.py:75
(Diff revision 6)
> +        """Represents a sequence of actions of one type for one input source.
> +
> +        Each action method adds one or more actions to a queue. When perform()
> +        is called, the queued actions fire in order.
> +
> +        May be chained together as in:
> +
> +             ActionSequence(session, "key", id) \
> +                .key_down("a") \
> +                .key_up("a") \
> +                .perform()

This should probably be on the `ActionSequence` object instead of the ctor.

::: testing/web-platform/tests/tools/webdriver/webdriver/client.py:80
(Diff revision 6)
> +        """Represents a sequence of actions of one type for one input source.
> +
> +        Each action method adds one or more actions to a queue. When perform()
> +        is called, the queued actions fire in order.
> +
> +        May be chained together as in:

Use the two colon `::` postfix to indicate the following block is a code example.

::: testing/web-platform/tests/tools/webdriver/webdriver/client.py:114
(Diff revision 6)
> +
> +    def _key_action(self, subtype, value):
> +        self._actions.append({"type": subtype, "value": value})
> +
> +    def key_up(self, value):
> +        """Queue a keyUp action for |value|.

I guess | should be a backtick.

::: testing/web-platform/tests/tools/webdriver/webdriver/client.py:122
(Diff revision 6)
> +        """
> +        self._key_action("keyUp", value)
> +        return self
> +
> +    def key_down(self, value):
> +        """Queue a keyDown action for |value|.

Backtick.

::: testing/web-platform/tests/tools/webdriver/webdriver/client.py:130
(Diff revision 6)
> +        """
> +        self._key_action("keyDown", value)
> +        return self
> +
> +    def send_keys(self, keys):
> +        """Queue a keyDown and keyUp action for each character in |keys|.

Backtick.

::: testing/web-platform/tests/tools/webdriver/webdriver/client.py:146
(Diff revision 6)
> +    def __init__(self, session):
> +        self.session = session
> +
> +    @command
> +    def perform(self, actions=None):
> +        """Performs actions by tick from each action sequence in |actions|.

Backtick.

::: testing/web-platform/tests/tools/webdriver/webdriver/client.py:148
(Diff revision 6)
> +
> +    @command
> +    def perform(self, actions=None):
> +        """Performs actions by tick from each action sequence in |actions|.
> +
> +        :param actions: list of input source action sequences. A single action

s/list/List/

::: testing/web-platform/tests/tools/webdriver/webdriver/client.py:150
(Diff revision 6)
> +    def perform(self, actions=None):
> +        """Performs actions by tick from each action sequence in |actions|.
> +
> +        :param actions: list of input source action sequences. A single action
> +                        sequence may be created with the help of
> +                        ActionSequence.dict.

Use double backticks on references to types.

::: testing/web-platform/tests/tools/webdriver/webdriver/client.py:152
(Diff revision 6)
> +
> +        :param actions: list of input source action sequences. A single action
> +                        sequence may be created with the help of
> +                        ActionSequence.dict.
> +        """
> +        body = {"actions": actions or []}

Use the `if actions is not None` style since passing `perform(actions=False)` (or any other falsy value) will cause an empty list to be used.

::: testing/web-platform/tests/tools/webdriver/webdriver/client.py:155
(Diff revision 6)
> +    @command
> +    def release(self):
> +        return self.session.send_command("DELETE", "actions")
> +
> +    def sequence(self, *args, **kwargs):
> +        """Return an empty ActionSequence of the designated type.
> +
> +        See ActionSequence for parameter list.
> +        """
> +        return ActionSequence(self.session, *args, **kwargs)

After much debating with myself, I think this API makes sense now.

Background: The Selenium clients don’t have a separate command to release modifiers because state isn’t preserved across calls action chains.

An alternative API would be:

```python
ActionSequence(session, "action type", "input id") \
    .key_down("a")
    .perform()
```

But this seems equally good, for as long as you’re aware that `session.actions.sequence` returns a new builder object:

```python
session.actions.sequence("action type", "input id") \
    .key_down("a")
    .perform()
```

It _could_ be argued that this is a bit tightly coupled, but I’m fine with it.

Comment 89

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

https://reviewboard.mozilla.org/r/102348/#review103428

> You need to access `session` because you add a finaliser, but if you hadn’t needed to do that `make_key_chain` wouldn’t need to be a fixture.  Apart from ensuring to release actions when the test is done, which isn’t explicit from the function’s name, all this function does is to construct a JSON structure.
> 
> > My thinking here is that make_key_chain is always used to perform actions, so it should be responsible for releasing those actions.
> 
> It’s not used to _perform_ actions at all: it’s only used to construct the dict body passed to `session.actions.perform_actions`.
> 
> I don’t think this is super-important so I will drop the issue, but I am concerned about the tight coupling and implicitness of `make_key_chain`.

I appreciate this discussion and I have an idea!

* I need a way to call `session.actions.release` after each test function.
* Using `teardown_function` won't work because I need the `session` fixture to release actions -- I can't access any fixtures in `teardown_function`. 
* But... I just realized I can create an "autouse" fixture that just contains the finalizer to release actions. No more tight coupling.

Comment 90

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

https://reviewboard.mozilla.org/r/102348/#review103428

> I appreciate this discussion and I have an idea!
> 
> * I need a way to call `session.actions.release` after each test function.
> * Using `teardown_function` won't work because I need the `session` fixture to release actions -- I can't access any fixtures in `teardown_function`. 
> * But... I just realized I can create an "autouse" fixture that just contains the finalizer to release actions. No more tight coupling.

I see what you mean by “fixture dependency” now that you spelt it out to me: You can’t use a teardown function because you rely on a fixture and fixtures aren’t available to XUnit-styled primitives because they break the way fixtures are intended to work.  Got it!

The way you describe “autouse” fixtures sounds as if it would solve my concerns, but don’t make it a priority if you have more important things to do.

Comment 91

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823858 [details]
Bug 1328726 - (wdclient) Add actions endpoints, key actions API;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102342/diff/6-7/

Comment 92

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823859 [details]
Bug 1328726 - (wdclient) Add element property command, use value key;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102344/diff/6-7/

Comment 93

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823860 [details]
Bug 1328726 - Add server fixture to pytestrunner;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102346/diff/6-7/

Comment 94

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102348/diff/10-11/

Comment 95

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8827688 [details]
Bug 1328726 - Add wdspec test for emoji in key actions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105316/diff/6-7/

Comment 96

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823857 [details]
Bug 1328726 - Style fix ups in Marionette key actions implementation;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102340/diff/4-5/

Comment 97

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8826059 [details]
Bug 1328726 - Make performActions and releaseActions blocking;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104104/diff/4-5/

Comment 98

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823858 [details]
Bug 1328726 - (wdclient) Add actions endpoints, key actions API;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102342/diff/7-8/

Comment 99

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823859 [details]
Bug 1328726 - (wdclient) Add element property command, use value key;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102344/diff/7-8/

Comment 100

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823860 [details]
Bug 1328726 - Add server fixture to pytestrunner;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102346/diff/7-8/

Comment 101

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102348/diff/11-12/

Comment 102

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8827688 [details]
Bug 1328726 - Add wdspec test for emoji in key actions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105316/diff/7-8/

Comment 103

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ec1a2cd38ffe
Style fix ups in Marionette key actions implementation; r=ato
https://hg.mozilla.org/integration/autoland/rev/a2520d52c7cc
Make performActions and releaseActions blocking; r=ato
https://hg.mozilla.org/integration/autoland/rev/0c10680f0705
(wdclient) Add actions endpoints, key actions API; r=ato
https://hg.mozilla.org/integration/autoland/rev/40fceb2644d7
(wdclient) Add element property command, use value key; r=ato
https://hg.mozilla.org/integration/autoland/rev/5606cc9b0654
Add server fixture to pytestrunner; r=ato,jgraham
https://hg.mozilla.org/integration/autoland/rev/0e6e8d3fccec
Add web-platform wdspec tests for key actions; r=ato,jgraham
https://hg.mozilla.org/integration/autoland/rev/93218dc2249f
Add wdspec test for emoji in key actions; r=ato,jgraham

Comment 104

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Setting n-i so we remember to upstream this to wptrunner and wdclient.

Comment 105

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back these out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=71640584&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/8ba0ac4a69175b8fcc5583f6831ac08a0d2d441b

Comment 106

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8823861 [details]
Bug 1328726 - Add web-platform wdspec tests for key actions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102348/diff/12-13/

Comment 107

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8827688 [details]
Bug 1328726 - Add wdspec test for emoji in key actions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105316/diff/8-9/

Comment 108

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Attachment: Bug 1328726 - Disable wdspec tests on linux64 debug;

The wdspec tests in contexts.py already fail silently on linux64
debug. After adding the actions tests, which pass, the job turns
orange so let's disable the failing tests for now.

Review commit: https://reviewboard.mozilla.org/r/107170/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107170/

Comment 109

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8830294 [details]
Bug 1328726 - Disable wdspec tests on linux64 debug;

https://reviewboard.mozilla.org/r/107170/#review108248

* on try: green Wd job with actions tests, but contexts.py is disabled: https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&selectedJob=71895486

What this fixes:
* on m-c: incorrectly green Wd job with "INTERNAL ERROR" for contexts.py: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&filter-searchStr=Wd&selectedJob=71892462
* on try: orange Wd job that runs both actions.py and contexts.py: https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&selectedJob=71510930

Comment 110

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Updated manifest and fixed the orange on linux64 debug described above.

Comment 111

[James Graham [:jgraham]]

Comment on attachment 8830294 [details]
Bug 1328726 - Disable wdspec tests on linux64 debug;

https://reviewboard.mozilla.org/r/107170/#review108258

Comment 112

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8830294 [details]
Bug 1328726 - Disable wdspec tests on linux64 debug;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107170/diff/1-2/

Comment 113

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2143f1aa8087
Style fix ups in Marionette key actions implementation; r=ato
https://hg.mozilla.org/integration/autoland/rev/4430151d0cb4
Make performActions and releaseActions blocking; r=ato
https://hg.mozilla.org/integration/autoland/rev/6be0203f1a26
(wdclient) Add actions endpoints, key actions API; r=ato
https://hg.mozilla.org/integration/autoland/rev/be79eb6f9d64
(wdclient) Add element property command, use value key; r=ato
https://hg.mozilla.org/integration/autoland/rev/6d9673eedd91
Add server fixture to pytestrunner; r=ato,jgraham
https://hg.mozilla.org/integration/autoland/rev/c039479ce446
Add web-platform wdspec tests for key actions; r=ato,jgraham
https://hg.mozilla.org/integration/autoland/rev/de5830f1b8ab
Add wdspec test for emoji in key actions; r=ato,jgraham
https://hg.mozilla.org/integration/autoland/rev/1039c2ca90ee
Disable contexts.py wdspec tests on linux64 debug; r=jgraham

Comment 114

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back these out again for webdriver bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=71915732&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/6bac3fb644fa132229d1e787b50b9b1325f43234

Comment 115

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8830294 [details]
Bug 1328726 - Disable wdspec tests on linux64 debug;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107170/diff/2-3/

Comment 116

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f85f7608bc7
Style fix ups in Marionette key actions implementation; r=ato
https://hg.mozilla.org/integration/autoland/rev/c92e10169830
Make performActions and releaseActions blocking; r=ato
https://hg.mozilla.org/integration/autoland/rev/ad2286346bc0
(wdclient) Add actions endpoints, key actions API; r=ato
https://hg.mozilla.org/integration/autoland/rev/dd3640717dd0
(wdclient) Add element property command, use value key; r=ato
https://hg.mozilla.org/integration/autoland/rev/3c11339b3fd3
Add server fixture to pytestrunner; r=ato,jgraham
https://hg.mozilla.org/integration/autoland/rev/024a926ab622
Add web-platform wdspec tests for key actions; r=ato,jgraham
https://hg.mozilla.org/integration/autoland/rev/6d077ab9ce5f
Add wdspec test for emoji in key actions; r=ato,jgraham
https://hg.mozilla.org/integration/autoland/rev/8e586e5b7c29
Disable wdspec tests on linux64 debug; r=jgraham

Comment 117

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

I've disabled wdspec tests in linux 64 debug and updated Bug 1318724.

Comment 118

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2f85f7608bc7
https://hg.mozilla.org/mozilla-central/rev/c92e10169830
https://hg.mozilla.org/mozilla-central/rev/ad2286346bc0
https://hg.mozilla.org/mozilla-central/rev/dd3640717dd0
https://hg.mozilla.org/mozilla-central/rev/3c11339b3fd3
https://hg.mozilla.org/mozilla-central/rev/024a926ab622
https://hg.mozilla.org/mozilla-central/rev/6d077ab9ce5f
https://hg.mozilla.org/mozilla-central/rev/8e586e5b7c29

Comment 119

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Attachment: aurora53.patch

Please uplift to aurora, a=test-only.

Comment 120

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a43ef146176a
https://hg.mozilla.org/releases/mozilla-aurora/rev/d9e64da4d041
https://hg.mozilla.org/releases/mozilla-aurora/rev/5b1d46b36da7
https://hg.mozilla.org/releases/mozilla-aurora/rev/0f3caf166881
https://hg.mozilla.org/releases/mozilla-aurora/rev/448d84ab7eda
https://hg.mozilla.org/releases/mozilla-aurora/rev/48a5f47f26c5
https://hg.mozilla.org/releases/mozilla-aurora/rev/d3deaae90a74
https://hg.mozilla.org/releases/mozilla-aurora/rev/94bbc18b7fc1

Comment 121

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Attachment: keyactionstests_esr52.patch

Please uplift to esr52, test-only. Uplift Bug 1329703 first. Thanks!

Comment 122

[Carsten Book [:Tomcat]]

seems this need a rebase for esr52 since i hit problems like merging testing/web-platform/harness/wptrunner/executors/pytestrunner/runner.py
warning: conflicts while merging testing/web-platform/harness/wptrunner/executors/pytestrunner/fixtures.py! (edit, then use 'hg resolve --mark')
warning: conflicts while merging testing/web-platform/harness/wptrunner/executors/pytestrunner/runner.py! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 123

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

The supplied patch applies without error on my end (see below), so I'm not sure how to proceed further. Next steps?


IRC excerpt:
10:02 <maja_zf> Tomcat|sheriffduty: hi, can you confirm that you mean it's the supplied patch file that needs a rebase? https://bugzilla.mozilla.org/show_bug.cgi?id=1328726#c121
10:02 <•Tomcat|sheriffduty> maja_zf: hi, yeah noticed i did the wrong one
10:02 <•Tomcat|sheriffduty> maja_zf: but  the supplied patch failed now too 
10:02 <•Tomcat|sheriffduty> renamed 1328726 -> keyactionstests_esr52.patch
10:02 <•Tomcat|sheriffduty> applying keyactionstests_esr52.patch
10:02 <•Tomcat|sheriffduty> patch failed, unable to continue (try -v)
10:02 <•Tomcat|sheriffduty> patch failed, rejects left in working directory
10:02 <•Tomcat|sheriffduty> errors during apply, please fix and qrefresh keyactionstests_esr52.patch
10:02 <maja_zf> Tomcat|sheriffduty: okay, thanks, i'll look into it
10:02 <•Tomcat|sheriffduty> maja_zf: thanks
10:11 <maja_zf> Tomcat|sheriffduty: hmm, weird, it works fine for me: hg pull esr52; hg up esr52; hg import keyactionstests_esr52.patch -- any idea what can be done to fix for you?

Comment 124

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Applied cleanly for me...

remote:   https://hg.mozilla.org/releases/mozilla-esr52/rev/52c9feec937e545e7e901957716096c895fb81ab
remote:   https://hg.mozilla.org/releases/mozilla-esr52/rev/3046db97519e131777f073d5e3680e70f5d9cc42
remote:   https://hg.mozilla.org/releases/mozilla-esr52/rev/953312848de9cbc76cd4b12d846f4a9ff41c1b91
remote:   https://hg.mozilla.org/releases/mozilla-esr52/rev/847ef3599faa75cec4a3387f571ccf21da865791
remote:   https://hg.mozilla.org/releases/mozilla-esr52/rev/15a1e8bc0dce808b80aeb1b7fb07db7e9c5d77ba
remote:   https://hg.mozilla.org/releases/mozilla-esr52/rev/f98c4db03e53a7269053177644ec208510e71cde
remote:   https://hg.mozilla.org/releases/mozilla-esr52/rev/17913deae97f5f5f63748fbdb3625630902449bc
remote:   https://hg.mozilla.org/releases/mozilla-esr52/rev/a1b92421760f87cc141ea33aa51e7206cf1d17df

Comment 125

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Please uplift this to release as well, after uplifting Bug 1329703. On my end, the following attachment applies cleanly to release and tests pass: https://bugzilla.mozilla.org/attachment.cgi?id=8845158

Comment 126

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/15d20d9dc9a1
https://hg.mozilla.org/releases/mozilla-release/rev/69f53b4b67c6
https://hg.mozilla.org/releases/mozilla-release/rev/b5454004fa2b
https://hg.mozilla.org/releases/mozilla-release/rev/19a078a46458
https://hg.mozilla.org/releases/mozilla-release/rev/7ac5c4f5edbe
https://hg.mozilla.org/releases/mozilla-release/rev/ce92236a559d
https://hg.mozilla.org/releases/mozilla-release/rev/afa028e9f4f0
https://hg.mozilla.org/releases/mozilla-release/rev/0c6739cc4a68

Comment 127

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment 104 resolved.