Sitelet https://github.com/appium/python-client/pull/869
Skip to content

fix: W3C errors to exception classes mapping - #869

Merged
KazuCocoa merged 3 commits into
appium:masterfrom
mykola-mokhnach:exc_map
May 20, 2023
Merged

KazuCocoa merged 3 commits into
appium:masterfrom
mykola-mokhnach:exc_map

Conversation

@mykola-mokhnach

Copy link
Copy Markdown
Contributor

It turns out the current selenium python client does not match error codes properly, at least for W3C responses.

@mykola-mokhnach
mykola-mokhnach requested a review from KazuCocoa May 19, 2023 19:08
@KazuCocoa
KazuCocoa merged commit 5c20a35 into appium:master May 20, 2023
@mykola-mokhnach
mykola-mokhnach deleted the exc_map branch May 26, 2023 05:13
@KazuCocoa

Copy link
Copy Markdown
Member

I have created a pr for selenium as SeleniumHQ/selenium#12255
Maybe once it will be merged, we can simplify this as well.

@rhysd

rhysd commented Jul 12, 2023

Copy link
Copy Markdown

@mykola-mokhnach @KazuCocoa

Hi, I'd like to ask a question about changes in this PR.

By changes in this PR, check_response totally ignores W3C's error response. Is the behavior intended?

class MobileErrorHandler(errorhandler.ErrorHandler):
    def check_response(self, response: Dict[str, Any]) -> None:
        """
        https://www.w3.org/TR/webdriver/#errors
        """
        payload = response.get('value', '')
        try:
            payload_dict = json.loads(payload)
        except (json.JSONDecodeError, TypeError):
            return

        ...

When the following error response (taken from the example 3) is passed to this method,

{
	"value": {
		"error": "invalid session id",
		"message": "No active session with ID 1234",
		"stacktrace": ""
	}
}

json.loads(payload) raises an error and the except: catches the error. And it returns from the method without raising any exception.

@KazuCocoa

KazuCocoa commented Jul 12, 2023 •

Copy link
Copy Markdown
Member

Something issue occurred?

Probably current Selenium Python client gives something like {'value': "{'value': 'xxx', 'error': 'yyyy',...}", 'status': 200} format message to the check_response. At least when I created a pr https://github.com/SeleniumHQ/selenium/pull/12255/files#diff-c87c150ed2a08f5293db58a06da8bf7f14c3a5582586c6270c1be661dfdd0985R345-R350 , it still provided nested value. The PR will gives the {'value': 'xxx', 'error': 'yyyy',...} only for check_response, but it is not in the trunk (at least).

So I think the selenium python client still provides the nested value format for check_response. Then, yes. The initial response.get('value', '') is to get W3C error response from the nested {'value': {'value': 'xxx', 'error': 'yyyy',...}, 'status': 200}.

@KazuCocoa

Copy link
Copy Markdown
Member

Perhaps I haven't checked with latest selenium client dependency, so potentially we should update the handler if it has an issue

@rhysd

rhysd commented Jul 12, 2023

Copy link
Copy Markdown

After some investigation, it turned out that the issue in my side was caused by our special usage of appium-python-client. I'm sorry for bothering you.

Though I can't describe details in public, we are still supporting very very old WebKit webview in our application. It requires very old version of Chromedriver to automate. Appium server proxies request between appium-python-client and the Chromedriver. Since the Chromedriver is very old, its response is a bit different from what the latest appium-python-client assumes. It caused the issue and it is totally an issue of our side.

@KazuCocoa

Copy link
Copy Markdown
Member

Got it, thanks for the info.
Selenium project started keeping w3c webdriver protcol only from their bindings, so our clients also basically keep w3c only (and for appium 2.0). But I know of chromedriver could accept w3c: false. Newer chorme may accept w3c protocol in the case as well, but may not for much older chromedriver.

Feel free to create a ticket if our client needs to have something for Appium-specific things to help it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants