-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Store the WebDriver error "data" property #48866
Store the WebDriver error "data" property #48866
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm totally up for this but please note that there is as well w3c/webdriver#1615 for discussion. We should get to a consensus first if we want that property or not.
I don't think anyone is suggesting removing it — that issue seems to be about whether "execute script" (and presumably "execute async script") set the data property? It already exists in general for errors. |
tools/webdriver/webdriver/error.py
Outdated
@@ -213,6 +221,8 @@ def from_response(response): | |||
code = value["error"] | |||
message = value["message"] | |||
stack = value["stacktrace"] or None | |||
# data is optional, and could even be an empty dict | |||
data = value.get("data") or None | |||
|
|||
cls = get(code) | |||
return cls(response.status, code, message, stacktrace=stack) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without that we won't set the data property:
return cls(response.status, code, message, stacktrace=stack) | |
return cls(response.status, code, message, stacktrace=stack, data=data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
@gsnedders do you have an update for this PR? |
ping @gsnedders |
8fdc126
to
64a34c8
Compare
I've updated this now! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but there is still a broken test. I've retriggered, so lets see if it is passing now.
It's unclear to me why this wasn't here before; it's been in the spec longer than we've stored anything on
WebDriverException
(specifically w3c/webdriver@33b1404 predates 726d661).