[py] simplify error check in check_response for w3c#12255
[py] simplify error check in check_response for w3c#12255KazuCocoa wants to merge 19 commits intoSeleniumHQ:trunkfrom
Conversation
| if not status or status == ErrorCode.SUCCESS: | ||
|
|
||
| payload_dict = response | ||
| if type(response) != dict: |
| from selenium.common.exceptions import ElementNotInteractableException | ||
| from selenium.common.exceptions import ElementNotSelectableException | ||
| from selenium.common.exceptions import ElementNotVisibleException | ||
| from selenium.common.exceptions import ImeActivationFailedException |
| from selenium.common.exceptions import WebDriverException | ||
|
|
||
|
|
||
| class ExceptionMapping: |
There was a problem hiding this comment.
By removing this logic we make the client 100% incompatible with the legacy JWP protocol.
In scope of Appium I can guarantee that the server only supports W3C one and we do not care about JWP support anymore. I cannot guarantee this is the case in Selenium world though. Selenium java client, for example, still supports both JWP and W3C protocols, for example.
Leaving this decision up to client maintainers.
There was a problem hiding this comment.
Yes, need feedback at the point. I saw W3C flag removals in the past so I thought this client was removing old protocol behaviors but could be wrong
There was a problem hiding this comment.
Selenium does not support JWP, but there are still various pieces of JWP-only code left that haven't been removed. If it doesn't apply to w3c we want it gone. :)
| LOGGER.debug(f"Remote response: data={data} | headers={response.headers}") | ||
| try: | ||
| if 300 <= statuscode < 304: | ||
| return self._request("GET", response.headers.get("location", None)) |
There was a problem hiding this comment.
this code looks suspicious to me as it might potentially fall into an endless recursion. Usually clients (including urllib3, which is this code base on) handle such stuff automatically with some default number of allowed redirect retries to avoid the above recursion issue.
There was a problem hiding this comment.
I was actually testing this out earlier this week for Sauce-related reasons... The default number of allowed redirects is 3, and that is actually what happens. You can set remote URL to http://demo.cyotek.com/features/redirectlooptest.php and see that you only get 3. I'm not sure what this loop does, I just know it didn't result in infinite retries.
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## trunk #12255 +/- ##
==========================================
+ Coverage 57.28% 57.54% +0.26%
==========================================
Files 86 86
Lines 5487 5432 -55
Branches 226 231 +5
==========================================
- Hits 3143 3126 -17
+ Misses 2118 2075 -43
- Partials 226 231 +5
☔ View full report in Codecov by Sentry. |
|
Somehow you broke the cookie_test and the linter is also being particular. |
|
The cookies test returned non-w3c error response (according to the error result). I have added the pattern as a test case https://github.com/SeleniumHQ/selenium/pull/12255/files#diff-16a7f3559108027005862f77d542ca48d5dc188f7325ca4df1b73984f7de86c5R246-R256 for now to handle the case as well. |
| """Error codes defined in the WebDriver wire protocol.""" | ||
|
|
||
| # Keep in sync with org.openqa.selenium.remote.ErrorCodes and errorcodes.h | ||
| SUCCESS = 0 |
There was a problem hiding this comment.
We don't need to use the Error codes at all anymore
There was a problem hiding this comment.
Removed the ErrorCode class from errorhandler.py. in py/selenium/webdriver/remote/remote_connection.py, i have left 0 and 15 instead. The status probably safe to remove, but I'm not so confident to include the change in this PR since the value also has value (vanilla webdriver response) key. That may need to tweak properly at the same time, I think.
|
Updated. It would be appreciated to run the CI, thanks |
|
fixed the lint issue after applying the trunk |
|
Hm.., tox seems good in Codespace: |
Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it
In Appium dev, I observed Python client did not handle exceptions properly. https://github.com/appium/python-client/pull/868/files#r1199231346 For example when a response had
unknown methodvalue in the error key, it raised a generalWebDriverException. It looks like python client had a case that did not handle w3c response properly.After a few investigation, probably the main reason was this python binding gives extra
valueforcheck_response. Maybe we can simplify it.Description
in this pr, I have parsed error response by checking the
errorkey as same as Appium Python client does after the above error case. This is should be the same as what currently Ruby binding also does.So,
valuefrom the responseerrorfrom the 1errorvalue in 2With this PR, python client will not refer to
statusin the response. Ruby binding also does this already, and selenium v4 no longer support old protocol, I think this behavior should be ok for you.I also noticed the input of
check_responsegot nestedvaluesuch asfor probably backward compatibility. So, the
responsevalue has extravaluefor some reason. I guess for backward compatibility in the past.In this PR, I have changed the behavior to pass only one
valuekey to thecheck_responseso that thecheck_responsecan expect only W3C error format as the argument. (test_handle_errors_bettertest code addresses this point.)I also removed unused exception for w3c such as IME stuff.
Motivation and Context
I'd like to cleanup error handling for W3C format to help Appium Python client's implementation to reduce duplication code for both.
Types of changes
Checklist