Skip to content

Conversation

@mykola-mokhnach
Copy link
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
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
Copy link

rhysd commented Jul 12, 2023

@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
Copy link
Member

KazuCocoa commented Jul 12, 2023

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
Member

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

@rhysd
Copy link

rhysd commented Jul 12, 2023

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
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