Closed Bug 1469752 Opened 6 years ago Closed 1 year ago

WebDriverError misses optional "data" property

Categories

(Testing :: geckodriver, defect)

Product:
Testing
Component:
geckodriver
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: whimboo, Unassigned)

References

(Blocks 1 open bug)

Details

As defined by the spec the WebDriverError contains an optional `data` property which can hold extra data to help with error diagnosis: https://w3c.github.io/webdriver/#handling-errors > Optionally "data", which is a JSON Object with additional error data helpful in diagnosing the error.
Hm, I have troubles in geckodriver and webdriver to store the data key. I tried the following: > pub data: Option<BTreeMap<String, Json>>, But the compiler complains that the required trait is not implemented: > 1262 | #[derive(RustcEncodable, RustcDecodable)] > | ^^^^^^^^^^^^^^ the trait `rustc_serialize::Decodable` is not implemented for `rustc_serialize::json::Json` I assume that I cannot use `Json` as value in the BTreeMap, but not sure what else I should choose. Andreas, can you please help? Thanks.
Flags: needinfo?(ato)
Here the WebDriver spec definition of `error data`: > An error data dictionary is a mapping of string keys to JSON serializable values that can optionally be included with error objects.
Also I wonder if we should use a builder API for WebDriverError so that it is easier to set the optional data and stacktrace properties.
I’m afraid I don’t know serde well enough to know what the appropriate type to use here is. I would assume rustc_serialize and serde are mutually incompatible in the sense that serde won’t know how to serialise a serialisation type from rustc_serialize. Perhaps you want to use a serde_json::value::Value here? https://docs.serde.rs/serde_json/value/enum.Value.html With regards to the API, a builder immediately sounds overkill but we should examine what the use cases are. We currently don’t make use of the "data" field at all so it is something we could approach later.
Flags: needinfo?(ato)
(In reply to Andreas Tolfsen 「:ato」 from comment #4) > I’m afraid I don’t know serde well enough to know what the appropriate > type to use here is. I would assume rustc_serialize and serde are > mutually incompatible in the sense that serde won’t know how to > serialise a serialisation type from rustc_serialize. This bug has nothing to do with Serde, which is a different effort and might still take a while. So my request from above is all about rustc_serialize/deserialize. > With regards to the API, a builder immediately sounds overkill but > we should examine what the use cases are. We currently don’t make > use of the "data" field at all so it is something we could approach > later. We have two implementations (new, and new_with_stack) in how to create a WebDriverError: https://dxr.mozilla.org/mozilla-central/source/testing/webdriver/src/error.rs#265 Given that `data` is also optional we would probably need two more methods.
Flags: needinfo?(ato)
(In reply to Henrik Skupin (:whimboo) from comment #5) > (In reply to Andreas Tolfsen 「:ato」 from comment #4) > > I’m afraid I don’t know serde well enough to know what the appropriate > > type to use here is. I would assume rustc_serialize and serde are > > mutually incompatible in the sense that serde won’t know how to > > serialise a serialisation type from rustc_serialize. > > This bug has nothing to do with Serde, which is a different effort and might > still take a while. > > So my request from above is all about rustc_serialize/deserialize. Sorry, then I don’t know the answer to your question.
Flags: needinfo?(ato)
Then I think it makes more sense to wait for the Serde work.
Assignee: hskupin → nobody
No longer blocks: 1396821
Status: ASSIGNED → NEW
Depends on: 1396821
Priority: P1 → P3
No longer blocks: 1264259
Depends on: 1264259
Blocks: webdriver
Priority: P3 → P2

The serde work has been done for a while, but so far there is no evidence that we might have to use this property. So far the message property was working all fine for us. It's not P2 or P3 worthy. Happy to take a patch if someone is interested to have it but otherwise we might not get to it.

Priority: P2 → P5
Severity: normal → S3
Product: Testing → Remote Protocol

Over on bug 1834971 we will need additional data to describe eg errors that are related to nodes. As such the data property could include a nodeId, navigable etc. While I will keep bug 1834971 for the remote agent I would move this bug to the geckodriver component and we can handle it whenever clients would need this information as well. Lets discuss in one of the next triage meetings.

Severity: S3 → --
Component: Marionette → geckodriver
Depends on: 1834971
Priority: P5 → --
Product: Remote Protocol → Testing

This is a feature that is mostly only used by intermediate nodes only. As discussed it's not really something that we want to have nowadays.

Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.