1100155 - Mention expected response key when raising MarionetteException on unexpected response

Status: RESOLVED INVALID

(Remote Protocol :: Marionette, defect)

Description

[Andreas Tolfsen ❲:ato❳]

The Marionette Python client raises an exception when it receives a response with an unexpected key.  The expected key is given as the argument `response_key` and it defaults to "ok".

If response_key is missing from the returned dictionary it will raise, but its exception message is unhelpful:

MarionetteException: MarionetteException: {u'ok': True, u'from': u'0'}

Instead we should change it say “expected key "value", but got: {u'ok': True, u'from': u'0'}” instead.

The relevant piece of code is testing/marionette/client/marionette/marionette.py:636.

Comment 1

[Giovanny Gongora [:gioyik]]

Hi Andreas,

Seems this was fixed, isn't it? I am seeing and the file has changed from "testing/marionette/client/marionette/marionette.py" to "testing/marionette/driver/marionette_driver/marionette.py".

There's a _handle_error in line 696 [0] that is called in 668 [1], specific in line 670 [2]. Am I correct, or is needed a fix yet?

[0] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver/marionette_driver/marionette.py#696
[1] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver/marionette_driver/marionette.py#668
[2] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver/marionette_driver/marionette.py#670

Comment 2

[Andreas Tolfsen ❲:ato❳]

This seems to be partially fixed.  It would be nice if we passed along which key we were expecting to _handle_error so that we could raise an exception on that too.  Right now it's just testing for the "error" key to be present to be able to display the error message.

Comment 3

[Giovanny Gongora [:gioyik]]

Attachment: 1100155.patch

Patch v1: feedback needed

Comment 4

[Giovanny Gongora [:gioyik]]

Andreas, I attached a patch for this that needs feedback because I am not sure if the solution was a new rule or modify this one [0]. Also I am not sure to use "_handle_error" or "raise errors.MarionetteException("expected key ok")".

Let me know your comments to get this a bug fixed. :)

[0] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver/marionette_driver/marionette.py#668

Comment 5

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8570071 [details] [diff] [review]
1100155.patch

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

::: testing/marionette/driver/marionette_driver/marionette.py
@@ +670,5 @@
>          self._handle_error(response)
>  
> +        if response_key in response is not "ok"
> +            _handle_error(response)
> +

This won't work because if response_key is missing from response, it will be caught by line 670 and never reach this.

I'd probably pass along response_key as a keyword argument to _handle_error like this instead:

    _handle_error(response, expected_key=response_key)

Then you'd check if the key "error" is missing in _handle_error, and if it use raise an exception about missing the expected_key.

Comment 6

[Andreas Tolfsen ❲:ato❳]

This is superseded by bug 1211503.