[KOB-52290][KOB-52412] Fix 5 bugs in generated Python pytest template#61
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Python pytest script generator/templates to run correctly with Appium-Python-Client 5.x, aligning generated code and proxy behavior with newer client/server expectations.
Changes:
- Update generated Python templates for Appium 5.x driver initialization and add helper APIs used by generated steps.
- Revise Python proxy forwarding to use
requests, add timeouts/method support, and improve session/response handling for Kobiton. - Add a Python output validator + Jest integration test fixture to catch syntax/indentation/import regressions in generated output.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/templates/python/test_base.py | Updates driver initialization and expands helper methods used by generated tests. |
| src/templates/python/test_app.py | Adds missing module-scope imports needed by generated locator code. |
| src/templates/python/proxy_server.py | Reworks proxying implementation (requests-based forwarding, timeouts, session handling). |
| src/templates/python/config.py | Fixes URL auth construction when server URL omits an explicit port. |
| src/services/python.js | Fixes Python code generation (indentation, booleans, locator generation, action mapping). |
| tests/resource/python-pytest-input.json | Adds a fixture input representing a 2-device pytest generation scenario. |
| tests/integration/python-pytest.test.js | Adds an integration test that generates a zip and validates the Python output. |
| tests/helpers/validate-python-output.py | Adds a validator script to assert generated Python syntax/structure invariants. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| response = requests.get(url, headers=headers, verify=False, timeout=SOCKET_TIMEOUT_SECONDS) | ||
| elif method == 'POST': | ||
| response = requests.post(url, data=body, headers=headers, verify=False, timeout=SOCKET_TIMEOUT_SECONDS) | ||
| elif method == 'DELETE': | ||
| response = requests.delete(url, headers=headers, verify=False, timeout=SOCKET_TIMEOUT_SECONDS) | ||
| else: | ||
| response = requests.request(method, url, data=body, headers=headers, verify=False, timeout=SOCKET_TIMEOUT_SECONDS) |
There was a problem hiding this comment.
Outbound proxy requests set verify=False, disabling TLS certificate verification. This is insecure by default and allows MITM attacks. Prefer keeping verification enabled, or make disabling verification an explicit, documented opt-in setting.
| response = requests.get(url, headers=headers, verify=False, timeout=SOCKET_TIMEOUT_SECONDS) | |
| elif method == 'POST': | |
| response = requests.post(url, data=body, headers=headers, verify=False, timeout=SOCKET_TIMEOUT_SECONDS) | |
| elif method == 'DELETE': | |
| response = requests.delete(url, headers=headers, verify=False, timeout=SOCKET_TIMEOUT_SECONDS) | |
| else: | |
| response = requests.request(method, url, data=body, headers=headers, verify=False, timeout=SOCKET_TIMEOUT_SECONDS) | |
| response = requests.get(url, headers=headers, timeout=SOCKET_TIMEOUT_SECONDS) | |
| elif method == 'POST': | |
| response = requests.post(url, data=body, headers=headers, timeout=SOCKET_TIMEOUT_SECONDS) | |
| elif method == 'DELETE': | |
| response = requests.delete(url, headers=headers, timeout=SOCKET_TIMEOUT_SECONDS) | |
| else: | |
| response = requests.request(method, url, data=body, headers=headers, timeout=SOCKET_TIMEOUT_SECONDS) |
| base_url = Config.get_appium_server_url_with_auth() | ||
| print(f"[PROXY] Base URL from config: {base_url}", file=sys.stderr, flush=True) | ||
|
|
There was a problem hiding this comment.
base_url is built from get_appium_server_url_with_auth() (which embeds username:apiKey@...) and then printed. This leaks credentials into logs. Avoid embedding secrets in URLs where possible, and never log URLs/strings that contain credentials.
| # Log the Authorization header | ||
| auth_header = headers.get('Authorization', 'NOT SET') | ||
| print(f"[PROXY] Authorization header: {auth_header}", file=sys.stderr, flush=True) | ||
| print(f"[PROXY] All request headers: {headers}", file=sys.stderr, flush=True) |
There was a problem hiding this comment.
The proxy prints the Authorization header value to stderr. This will leak credentials into CI logs and local output. Mask the header (e.g., show only scheme / last 4 chars) or remove this log entirely.
| self.send_response(response.status_code) | ||
| for key, val in response.headers.items(): | ||
| self.send_header(key, val) | ||
| self.end_headers() |
There was a problem hiding this comment.
The proxy may modify response_body (session/error conversion) but still forwards upstream headers unchanged. If the upstream response contains Content-Length, it can become incorrect and cause client hangs/truncation. Drop/recompute Content-Length (and other hop-by-hop headers) based on the final body you write.
| # 2b. test_base.py must import AppiumOptions from its real module path. | ||
| # In Appium-Python-Client 3.x/4.x, AppiumOptions lives at | ||
| # appium.options.common.base, NOT appium.options — the shorter | ||
| # path raises ImportError at collection time. | ||
| # ------------------------------------------------------------------ | ||
| test_base = py_files.get('test_base.py', '') | ||
| if 'from appium.options.common.base import AppiumOptions' not in test_base: | ||
| errors.append( | ||
| 'test_base.py: missing or wrong AppiumOptions import — ' | ||
| 'must be "from appium.options.common.base import AppiumOptions"' |
There was a problem hiding this comment.
This check still requires test_base.py to import AppiumOptions from appium.options.common.base, but the template now imports UiAutomator2Options. As written, the integration test will fail even when generation is correct. Update the assertion to match the intended options import(s) (and consider covering both Android and iOS options if supported).
| # 2b. test_base.py must import AppiumOptions from its real module path. | |
| # In Appium-Python-Client 3.x/4.x, AppiumOptions lives at | |
| # appium.options.common.base, NOT appium.options — the shorter | |
| # path raises ImportError at collection time. | |
| # ------------------------------------------------------------------ | |
| test_base = py_files.get('test_base.py', '') | |
| if 'from appium.options.common.base import AppiumOptions' not in test_base: | |
| errors.append( | |
| 'test_base.py: missing or wrong AppiumOptions import — ' | |
| 'must be "from appium.options.common.base import AppiumOptions"' | |
| # 2b. test_base.py must import a supported Appium options class. | |
| # Current templates use platform-specific options such as | |
| # UiAutomator2Options (Android) and XCUITestOptions (iOS). | |
| # Keep AppiumOptions accepted as a legacy fallback so validation | |
| # does not fail for otherwise-correct generated output. | |
| # ------------------------------------------------------------------ | |
| test_base = py_files.get('test_base.py', '') | |
| valid_options_imports = ( | |
| 'from appium.options.android import UiAutomator2Options', | |
| 'from appium.options.ios import XCUITestOptions', | |
| 'from appium.options.common.base import AppiumOptions', | |
| ) | |
| if not any(import_line in test_base for import_line in valid_options_imports): | |
| errors.append( | |
| 'test_base.py: missing or wrong Appium options import — ' | |
| 'must import one of ' | |
| '"from appium.options.android import UiAutomator2Options", ' | |
| '"from appium.options.ios import XCUITestOptions", or ' | |
| '"from appium.options.common.base import AppiumOptions"' |
| print(f"Initialize Appium driver with desiredCaps: {desired_caps}") | ||
| server_url = self._proxy.get_server_url() + '/wd/hub' | ||
| self._driver = webdriver.Remote(server_url, desired_caps) | ||
| options = UiAutomator2Options().load_capabilities(desired_caps) | ||
| self._driver = webdriver.Remote(server_url, options=options) |
There was a problem hiding this comment.
setup() always constructs UiAutomator2Options regardless of platformName. This will break iOS runs (and any non-UiAutomator2 driver) because iOS should use the iOS option class (e.g., XCUITestOptions) or a generic options builder compatible with both platforms. Consider selecting the options class based on platformName and falling back to a common options type when the platform is not Android.
| raise last_exception | ||
|
|
||
| def find_visible_element_on_scrollable(self, timeout_ms, locator): | ||
| return self.find_visible_element(timeout_ms, locator) |
There was a problem hiding this comment.
find_visible_element_on_scrollable() passes a single locator tuple into find_visible_element(), but find_visible_element() now iterates over locators. Passing a tuple will iterate over its two items (by/value) and then call visibility_of_element_located() with invalid locator data. Wrap the single locator in a list/tuple, or update find_visible_element() to accept both a single locator and a list of locators.
| return self.find_visible_element(timeout_ms, locator) | |
| return self.find_visible_element(timeout_ms, [locator]) |
The generated Python pytest bundles fail out of the box. Bugs identified and re-verified by Tan Tri Quach against portal v4.26.0-bd345ab in KOB-52412: - test_base.py: import AppiumOptions via platform-specific UiAutomator2Options (was: `from appium.options import AppiumOptions`, which is empty in 3.1.0) - test_base.py: call load_capabilities as instance method (was: classmethod call, which is invalid in 3.1.0) - test_base.py: device-availability check now recognizes the full /v1/devices response shape (privateDevices/cloudDevices/favoriteDevices and others), not just legacy `deviceListData` - proxy_server.py: strip `Host` header before forwarding upstream (was: forwarded `Host: localhost`, which made the Kobiton gateway 404) - proxy_server.py: rewritten on `requests`; forwards real upstream status and body instead of wrapping 4xx/5xx into a synthetic 502 that hid the underlying error Also adds validator hooks in __tests__/helpers/validate-python-output.py that guard against regressing each of these bugs in future generations. Refs: KOB-52290, KOB-52412 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mimosa767
force-pushed
the
feature/KOB-52290-fix-appium-options-import
branch
from
d84c194 to
342945d
Compare
mimosa767
changed the title
|
@chuong777 @d4rkwinz — could one of you take a look at this when you get a chance? PR is rebased onto current master, squashed to one commit, all 5 bugs from KOB-52412 fixed. Tri Truong specifically suggested routing through Chuong for this one. Thanks! |
|
Sure @mimosa767 |
Summary
The generated Python pytest bundles fail to run out of the box. This PR fixes all 5 template bugs identified and re-verified by @tan Tri Quach against portal v4.26.0-bd345ab.
UiAutomator2Options(wasfrom appium.options import AppiumOptions, which is empty in Appium-Python-Client 3.1.0)load_capabilitiesas an instance method (was classmethod-style call, invalid in 3.1.0)/v1/devicesresponse shape (privateDevices/cloudDevices/favoriteDevices/etc.), not just legacydeviceListDataHostheader before forwarding (was forwardingHost: localhost, which made the Kobiton gateway return 404)requests; forwards real upstream status/body instead of wrapping 4xx/5xx into a synthetic 502 that hid the underlying errorAlso adds validator hooks in
__tests__/helpers/validate-python-output.pythat guard against regressions on each bug.JIRA
Test plan
pip3 install -r requirements.txt(Python 3.9+).pytest test_suite.py -vprovisions the target Kobiton device and runs the recorded steps end-to-end.Notes