Closed
Bug 1100155
Opened 10 years ago
Closed 9 years ago
Mention expected response key when raising MarionetteException on unexpected response
Categories
(Remote Protocol :: Marionette, defect)
Remote Protocol
Marionette
Tracking
(Not tracked)
RESOLVED
INVALID
People
(Reporter: ato, Unassigned, Mentored)
Details
(Keywords: pi-marionette-client, Whiteboard: [good first bug] [mentor=ato][language=py])
Attachments
(1 file)
1.23 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•10 years ago
|
Whiteboard: [good first bug] [mentor=ato]
Updated•10 years ago
|
Keywords: ateam-marionette-client
Whiteboard: [good first bug] [mentor=ato] → [good first bug] [mentor=ato][language=py]
Comment 1•9 years ago
|
||
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
Flags: needinfo?(ato)
Reporter | ||
Comment 2•9 years ago
|
||
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.
Flags: needinfo?(ato)
Comment 4•9 years ago
|
||
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
Updated•9 years ago
|
Assignee: nobody → gioyik
Reporter | ||
Comment 5•9 years ago
|
||
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.
Attachment #8570071 -
Flags: feedback?(ato)
Reporter | ||
Comment 6•9 years ago
|
||
This is superseded by bug 1211503.
Assignee: gioyik → nobody
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → INVALID
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•