[KOB-52290] Feature/python pytest generator restart#60
Conversation
…overage Generated Python scripts were malformed in several ways: @classmethod landed outside the class body, indent levels drifted with each device block causing pytest to miss all but the first test function, finally was nested inside except causing an immediate SyntaxError, JS true/false appeared as Python literals, XPath selectors with double-quoted attributes broke string quoting, and multi-device if/elif/else branches were nested inside each other. Runtime template gaps were also fixed: missing AppiumBy/By imports in test_app.py, deprecated webdriver.Remote API updated to AppiumOptions in test_base.py, missing action helpers added (send_keys_to_active_element, swipe_on_element, press_button_multiple, rotate_screen, set_location), port handling corrected in config.py, and proxy auth switched from URL-embedded credentials to an Authorization header in proxy_server.py. A direct Python regression test (no gRPC) now generates a real zip from a 2-device fixture and runs ast.parse plus six structural checks over every .py file to protect against recurrence. Note: the pre-existing open-handles warning from the gRPC integration tests in generate-script.test.js is unrelated and left for a separate cleanup. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No documentation changes needed; this PR fixes Python script generation issues and adds regression coverage. |
There was a problem hiding this comment.
Pull request overview
Updates the Python (pytest) script generator and templates to produce syntactically-correct, Appium-client-compatible output (notably around indentation, booleans, and driver initialization), and adds an integration fixture + validator to prevent regressions.
Changes:
- Switch Python template driver creation to use
AppiumOptionsinstead of passing raw desired capabilities. - Fix Python code generation issues: emit Python
True/False, correct indentation handling (includingfinally:), and adjust locator generation to use instance device fields. - Add a Python pytest integration test that generates a script zip and validates output with a small Python-based validator.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/templates/python/test_base.py | Updates driver initialization to AppiumOptions; expands helper APIs used by generated steps; updates element-finding to try multiple locators. |
| src/templates/python/test_app.py | Adds required imports (AppiumBy, By) for generated locator tuples. |
| src/templates/python/proxy_server.py | Changes proxy auth approach to use Authorization header instead of embedding creds in URL. |
| src/templates/python/config.py | Fixes URL-with-auth construction when no port is present; keeps Basic auth helper. |
| src/services/python.js | Fixes Python output correctness (indentation, True/False, locator quoting/branching) and generator indentation offsets. |
| tests/resource/python-pytest-input.json | Adds a 2-device pytest fixture for integration testing. |
| tests/integration/python-pytest.test.js | Adds integration test that generates a pytest zip and validates it. |
| tests/helpers/validate-python-output.py | Adds validator to catch Python syntax/indentation and key content regressions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # 1. Syntax check — ast.parse catches IndentationError, SyntaxError, | ||
| # wrong boolean literals, and broken string literals. | ||
| # ------------------------------------------------------------------ |
There was a problem hiding this comment.
The validator docstring claims ast.parse() “catches wrong boolean literals”, but invalid Python booleans like true/false are parsed as names and won’t raise SyntaxError (they fail at runtime). Consider updating the comment to reflect that the dedicated regex check (section 3) is what catches JS boolean literals, while ast.parse() covers syntax/indentation only.
| if self.current_command_id: | ||
| separator = '&' if '?' in url else '?' | ||
| url = f"{url}{separator}baseCommandId={self.current_command_id}" | ||
|
|
||
| headers = {key: val for key, val in self.headers.items()} | ||
| headers['Authorization'] = Config.get_basic_auth_string() | ||
| req = Request(url, data=body, headers=headers, method=method) |
There was a problem hiding this comment.
current_command_id is maintained on ProxyServer, but ProxyHandler.do_request() reads self.current_command_id where current_command_id is a class attribute on ProxyHandler. As a result, updates via TestBase.set_current_command_id() won't affect the handler and baseCommandId will never be appended. Consider wiring the value through self.server (custom HTTPServer with a current_command_id field) or making ProxyServer.current_command_id update ProxyHandler.current_command_id (e.g., via a property/setter).
| return wait.until(EC.visibility_of_element_located(locator)) | ||
| except Exception as e: | ||
| last_exception = e | ||
| raise last_exception |
There was a problem hiding this comment.
Nit (non-blocking): The new multi-locator loop applies the full WebDriverWait timeout per locator sequentially, so worst case = N locators × timeout. For 2 locators with a 7s timeout, that's ~14s if none match.
The Java/Node.js/C# siblings handle this differently in their findElements — they set implicit wait to 0 (instant check), loop all locators quickly, then retry with a 5s interval, staying within the original timeout budget regardless of locator count.
Not a blocker for this PR since the focus is fixing broken generated code, but worth aligning with the sibling pattern in a follow-up.
There was a problem hiding this comment.
If it is nice to have, I'm going to merge it so it does not block TE testing.
d4rkwinz
changed the title
|
@mimosa767 Please help to add the prefix "ticket" next time on your PR. Thank you! |
Summary
Types of changes
What types of changes does your code introduce to this project? Put an
xin the boxes that apply.Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the pull request. If you are unsure about any of them, do not hesitate to ask. We are here to help! This is simply a reminder of what we are going to look for before merging your code.masterbranch of the upstream.Further comments