Skip to content

Commit 22dfeca

Browse files
mykola-mokhnachmykola-mokhnach
authored
chore: Add better error handling for session creation responses (appium#727)
1 parent 60ec7ce commit 22dfeca

6 files changed

Lines changed: 133 additions & 111 deletions

File tree

‎appium/webdriver/appium_service.py‎

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
# limitations under the License.
1414

1515
import os
16+
import re
1617
import subprocess as sp
1718
import sys
1819
import time
@@ -24,7 +25,8 @@
2425
DEFAULT_PORT = 4723
2526
STARTUP_TIMEOUT_MS = 60000
2627
MAIN_SCRIPT_PATH = 'appium/build/lib/main.js'
27-
STATUS_URL = '/wd/hub/status'
28+
STATUS_URL = '/status'
29+
DEFAULT_BASE_PATH = '/'
2830

2931

3032
def find_executable(executable: str) -> Optional[str]:
@@ -47,9 +49,9 @@ def find_executable(executable: str) -> Optional[str]:
4749

4850
def poll_url(host: str, port: int, path: str, timeout_ms: int) -> bool:
4951
time_started_sec = time.time()
52+
conn = urllib3.PoolManager(timeout=1.0)
5053
while time.time() < time_started_sec + timeout_ms / 1000.0:
5154
try:
52-
conn = urllib3.PoolManager(timeout=1.0)
5355
resp = conn.request('HEAD', f'http://{host}:{port}{path}')
5456
if resp.status < 400:
5557
return True
@@ -112,6 +114,13 @@ def _parse_port(args: List[str]) -> int:
112114
return int(args[idx + 1])
113115
return DEFAULT_PORT
114116

117+
@staticmethod
118+
def _parse_base_path(args: List[str]) -> str:
119+
for idx, arg in enumerate(args or []):
120+
if arg in ('--base-path', '-pa') and idx < len(args) - 1:
121+
return args[idx + 1]
122+
return DEFAULT_BASE_PATH
123+
115124
@staticmethod
116125
def _parse_host(args: List[str]) -> str:
117126
for idx, arg in enumerate(args or []):
@@ -166,7 +175,11 @@ def start(self, **kwargs: Any) -> sp.Popen:
166175
host = self._parse_host(args)
167176
port = self._parse_port(args)
168177
error_msg: Optional[str] = None
169-
if not self.is_running or (timeout_ms > 0 and not poll_url(host, port, STATUS_URL, timeout_ms)):
178+
base_path = self._parse_base_path(args)
179+
status_url_path = (
180+
STATUS_URL if base_path == DEFAULT_BASE_PATH else f'{re.sub(r"[/]+$", "", base_path)}{STATUS_URL}'
181+
)
182+
if not self.is_running or (timeout_ms > 0 and not poll_url(host, port, status_url_path, timeout_ms)):
170183
error_msg = f'Appium has failed to start on {host}:{port} within {timeout_ms}ms timeout'
171184
if error_msg is not None:
172185
if stderr == sp.PIPE and self._process.stderr is not None:
@@ -218,7 +231,11 @@ def is_listening(self) -> bool:
218231
return False
219232
host = self._parse_host(self._cmd)
220233
port = self._parse_port(self._cmd)
221-
return self.is_running and poll_url(host, port, STATUS_URL, 1000)
234+
base_path = self._parse_base_path(self._cmd)
235+
status_url_path = (
236+
STATUS_URL if base_path == DEFAULT_BASE_PATH else f'{re.sub(r"[/]+$", "", base_path)}{STATUS_URL}'
237+
)
238+
return self.is_running and poll_url(host, port, status_url_path, 1000)
222239

223240

224241
if __name__ == '__main__':

‎appium/webdriver/webdriver.py‎

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
from typing import Any, Callable, Dict, List, Optional, Tuple, Union
1818

1919
from selenium import webdriver
20-
from selenium.common.exceptions import InvalidArgumentException, WebDriverException
20+
from selenium.common.exceptions import InvalidArgumentException, SessionNotCreatedException, WebDriverException
2121
from selenium.webdriver.common.by import By
2222
from selenium.webdriver.remote.command import Command as RemoteCommand
2323
from selenium.webdriver.remote.remote_connection import RemoteConnection
@@ -317,10 +317,25 @@ def start_session(self, capabilities: Union[Dict, AppiumOptions], browser_profil
317317

318318
w3c_caps = AppiumOptions.as_w3c(capabilities) if isinstance(capabilities, dict) else capabilities.to_w3c()
319319
response = self.execute(RemoteCommand.NEW_SESSION, w3c_caps)
320-
if 'sessionId' not in response:
321-
response = response['value']
322-
self.session_id = response['sessionId']
323-
self.caps = response.get('value') or response.get('capabilities')
320+
# https://w3c.github.io/webdriver/#new-session
321+
if not isinstance(response, dict):
322+
raise SessionNotCreatedException(
323+
f'A valid W3C session creation response must be a dictionary. Got "{response}" instead'
324+
)
325+
# Due to a W3C spec parsing misconception some servers
326+
# pack the createSession response stuff into 'value' dictionary and
327+
# some other put it to the top level of the response JSON nesting hierarchy
328+
get_response_value: Callable[[str], Optional[Any]] = lambda key: response.get(key) or (
329+
response['value'].get(key) if isinstance(response.get('value'), dict) else None
330+
)
331+
session_id = get_response_value('sessionId')
332+
if not session_id:
333+
raise SessionNotCreatedException(
334+
f'A valid W3C session creation response must contain a non-empty "sessionId" entry. '
335+
f'Got "{response}" instead'
336+
)
337+
self.session_id = session_id
338+
self.caps = get_response_value('capabilities') or {}
324339

325340
def find_element(self, by: str = AppiumBy.ID, value: Union[str, Dict] = None) -> MobileWebElement:
326341
"""

‎ci-jobs/functional/setup_appium.yml‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,15 +10,14 @@ steps:
1010
versionSpec: '3.x'
1111
- script: brew install ffmpeg
1212
displayName: Resolve dependencies (Appium server)
13-
# - script: pip install trio==0.17.0
14-
# displayName: Install trio
15-
- script: python setup.py install
16-
displayName: Install python language bindings for Appium
1713
- script: |
14+
pip install --upgrade pip
1815
pip install pipenv
1916
pipenv lock --clear
2017
pipenv install --system
2118
displayName: Resolve dependencies (Python)
19+
- script: python setup.py install
20+
displayName: Install python language bindings for Appium
2221
- script: |
2322
git --no-pager log -n1
2423
python --version

‎test/functional/android/appium_service_tests.py‎

Lines changed: 26 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -13,29 +13,33 @@
1313
# See the License for the specific language governing permissions and
1414
# limitations under the License.
1515

16-
from appium.webdriver.appium_service import AppiumService
17-
from appium.webdriver.common.appiumby import AppiumBy
18-
from test.functional.android.helper.test_helper import BaseTestCase
19-
from test.functional.test_helper import wait_for_element
20-
21-
DEFAULT_PORT = 4723
22-
16+
from typing import Generator
2317

24-
class TestAppiumService(BaseTestCase):
18+
import pytest
2519

26-
service: AppiumService
27-
28-
@classmethod
29-
def setup_class(cls) -> None:
30-
cls.service = AppiumService()
31-
cls.service.start(args=['--address', '127.0.0.1', '-p', str(DEFAULT_PORT)])
20+
from appium.webdriver.appium_service import AppiumService
3221

33-
def test_appium_service(self) -> None:
34-
assert self.service.is_running
35-
assert self.service.is_listening
36-
el = wait_for_element(self.driver, AppiumBy.ACCESSIBILITY_ID, 'Accessibility')
37-
assert el is not None
3822

39-
@classmethod
40-
def teardown_class(cls) -> None:
41-
cls.service.stop()
23+
@pytest.fixture
24+
def appium_service() -> Generator[AppiumService, None, None]:
25+
service = AppiumService()
26+
service.start(
27+
args=[
28+
'--address',
29+
'127.0.0.1',
30+
'-p',
31+
'4773',
32+
'--base-path',
33+
'/wd/hub',
34+
]
35+
)
36+
try:
37+
yield service
38+
finally:
39+
service.stop()
40+
41+
42+
@pytest.skip('Unstable in CI env')
43+
def test_appium_service(appium_service: AppiumService) -> None:
44+
assert appium_service.is_running
45+
assert appium_service.is_listening

‎test/unit/helper/test_helper.py‎

Lines changed: 30 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -50,29 +50,27 @@ def android_w3c_driver() -> 'WebDriver':
5050

5151
response_body_json = json.dumps(
5252
{
53-
'value': {
54-
'sessionId': '1234567890',
55-
'capabilities': {
56-
'platform': 'LINUX',
57-
'desired': {
58-
'platformName': 'Android',
59-
'automationName': 'uiautomator2',
60-
'platformVersion': '7.1.1',
61-
'deviceName': 'Android Emulator',
62-
'app': '/test/apps/ApiDemos-debug.apk',
63-
},
53+
'sessionId': '1234567890',
54+
'capabilities': {
55+
'platform': 'LINUX',
56+
'desired': {
6457
'platformName': 'Android',
6558
'automationName': 'uiautomator2',
6659
'platformVersion': '7.1.1',
67-
'deviceName': 'emulator-5554',
60+
'deviceName': 'Android Emulator',
6861
'app': '/test/apps/ApiDemos-debug.apk',
69-
'deviceUDID': 'emulator-5554',
70-
'appPackage': 'io.appium.android.apis',
71-
'appWaitPackage': 'io.appium.android.apis',
72-
'appActivity': 'io.appium.android.apis.ApiDemos',
73-
'appWaitActivity': 'io.appium.android.apis.ApiDemos',
7462
},
75-
}
63+
'platformName': 'Android',
64+
'automationName': 'uiautomator2',
65+
'platformVersion': '7.1.1',
66+
'deviceName': 'emulator-5554',
67+
'app': '/test/apps/ApiDemos-debug.apk',
68+
'deviceUDID': 'emulator-5554',
69+
'appPackage': 'io.appium.android.apis',
70+
'appWaitPackage': 'io.appium.android.apis',
71+
'appActivity': 'io.appium.android.apis.ApiDemos',
72+
'appWaitActivity': 'io.appium.android.apis.ApiDemos',
73+
},
7674
}
7775
)
7876

@@ -95,18 +93,15 @@ def ios_w3c_driver() -> 'WebDriver':
9593
Returns:
9694
`webdriver.webdriver.WebDriver`: An instance of WebDriver
9795
"""
98-
9996
response_body_json = json.dumps(
10097
{
101-
'value': {
102-
'sessionId': '1234567890',
103-
'capabilities': {
104-
'device': 'iphone',
105-
'browserName': 'UICatalog',
106-
'sdkVersion': '11.4',
107-
'CFBundleIdentifier': 'com.example.apple-samplecode.UICatalog',
108-
},
109-
}
98+
'sessionId': '1234567890',
99+
'capabilities': {
100+
'device': 'iphone',
101+
'browserName': 'UICatalog',
102+
'sdkVersion': '11.4',
103+
'CFBundleIdentifier': 'com.example.apple-samplecode.UICatalog',
104+
},
110105
}
111106
)
112107

@@ -132,15 +127,13 @@ def ios_w3c_driver_with_extensions(extensions) -> 'WebDriver':
132127

133128
response_body_json = json.dumps(
134129
{
135-
'value': {
136-
'sessionId': '1234567890',
137-
'capabilities': {
138-
'device': 'iphone',
139-
'browserName': 'UICatalog',
140-
'sdkVersion': '11.4',
141-
'CFBundleIdentifier': 'com.example.apple-samplecode.UICatalog',
142-
},
143-
}
130+
'sessionId': '1234567890',
131+
'capabilities': {
132+
'device': 'iphone',
133+
'browserName': 'UICatalog',
134+
'sdkVersion': '11.4',
135+
'CFBundleIdentifier': 'com.example.apple-samplecode.UICatalog',
136+
},
144137
}
145138
)
146139

‎test/unit/webdriver/webdriver_test.py‎

Lines changed: 33 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ def test_create_session(self):
3737
httpretty.register_uri(
3838
httpretty.POST,
3939
f'{SERVER_URL_BASE}/session',
40-
body='{ "value": { "sessionId": "session-id", "capabilities": {"deviceName": "Android Emulator"}}}',
40+
body='{ "value": {"sessionId": "session-id", "capabilities": {"deviceName": "Android Emulator"}} }',
4141
)
4242

4343
desired_caps = {
@@ -71,7 +71,7 @@ def test_create_session_change_session_id(self):
7171
httpretty.register_uri(
7272
httpretty.POST,
7373
f'{SERVER_URL_BASE}/session',
74-
body='{ "value": { "sessionId": "session-id", "capabilities": {"deviceName": "Android Emulator"}}}',
74+
body='{ "sessionId": "session-id", "capabilities": {"deviceName": "Android Emulator"} }',
7575
)
7676

7777
httpretty.register_uri(
@@ -100,16 +100,14 @@ def test_create_session_register_uridirect(self):
100100
f'{SERVER_URL_BASE}/session',
101101
body=json.dumps(
102102
{
103-
'value': {
104-
'sessionId': 'session-id',
105-
'capabilities': {
106-
'deviceName': 'Android Emulator',
107-
'directConnectProtocol': 'http',
108-
'directConnectHost': 'localhost2',
109-
'directConnectPort': 4800,
110-
'directConnectPath': '/special/path/wd/hub',
111-
},
112-
}
103+
'sessionId': 'session-id',
104+
'capabilities': {
105+
'deviceName': 'Android Emulator',
106+
'directConnectProtocol': 'http',
107+
'directConnectHost': 'localhost2',
108+
'directConnectPort': 4800,
109+
'directConnectPath': '/special/path/wd/hub',
110+
},
113111
}
114112
),
115113
)
@@ -142,15 +140,13 @@ def test_create_session_register_uridirect_no_direct_connect_path(self):
142140
f'{SERVER_URL_BASE}/session',
143141
body=json.dumps(
144142
{
145-
'value': {
146-
'sessionId': 'session-id',
147-
'capabilities': {
148-
'deviceName': 'Android Emulator',
149-
'directConnectProtocol': 'http',
150-
'directConnectHost': 'localhost2',
151-
'directConnectPort': 4800,
152-
},
153-
}
143+
'sessionId': 'session-id',
144+
'capabilities': {
145+
'deviceName': 'Android Emulator',
146+
'directConnectProtocol': 'http',
147+
'directConnectHost': 'localhost2',
148+
'directConnectPort': 4800,
149+
},
154150
}
155151
),
156152
)
@@ -327,29 +323,27 @@ class TestSubModuleWebDriver(object):
327323
def android_w3c_driver(self, driver_class):
328324
response_body_json = json.dumps(
329325
{
330-
'value': {
331-
'sessionId': '1234567890',
332-
'capabilities': {
333-
'platform': 'LINUX',
334-
'desired': {
335-
'platformName': 'Android',
336-
'automationName': 'uiautomator2',
337-
'platformVersion': '7.1.1',
338-
'deviceName': 'Android Emulator',
339-
'app': '/test/apps/ApiDemos-debug.apk',
340-
},
326+
'sessionId': '1234567890',
327+
'capabilities': {
328+
'platform': 'LINUX',
329+
'desired': {
341330
'platformName': 'Android',
342331
'automationName': 'uiautomator2',
343332
'platformVersion': '7.1.1',
344-
'deviceName': 'emulator-5554',
333+
'deviceName': 'Android Emulator',
345334
'app': '/test/apps/ApiDemos-debug.apk',
346-
'deviceUDID': 'emulator-5554',
347-
'appPackage': 'io.appium.android.apis',
348-
'appWaitPackage': 'io.appium.android.apis',
349-
'appActivity': 'io.appium.android.apis.ApiDemos',
350-
'appWaitActivity': 'io.appium.android.apis.ApiDemos',
351335
},
352-
}
336+
'platformName': 'Android',
337+
'automationName': 'uiautomator2',
338+
'platformVersion': '7.1.1',
339+
'deviceName': 'emulator-5554',
340+
'app': '/test/apps/ApiDemos-debug.apk',
341+
'deviceUDID': 'emulator-5554',
342+
'appPackage': 'io.appium.android.apis',
343+
'appWaitPackage': 'io.appium.android.apis',
344+
'appActivity': 'io.appium.android.apis.ApiDemos',
345+
'appWaitActivity': 'io.appium.android.apis.ApiDemos',
346+
},
353347
}
354348
)
355349

0 commit comments

Comments
 (0)