Closed Bug 1411281 Opened 6 years ago Closed 6 years ago

Unmarshal all responses to wdclient

Categories

(Remote Protocol :: Marionette, enhancement)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: ato, Assigned: ato)

References

Details

Attachments

(6 files)

Bug 1411281 - Associate web element identifier with webdriver.Element
59 bytes, text/x-review-board-request
jgraham
: review+
Details
Bug 1411281 - Add equality test for webdriver.Element
59 bytes, text/x-review-board-request
jgraham
: review+
Details
Bug 1411281 - Swap webdriver.Element ctor arguments
59 bytes, text/x-review-board-request
jgraham
: review+
Details
Bug 1411281 - Unmarshal all responses in WPT WebDriver client
59 bytes, text/x-review-board-request
jgraham
: review+
Details
Bug 1411281 - Make assert_same_element accept webdriver.Element
59 bytes, text/x-review-board-request
jgraham
: review+
Details
Bug 1411281 - Add equality test for webdriver.Session
59 bytes, text/x-review-board-request
jgraham
: review+
Details 
The WPT WebDriver client currently only unmarshals responses for
some commands (notable execute_script and execute_async_script).
For the client API we will want to unmarshal all response bodies.
Assignee: nobody → ato
Status: NEW → ASSIGNED
Depends on: 1411286
Blocks: 1411286
No longer depends on: 1411286
Attachment #8921874 - Flags: review?(james)
Attachment #8921875 - Flags: review?(james)
Attachment #8921876 - Flags: review?(james)
Attachment #8921877 - Flags: review?(james)
Attachment #8921955 - Flags: review?(james)
Comment on attachment 8921877 [details]
Bug 1411281 - Unmarshal all responses in WPT WebDriver client

https://reviewboard.mozilla.org/r/192918/#review198284

::: testing/web-platform/tests/tools/webdriver/webdriver/client.py:436
(Diff revision 2)
> +        if response.status != 200:
> +            cls = error.get(value.get("error"))
> +            raise cls(value.get("message"))

This should not be here, I think this snuck in during rebase.  It is covered by error.from_response above.
Comment on attachment 8921874 [details]
Bug 1411281 - Associate web element identifier with webdriver.Element

https://reviewboard.mozilla.org/r/192912/#review198314
Attachment #8921874 - Flags: review?(james) → review+
Comment on attachment 8921875 [details]
Bug 1411281 - Add equality test for webdriver.Element

https://reviewboard.mozilla.org/r/192914/#review198316

::: testing/web-platform/tests/tools/webdriver/webdriver/client.py:620
(Diff revision 2)
>          self.id = id
>          assert id not in self.session._element_cache
>          self.session._element_cache[self.id] = self
>  
> +    def __eq__(self, other):
> +        return isinstance(other, Element) and self.id == other.id

Theoretically you also need to check the sessions are the same.
Attachment #8921875 - Flags: review?(james) → review+
Comment on attachment 8921876 [details]
Bug 1411281 - Swap webdriver.Element ctor arguments

https://reviewboard.mozilla.org/r/192916/#review198526

::: testing/web-platform/tests/tools/webdriver/webdriver/client.py:613
(Diff revision 2)
>      A web element is an abstraction used to identify an element when
>      it is transported via the protocol, between remote- and local ends.
>      """
>      identifier = "element-6066-11e4-a52e-4f735466cecf"
>  
> -    def __init__(self, session, id):
> +    def __init__(self, id, session):

I guess I don't care, but this doesn't seem obviously like an improvement.
Attachment #8921876 - Flags: review?(james) → review+
Comment on attachment 8921877 [details]
Bug 1411281 - Unmarshal all responses in WPT WebDriver client

https://reviewboard.mozilla.org/r/192918/#review198532

::: testing/web-platform/tests/tools/webdriver/webdriver/protocol.py:10
(Diff revision 2)
> +
> +"""WebDriver wire protocol codecs."""
> +
> +
> +class Encoder(json.JSONEncoder):
> +

No blank line here.

::: testing/web-platform/tests/tools/webdriver/webdriver/protocol.py:24
(Diff revision 2)
> +            return {webdriver.Element.identifier: obj.id}
> +        return super(ProtocolEncoder, self).default(obj)
> +
> +
> +class Decoder(json.JSONDecoder):
> +

No blank line here.

::: testing/web-platform/tests/tools/webdriver/webdriver/protocol.py:33
(Diff revision 2)
> +            object_hook=self.object_hook, *args, **kwargs)
> +
> +    def object_hook(self, payload):
> +        if isinstance(payload, (list, tuple)):
> +            return [self.object_hook(x) for x in payload]
> +        elif isinstance(payload, dict) and webdriver.Element.identifier in payload:

This seems to end up putting a lot of type-specific information directly into this class, which doesn't seem ideal.

::: testing/web-platform/tests/tools/webdriver/webdriver/transport.py:89
(Diff revision 2)
>  
>      def url(self, suffix):
>          return urlparse.urljoin(self.url_prefix, suffix)
>  
> -    def send(self, method, uri, body=None, headers=None):
> +    def send(self, method, uri, body=None, headers=None,
> +        encoder=json.JSONEncoder, decoder=json.JSONDecoder,

This indentation is wrong.
Attachment #8921877 - Flags: review?(james) → review+
Comment on attachment 8921955 [details]
Bug 1411281 - Make assert_same_element accept webdriver.Element

https://reviewboard.mozilla.org/r/192964/#review198534
Attachment #8921955 - Flags: review?(james) → review+
Comment on attachment 8921875 [details]
Bug 1411281 - Add equality test for webdriver.Element

https://reviewboard.mozilla.org/r/192914/#review198316

> Theoretically you also need to check the sessions are the same.

Good point.  I’ll squeeze in an additional patch that implements
__eq__ for webdriver.Session and add that here.
Comment on attachment 8921876 [details]
Bug 1411281 - Swap webdriver.Element ctor arguments

https://reviewboard.mozilla.org/r/192916/#review198526

> I guess I don't care, but this doesn't seem obviously like an improvement.

I initially wanted to change webdriver.Element so that it could be
constructed without an associated session to take advantage of its
equality test in assert_same_element.  I later decided against this
due to the complexity involved for apparently little gain.

However, I still think it makes more sense to construct Element
with the web element UUID first since this is its defining
characteristic.  It only needs a session because that happens to
store the connection.
Comment on attachment 8921877 [details]
Bug 1411281 - Unmarshal all responses in WPT WebDriver client

https://reviewboard.mozilla.org/r/192918/#review198532

> This seems to end up putting a lot of type-specific information directly into this class, which doesn't seem ideal.

It necessarily needs to have some degree of knowledge about what
it’s looking for.  We know that in WebDriver HTTP bodies the web
element identifier, the web window identifier, and the web frame
identifier always represents some kind of complex object.

The alternative to this approach is to encode this into each command
that we reasonably expects to send or receive these types, which is
an approach this patch is trying to move us away from.

I guess the lines that follow (lines 34-37) could be moved to a
class method on webdriver.Element or something.  Would that address
your concern?
Attachment #8922392 - Flags: review?(james)
Comment on attachment 8922392 [details]
Bug 1411281 - Add equality test for webdriver.Session

https://reviewboard.mozilla.org/r/193432/#review199022

::: testing/web-platform/tests/tools/webdriver/webdriver/client.py:377
(Diff revision 1)
>          self.find = Find(self)
>          self.alert = UserPrompt(self)
>          self.actions = Actions(self)
>  
> +    def __eq__(self, other):
> +        return self.session_id is not None \

Don't use lint continuation characters. Use parens around the condition

```
return (self.session_id is not None and
        […])
```
Attachment #8922392 - Flags: review?(james) → review+
Comment on attachment 8921877 [details]
Bug 1411281 - Unmarshal all responses in WPT WebDriver client

https://reviewboard.mozilla.org/r/192918/#review199028
I still need a decision on the remaining open issue from
https://bugzil.la/1411281#c18.
Flags: needinfo?(james)
Comment on attachment 8922392 [details]
Bug 1411281 - Add equality test for webdriver.Session

https://reviewboard.mozilla.org/r/193432/#review199022

> Don't use lint continuation characters. Use parens around the condition
> 
> ```
> return (self.session_id is not None and
>         […])
> ```

I love it when you can only use a subset of the language.  It would
be worth standing up a linter job to catch these things so we
don’t have to waste time on it in reviews.
Comment on attachment 8921877 [details]
Bug 1411281 - Unmarshal all responses in WPT WebDriver client

https://reviewboard.mozilla.org/r/192918/#review198532

> It necessarily needs to have some degree of knowledge about what
> it’s looking for.  We know that in WebDriver HTTP bodies the web
> element identifier, the web window identifier, and the web frame
> identifier always represents some kind of complex object.
> 
> The alternative to this approach is to encode this into each command
> that we reasonably expects to send or receive these types, which is
> an approach this patch is trying to move us away from.
> 
> I guess the lines that follow (lines 34-37) could be moved to a
> class method on webdriver.Element or something.  Would that address
> your concern?

I have now made this change, and I think it indeed looks better.
Please pitch in soon, or I will go ahead and land this change.
Please go ahead and land.
Flags: needinfo?(james)
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f58e9711dfc5
Associate web element identifier with webdriver.Element r=jgraham
https://hg.mozilla.org/integration/autoland/rev/4fc2942a3c69
Add equality test for webdriver.Session r=jgraham
https://hg.mozilla.org/integration/autoland/rev/2e0cca1c1e87
Add equality test for webdriver.Element r=jgraham
https://hg.mozilla.org/integration/autoland/rev/151ca34c9c62
Swap webdriver.Element ctor arguments r=jgraham
https://hg.mozilla.org/integration/autoland/rev/6da19712fa96
Unmarshal all responses in WPT WebDriver client r=jgraham
https://hg.mozilla.org/integration/autoland/rev/475350247c52
Make assert_same_element accept webdriver.Element r=jgraham
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.