-
-
Notifications
You must be signed in to change notification settings - Fork 8.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
[dotnet] Remove indirection through Response
on HTTP exception
#14892
base: trunk
Are you sure you want to change the base?
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨No code suggestions found for the PR. |
Status = WebDriverResult.UnhandledError, | ||
Value = e | ||
}; | ||
throw new WebDriverException("The " + driverCommandToExecute + " command failed to send.", e); |
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 don't mind.
Do you see potential miss-using? It is new exception type...
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.
Luckily this is the same behavior which already exists. Currently the method UnpackAndThrowOnError
throws the same exception. The only difference here is the more specific message and inner exception passed instead to e.ToString()
appending to the message.
For reasons I expanded in the PR description, this doesn’t change any execution, so there’s no potential issue here.
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.
All right. Let's think how to improve the error. This error will be user faced in any cace of network issue - it is so popular.
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.
Unhandled error while executing <command>.
?
User description
Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it
Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.
Description
This removes an unnecessary use of
Response
, which would throw an exception in the subsequentUnpackAndThrowOnError
anyway.Motivation and Context
This code stores an
Exception
intoResponse.Value
. This is the only usage of that type to store anything other than a JSON-deserialized response, which the custom converter handles to only containbool
,long
,double
,string
,List<object>
, orDictionary<string, object>
.This change makes it so we can assume
Response.Value
is always one of those types.While trying to test this, I noticed this code path is unreachable by normal means anyway. The
HttpRequestException
is caught earlier on and turned into a normalWebDriverException
.selenium/dotnet/src/webdriver/Remote/HttpCommandExecutor.cs
Lines 201 to 205 in 4ea71ca
However, this code path may be hit if an advanced user supplies their own
ICommandExecutor
in theWebDriver
constructor. Therefore, this catch block is still meaningful.Types of changes
Checklist
PR Type
enhancement, bug_fix
Description
Response
object when handlingHttpRequestException
inWebDriver.cs
.WebDriverException
with a clear error message, improving code clarity and error handling.Response.Value
is not used to store exceptions, aligning with the expected data types.Changes walkthrough 📝
WebDriver.cs
Simplify exception handling for HTTP requests in WebDriver
dotnet/src/webdriver/WebDriver.cs
Response
object in the catch block forHttpRequestException
.WebDriverException
directly with a descriptive message.