w3c / webdriver

Remote control interface that enables introspection and control of user agents.
https://w3c.github.io/webdriver/
Other
683 stars 195 forks source link

Execute script tries to set a data property on errors #1615

Open jgraham opened 3 years ago

jgraham commented 3 years ago

e.g. https://w3c.github.io/webdriver/#execute-script step 8 says:

Upon rejection of promise with reason r, let result be a JSON clone of r, and return error with error code javascript error and data result.

But there's no data property on error objects, so this doesn't make sense. The way the spec currently works it's an implementation detail whether the UA fills in useful information about the error or not.

shs96c commented 2 years ago

There should be a data property on the error object: we specced it out that way to support people who want to provide additional context when things go wrong. I'm not sure when this removed, but we should make sure it's present. There are implementations (such as Selenium Grid) that use this field.

whimboo commented 2 years ago

Note that issue #1661 prevents it to be shown on the GitHub page, but it's visible and still updated on https://www.w3.org/TR/webdriver/#errors.

whimboo commented 1 year ago

Maybe it's time to poke here again. The data field is optional: https://w3c.github.io/webdriver/#errors

@jgraham should we slightly rephrase the content to say that the remote end could add some additional data? Otherwise we might close this issue?

whimboo commented 2 weeks ago

CC @gsnedders @OrKoN

Is Safari and Chrome using this property when returning errors? If yes it might be established by various clients so that we should keep using it. If we agree on that we should check if something is still left to do for this issue on the current content of the classic spec or not.

gsnedders commented 2 weeks ago

First off, just looking at the spec:

But there's no data property on error objects, so this doesn't make sense. The way the spec currently works it's an implementation detail whether the UA fills in useful information about the error or not.

There is a data property, as @whimboo linked above; https://w3c.github.io/webdriver/#errors says:

Optionally "data", which is a JSON Object with additional error data helpful in diagnosing the error.

This doesn't make it implementation-defined (compare with, above it, '"message", containing an implementation-defined string with a human readable description of the kind of error that occurred'), but it does make it optional to include it at all.

The "error data" object itself is defined at https://w3c.github.io/webdriver/#dfn-error-data:

An error data dictionary is a mapping of string keys to JSON serializable values that can optionally be included with error objects.

However, https://w3c.github.io/webdriver/#dfn-send-an-error:

If the error data dictionary contains any entries, set the "data" field on body to a new JSON Object populated with the dictionary.

This should probably be explicit that it should be "optionally set"…?

That said, https://w3c.github.io/webdriver/#algorithms merely says:

Where algorithms that return values are fallible, they are written in terms of returning either success or error. A success value has an associated data field which encapsulates the value returned, whereas an error value has an associated error code.

Which doesn't explicitly pass the error data along.


Moving on, we say to return it in a few places:

https://w3c.github.io/webdriver/#dfn-annotated-unexpected-alert-open-error says:

An annotated unexpected alert open error is an error with error code unexpected alert open and an optional error data dictionary with the following entries:

"text" The current user prompt's message.

So anywhere which calls handle any user prompts can return an "annotated unexpected alert open error" with the data property optionally set. This probably doesn't need to make it "optional", because we can rely on "send an error" to make it always optional?

Moving back to execute script and execute async script, they both have:

return error with error code javascript error and data result

This seems wrong, this probably should be more like the previous case, something like "and an error data dictionary with the following entry: "data": 'result'"?

gsnedders commented 2 weeks ago

Is Safari and Chrome using this property when returning errors?

SafariDriver at least is returning it for annotated unexpected alert open errors, but not for javascript errors.