1411026 - geckodriver discards Marionette stacktraces

Status: RESOLVED FIXED

(Testing :: geckodriver, enhancement)

Description

[Andreas Tolfsen ❲:ato❳]

Stacktraces from Marionette are discarded when the WebDriverError is
constructed in testing/geckodriver/src/marionette.rs:

> if let Some(error) = resp.error {
>     let status = self.error_from_string(&error.status);
> 
>     return Err(WebDriverError::new(status, error.message));
> }

Comment 1

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1411026 - Provide String type conversion for ErrorStatus.

This allows us to construct an ErrorStatus variant from a String.  This is useful if the remote end implementation uses the string codes for transport.

The new From<String> trait for ErrorStatus is a reverse lookup of ErrorStatus::error_code.

It currently handles two cases of Selenium errors, which are "element not visible" and "invalid element coordinates".  Both these have been deprecated in the WebDriver standard and we need to figure out a deprecation strategy for them.

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

Comment 2

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1411026 - Rename MarionetteError::status to ::code.

For errors originating from Marionette we talk about "error codes", not about "error statuses".  This patch fixes the JSON serialisation method Marionette::to_json which incorrectly serialised the error code as "status"; it should be "error".

This also removes the MarionetteError::new constructor as we have no need to allow explicit construction of MarionetteError's.

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

Comment 3

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1411026 - Propagate Marionette stacks to WebDriver error.

When a stacktrace is provided from Marionette, use the WebDriverError::new_with_stack constructor to have it propagated as the WebDriver error's stacktrace rather than use the Rust backtrace.

This patch also removes MarionetteSession::error_from_string in favour of the new From<String> trait added to ErrorStatus.

It also implements an Into<WebDriverError> trait for MarionetteError to ease the conversion.

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

Comment 4

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8921217 [details]
Bug 1411026 - Provide String type conversion for ErrorStatus.

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

Comment 5

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8921218 [details]
Bug 1411026 - Rename MarionetteError::status to ::code.

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

Comment 6

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8921219 [details]
Bug 1411026 - Propagate Marionette stacks to WebDriver error.

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

Comment 7

[Andreas Tolfsen ❲:ato❳]

https://bugzil.la/1411045 is also relevant to this change.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8921217 [details]
Bug 1411026 - Provide String type conversion for ErrorStatus.

https://reviewboard.mozilla.org/r/192212/#review197490

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8921218 [details]
Bug 1411026 - Rename MarionetteError::status to ::code.

https://reviewboard.mozilla.org/r/192214/#review197492

Good find!

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8921219 [details]
Bug 1411026 - Propagate Marionette stacks to WebDriver error.

https://reviewboard.mozilla.org/r/192216/#review197494

If you could also update the geckodriver changelog that would be great and will make the release work easier.

::: testing/geckodriver/src/marionette.rs:1270
(Diff revision 2)
>      }
>  }
>  
> +impl Into<WebDriverError> for MarionetteError {
> +    fn into(self) -> WebDriverError {
> +        let status = ErrorStatus::from(self.code);

s/status/code

Comment 11

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8921219 [details]
Bug 1411026 - Propagate Marionette stacks to WebDriver error.

https://reviewboard.mozilla.org/r/192216/#review197494

> s/status/code

Apparently ErrorStatus is still named “status” so I’ll keep
this around, but we should think about renaming that as well!

Not good to mix code/status/error.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/135d220e2178
Provide String type conversion for ErrorStatus. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/6a24cb5cb401
Rename MarionetteError::status to ::code. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/7d3f5015ac7e
Propagate Marionette stacks to WebDriver error. r=whimboo

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/135d220e2178
https://hg.mozilla.org/mozilla-central/rev/6a24cb5cb401
https://hg.mozilla.org/mozilla-central/rev/7d3f5015ac7e