From 444413675e9c14f4cec66287ab4f274c210d4db1 Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Wed, 15 Jun 2022 15:01:06 +0200 Subject: [PATCH 01/10] chore: Add better error handling for session creation responses --- appium/webdriver/webdriver.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/appium/webdriver/webdriver.py b/appium/webdriver/webdriver.py index 19da31b1..c229b440 100644 --- a/appium/webdriver/webdriver.py +++ b/appium/webdriver/webdriver.py @@ -17,7 +17,7 @@ from typing import Any, Callable, Dict, List, Optional, Tuple, Union from selenium import webdriver -from selenium.common.exceptions import InvalidArgumentException, WebDriverException +from selenium.common.exceptions import InvalidArgumentException, SessionNotCreatedException, WebDriverException from selenium.webdriver.common.by import By from selenium.webdriver.remote.command import Command as RemoteCommand from selenium.webdriver.remote.remote_connection import RemoteConnection @@ -317,10 +317,18 @@ def start_session(self, capabilities: Union[Dict, AppiumOptions], browser_profil w3c_caps = AppiumOptions.as_w3c(capabilities) if isinstance(capabilities, dict) else capabilities.to_w3c() response = self.execute(RemoteCommand.NEW_SESSION, w3c_caps) - if 'sessionId' not in response: - response = response['value'] + # https://w3c.github.io/webdriver/#new-session + if not isinstance(response, dict): + raise SessionNotCreatedException( + f'A valid W3C session creation response must be a dictionary. Got "{response}" instead' + ) + if not response.get('sessionId'): + raise SessionNotCreatedException( + f'A valid W3C session creation response must contain a non-empty "sessionId" entry. ' + f'Got "{response}" instead' + ) self.session_id = response['sessionId'] - self.caps = response.get('value') or response.get('capabilities') + self.caps = response.get('capabilities') or {} def find_element(self, by: str = AppiumBy.ID, value: Union[str, Dict] = None) -> MobileWebElement: """ From af7e87408f1763efbfcd22d8e8323e6a12b7f070 Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Wed, 15 Jun 2022 15:13:37 +0200 Subject: [PATCH 02/10] Update unit tests --- test/unit/helper/test_helper.py | 67 +++++++++++-------------- test/unit/webdriver/webdriver_test.py | 72 ++++++++++++--------------- 2 files changed, 63 insertions(+), 76 deletions(-) diff --git a/test/unit/helper/test_helper.py b/test/unit/helper/test_helper.py index 6c85eb2e..7037757e 100644 --- a/test/unit/helper/test_helper.py +++ b/test/unit/helper/test_helper.py @@ -50,29 +50,27 @@ def android_w3c_driver() -> 'WebDriver': response_body_json = json.dumps( { - 'value': { - 'sessionId': '1234567890', - 'capabilities': { - 'platform': 'LINUX', - 'desired': { - 'platformName': 'Android', - 'automationName': 'uiautomator2', - 'platformVersion': '7.1.1', - 'deviceName': 'Android Emulator', - 'app': '/test/apps/ApiDemos-debug.apk', - }, + 'sessionId': '1234567890', + 'capabilities': { + 'platform': 'LINUX', + 'desired': { 'platformName': 'Android', 'automationName': 'uiautomator2', 'platformVersion': '7.1.1', - 'deviceName': 'emulator-5554', + 'deviceName': 'Android Emulator', 'app': '/test/apps/ApiDemos-debug.apk', - 'deviceUDID': 'emulator-5554', - 'appPackage': 'io.appium.android.apis', - 'appWaitPackage': 'io.appium.android.apis', - 'appActivity': 'io.appium.android.apis.ApiDemos', - 'appWaitActivity': 'io.appium.android.apis.ApiDemos', }, - } + 'platformName': 'Android', + 'automationName': 'uiautomator2', + 'platformVersion': '7.1.1', + 'deviceName': 'emulator-5554', + 'app': '/test/apps/ApiDemos-debug.apk', + 'deviceUDID': 'emulator-5554', + 'appPackage': 'io.appium.android.apis', + 'appWaitPackage': 'io.appium.android.apis', + 'appActivity': 'io.appium.android.apis.ApiDemos', + 'appWaitActivity': 'io.appium.android.apis.ApiDemos', + }, } ) @@ -95,18 +93,15 @@ def ios_w3c_driver() -> 'WebDriver': Returns: `webdriver.webdriver.WebDriver`: An instance of WebDriver """ - response_body_json = json.dumps( { - 'value': { - 'sessionId': '1234567890', - 'capabilities': { - 'device': 'iphone', - 'browserName': 'UICatalog', - 'sdkVersion': '11.4', - 'CFBundleIdentifier': 'com.example.apple-samplecode.UICatalog', - }, - } + 'sessionId': '1234567890', + 'capabilities': { + 'device': 'iphone', + 'browserName': 'UICatalog', + 'sdkVersion': '11.4', + 'CFBundleIdentifier': 'com.example.apple-samplecode.UICatalog', + }, } ) @@ -132,15 +127,13 @@ def ios_w3c_driver_with_extensions(extensions) -> 'WebDriver': response_body_json = json.dumps( { - 'value': { - 'sessionId': '1234567890', - 'capabilities': { - 'device': 'iphone', - 'browserName': 'UICatalog', - 'sdkVersion': '11.4', - 'CFBundleIdentifier': 'com.example.apple-samplecode.UICatalog', - }, - } + 'sessionId': '1234567890', + 'capabilities': { + 'device': 'iphone', + 'browserName': 'UICatalog', + 'sdkVersion': '11.4', + 'CFBundleIdentifier': 'com.example.apple-samplecode.UICatalog', + }, } ) diff --git a/test/unit/webdriver/webdriver_test.py b/test/unit/webdriver/webdriver_test.py index 13fbfab6..5a57415a 100644 --- a/test/unit/webdriver/webdriver_test.py +++ b/test/unit/webdriver/webdriver_test.py @@ -37,7 +37,7 @@ def test_create_session(self): httpretty.register_uri( httpretty.POST, f'{SERVER_URL_BASE}/session', - body='{ "value": { "sessionId": "session-id", "capabilities": {"deviceName": "Android Emulator"}}}', + body='{ "sessionId": "session-id", "capabilities": {"deviceName": "Android Emulator"} }', ) desired_caps = { @@ -71,7 +71,7 @@ def test_create_session_change_session_id(self): httpretty.register_uri( httpretty.POST, f'{SERVER_URL_BASE}/session', - body='{ "value": { "sessionId": "session-id", "capabilities": {"deviceName": "Android Emulator"}}}', + body='{ "sessionId": "session-id", "capabilities": {"deviceName": "Android Emulator"} }', ) httpretty.register_uri( @@ -100,16 +100,14 @@ def test_create_session_register_uridirect(self): f'{SERVER_URL_BASE}/session', body=json.dumps( { - 'value': { - 'sessionId': 'session-id', - 'capabilities': { - 'deviceName': 'Android Emulator', - 'directConnectProtocol': 'http', - 'directConnectHost': 'localhost2', - 'directConnectPort': 4800, - 'directConnectPath': '/special/path/wd/hub', - }, - } + 'sessionId': 'session-id', + 'capabilities': { + 'deviceName': 'Android Emulator', + 'directConnectProtocol': 'http', + 'directConnectHost': 'localhost2', + 'directConnectPort': 4800, + 'directConnectPath': '/special/path/wd/hub', + }, } ), ) @@ -142,15 +140,13 @@ def test_create_session_register_uridirect_no_direct_connect_path(self): f'{SERVER_URL_BASE}/session', body=json.dumps( { - 'value': { - 'sessionId': 'session-id', - 'capabilities': { - 'deviceName': 'Android Emulator', - 'directConnectProtocol': 'http', - 'directConnectHost': 'localhost2', - 'directConnectPort': 4800, - }, - } + 'sessionId': 'session-id', + 'capabilities': { + 'deviceName': 'Android Emulator', + 'directConnectProtocol': 'http', + 'directConnectHost': 'localhost2', + 'directConnectPort': 4800, + }, } ), ) @@ -327,29 +323,27 @@ class TestSubModuleWebDriver(object): def android_w3c_driver(self, driver_class): response_body_json = json.dumps( { - 'value': { - 'sessionId': '1234567890', - 'capabilities': { - 'platform': 'LINUX', - 'desired': { - 'platformName': 'Android', - 'automationName': 'uiautomator2', - 'platformVersion': '7.1.1', - 'deviceName': 'Android Emulator', - 'app': '/test/apps/ApiDemos-debug.apk', - }, + 'sessionId': '1234567890', + 'capabilities': { + 'platform': 'LINUX', + 'desired': { 'platformName': 'Android', 'automationName': 'uiautomator2', 'platformVersion': '7.1.1', - 'deviceName': 'emulator-5554', + 'deviceName': 'Android Emulator', 'app': '/test/apps/ApiDemos-debug.apk', - 'deviceUDID': 'emulator-5554', - 'appPackage': 'io.appium.android.apis', - 'appWaitPackage': 'io.appium.android.apis', - 'appActivity': 'io.appium.android.apis.ApiDemos', - 'appWaitActivity': 'io.appium.android.apis.ApiDemos', }, - } + 'platformName': 'Android', + 'automationName': 'uiautomator2', + 'platformVersion': '7.1.1', + 'deviceName': 'emulator-5554', + 'app': '/test/apps/ApiDemos-debug.apk', + 'deviceUDID': 'emulator-5554', + 'appPackage': 'io.appium.android.apis', + 'appWaitPackage': 'io.appium.android.apis', + 'appActivity': 'io.appium.android.apis.ApiDemos', + 'appWaitActivity': 'io.appium.android.apis.ApiDemos', + }, } ) From 9e3fff550a4f04c22699d89413cea100529f581f Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Wed, 15 Jun 2022 21:50:21 +0200 Subject: [PATCH 03/10] Address comments --- appium/webdriver/webdriver.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/appium/webdriver/webdriver.py b/appium/webdriver/webdriver.py index c229b440..bf9a9b71 100644 --- a/appium/webdriver/webdriver.py +++ b/appium/webdriver/webdriver.py @@ -322,13 +322,18 @@ def start_session(self, capabilities: Union[Dict, AppiumOptions], browser_profil raise SessionNotCreatedException( f'A valid W3C session creation response must be a dictionary. Got "{response}" instead' ) - if not response.get('sessionId'): + # some browsers pack the createSession response stuff into 'value' dictionary + get_response_value: Callable[[str], Optional[Any]] = lambda key: response.get(key) or ( + response['value'].get(key) if isinstance(response.get('value'), dict) else None + ) + sessionId = get_response_value('sessionId') + if not sessionId: raise SessionNotCreatedException( f'A valid W3C session creation response must contain a non-empty "sessionId" entry. ' f'Got "{response}" instead' ) - self.session_id = response['sessionId'] - self.caps = response.get('capabilities') or {} + self.session_id = sessionId + self.caps = get_response_value('capabilities') or {} def find_element(self, by: str = AppiumBy.ID, value: Union[str, Dict] = None) -> MobileWebElement: """ From 9b334059dabe63d5c16202c21ee2f3cf8370b85f Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Wed, 15 Jun 2022 21:52:55 +0200 Subject: [PATCH 04/10] Check both cases --- test/unit/webdriver/webdriver_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/webdriver/webdriver_test.py b/test/unit/webdriver/webdriver_test.py index 5a57415a..51ea40d9 100644 --- a/test/unit/webdriver/webdriver_test.py +++ b/test/unit/webdriver/webdriver_test.py @@ -37,7 +37,7 @@ def test_create_session(self): httpretty.register_uri( httpretty.POST, f'{SERVER_URL_BASE}/session', - body='{ "sessionId": "session-id", "capabilities": {"deviceName": "Android Emulator"} }', + body='{ "value": {"sessionId": "session-id", "capabilities": {"deviceName": "Android Emulator"}} }', ) desired_caps = { From b3a1f17d55cbdff4aab26e31f9e5716f2ecb81cc Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Thu, 16 Jun 2022 13:18:25 +0200 Subject: [PATCH 05/10] Tune the comment --- appium/webdriver/webdriver.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/appium/webdriver/webdriver.py b/appium/webdriver/webdriver.py index bf9a9b71..e26d0077 100644 --- a/appium/webdriver/webdriver.py +++ b/appium/webdriver/webdriver.py @@ -322,17 +322,19 @@ def start_session(self, capabilities: Union[Dict, AppiumOptions], browser_profil raise SessionNotCreatedException( f'A valid W3C session creation response must be a dictionary. Got "{response}" instead' ) - # some browsers pack the createSession response stuff into 'value' dictionary + # Due to a W3C spec parsing misconception some servers + # pack the createSession response stuff into 'value' dictionary and + # some other put it to the top level of the response JSON nesting hierarchy get_response_value: Callable[[str], Optional[Any]] = lambda key: response.get(key) or ( response['value'].get(key) if isinstance(response.get('value'), dict) else None ) - sessionId = get_response_value('sessionId') - if not sessionId: + session_id = get_response_value('sessionId') + if not session_id: raise SessionNotCreatedException( f'A valid W3C session creation response must contain a non-empty "sessionId" entry. ' f'Got "{response}" instead' ) - self.session_id = sessionId + self.session_id = session_id self.caps = get_response_value('capabilities') or {} def find_element(self, by: str = AppiumBy.ID, value: Union[str, Dict] = None) -> MobileWebElement: From c2a4381768b7fda77f81092f1b1fcc2a6f786d74 Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Thu, 16 Jun 2022 18:33:26 +0200 Subject: [PATCH 06/10] Upgrade pip --- ci-jobs/functional/setup_appium.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/ci-jobs/functional/setup_appium.yml b/ci-jobs/functional/setup_appium.yml index a6a7e6ac..ae96e683 100644 --- a/ci-jobs/functional/setup_appium.yml +++ b/ci-jobs/functional/setup_appium.yml @@ -15,6 +15,7 @@ steps: - script: python setup.py install displayName: Install python language bindings for Appium - script: | + pip install --upgrade pip pip install pipenv pipenv lock --clear pipenv install --system From f452444ceafa985b5c8a9509b9994f655e39509a Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Thu, 16 Jun 2022 20:52:36 +0200 Subject: [PATCH 07/10] Install setuptools rust --- ci-jobs/functional/setup_appium.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/ci-jobs/functional/setup_appium.yml b/ci-jobs/functional/setup_appium.yml index ae96e683..211085ca 100644 --- a/ci-jobs/functional/setup_appium.yml +++ b/ci-jobs/functional/setup_appium.yml @@ -16,6 +16,7 @@ steps: displayName: Install python language bindings for Appium - script: | pip install --upgrade pip + pip install setuptools-rust pip install pipenv pipenv lock --clear pipenv install --system From 6d44c8998b55c1f3708f0f71e858dd9cb328b640 Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Thu, 16 Jun 2022 21:01:22 +0200 Subject: [PATCH 08/10] Upgrade pip earlier --- ci-jobs/functional/setup_appium.yml | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/ci-jobs/functional/setup_appium.yml b/ci-jobs/functional/setup_appium.yml index 211085ca..6a220ef5 100644 --- a/ci-jobs/functional/setup_appium.yml +++ b/ci-jobs/functional/setup_appium.yml @@ -10,17 +10,14 @@ steps: versionSpec: '3.x' - script: brew install ffmpeg displayName: Resolve dependencies (Appium server) -# - script: pip install trio==0.17.0 -# displayName: Install trio -- script: python setup.py install - displayName: Install python language bindings for Appium - script: | pip install --upgrade pip - pip install setuptools-rust pip install pipenv pipenv lock --clear pipenv install --system displayName: Resolve dependencies (Python) +- script: python setup.py install + displayName: Install python language bindings for Appium - script: | git --no-pager log -n1 python --version From 769ef1eb93c25025b0e9aef5c21edebf76fa1a6a Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Thu, 16 Jun 2022 22:06:52 +0200 Subject: [PATCH 09/10] Fix tests --- appium/webdriver/appium_service.py | 25 ++++++++-- .../android/appium_service_tests.py | 47 ++++++++++--------- 2 files changed, 46 insertions(+), 26 deletions(-) diff --git a/appium/webdriver/appium_service.py b/appium/webdriver/appium_service.py index e3e817a1..e99c1ad0 100644 --- a/appium/webdriver/appium_service.py +++ b/appium/webdriver/appium_service.py @@ -13,6 +13,7 @@ # limitations under the License. import os +import re import subprocess as sp import sys import time @@ -24,7 +25,8 @@ DEFAULT_PORT = 4723 STARTUP_TIMEOUT_MS = 60000 MAIN_SCRIPT_PATH = 'appium/build/lib/main.js' -STATUS_URL = '/wd/hub/status' +STATUS_URL = '/status' +DEFAULT_BASE_PATH = '/' def find_executable(executable: str) -> Optional[str]: @@ -47,9 +49,9 @@ def find_executable(executable: str) -> Optional[str]: def poll_url(host: str, port: int, path: str, timeout_ms: int) -> bool: time_started_sec = time.time() + conn = urllib3.PoolManager(timeout=1.0) while time.time() < time_started_sec + timeout_ms / 1000.0: try: - conn = urllib3.PoolManager(timeout=1.0) resp = conn.request('HEAD', f'http://{host}:{port}{path}') if resp.status < 400: return True @@ -112,6 +114,13 @@ def _parse_port(args: List[str]) -> int: return int(args[idx + 1]) return DEFAULT_PORT + @staticmethod + def _parse_base_path(args: List[str]) -> str: + for idx, arg in enumerate(args or []): + if arg in ('--base-path', '-pa') and idx < len(args) - 1: + return args[idx + 1] + return DEFAULT_BASE_PATH + @staticmethod def _parse_host(args: List[str]) -> str: for idx, arg in enumerate(args or []): @@ -166,7 +175,11 @@ def start(self, **kwargs: Any) -> sp.Popen: host = self._parse_host(args) port = self._parse_port(args) error_msg: Optional[str] = None - if not self.is_running or (timeout_ms > 0 and not poll_url(host, port, STATUS_URL, timeout_ms)): + base_path = self._parse_base_path(args) + status_url_path = ( + STATUS_URL if base_path == DEFAULT_BASE_PATH else f'{re.sub(r"[/]+$", "", base_path)}{STATUS_URL}' + ) + if not self.is_running or (timeout_ms > 0 and not poll_url(host, port, status_url_path, timeout_ms)): error_msg = f'Appium has failed to start on {host}:{port} within {timeout_ms}ms timeout' if error_msg is not None: if stderr == sp.PIPE and self._process.stderr is not None: @@ -218,7 +231,11 @@ def is_listening(self) -> bool: return False host = self._parse_host(self._cmd) port = self._parse_port(self._cmd) - return self.is_running and poll_url(host, port, STATUS_URL, 1000) + base_path = self._parse_base_path(self._cmd) + status_url_path = ( + STATUS_URL if base_path == DEFAULT_BASE_PATH else f'{re.sub(r"[/]+$", "", base_path)}{STATUS_URL}' + ) + return self.is_running and poll_url(host, port, status_url_path, 1000) if __name__ == '__main__': diff --git a/test/functional/android/appium_service_tests.py b/test/functional/android/appium_service_tests.py index 806bdb29..6c346f73 100644 --- a/test/functional/android/appium_service_tests.py +++ b/test/functional/android/appium_service_tests.py @@ -13,29 +13,32 @@ # See the License for the specific language governing permissions and # limitations under the License. -from appium.webdriver.appium_service import AppiumService -from appium.webdriver.common.appiumby import AppiumBy -from test.functional.android.helper.test_helper import BaseTestCase -from test.functional.test_helper import wait_for_element - -DEFAULT_PORT = 4723 - +from typing import Generator -class TestAppiumService(BaseTestCase): +import pytest - service: AppiumService - - @classmethod - def setup_class(cls) -> None: - cls.service = AppiumService() - cls.service.start(args=['--address', '127.0.0.1', '-p', str(DEFAULT_PORT)]) +from appium.webdriver.appium_service import AppiumService - def test_appium_service(self) -> None: - assert self.service.is_running - assert self.service.is_listening - el = wait_for_element(self.driver, AppiumBy.ACCESSIBILITY_ID, 'Accessibility') - assert el is not None - @classmethod - def teardown_class(cls) -> None: - cls.service.stop() +@pytest.fixture +def appium_service() -> Generator[AppiumService, None, None]: + service = AppiumService() + service.start( + args=[ + '--address', + '127.0.0.1', + '-p', + '4773', + '--base-path', + '/wd/hub', + ] + ) + try: + yield service + finally: + service.stop() + + +def test_appium_service(appium_service: AppiumService) -> None: + assert appium_service.is_running + assert appium_service.is_listening From 781aa3b22ab62f6ddcb662bf81b741cf40bf7e24 Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Fri, 17 Jun 2022 08:48:18 +0200 Subject: [PATCH 10/10] Skip service test in CI env --- test/functional/android/appium_service_tests.py | 1 + 1 file changed, 1 insertion(+) diff --git a/test/functional/android/appium_service_tests.py b/test/functional/android/appium_service_tests.py index 6c346f73..a400dfb5 100644 --- a/test/functional/android/appium_service_tests.py +++ b/test/functional/android/appium_service_tests.py @@ -39,6 +39,7 @@ def appium_service() -> Generator[AppiumService, None, None]: service.stop() +@pytest.skip('Unstable in CI env') def test_appium_service(appium_service: AppiumService) -> None: assert appium_service.is_running assert appium_service.is_listening