Merged
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
📝 WalkthroughWalkthroughAdds two new drivers— Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AdbClient
participant TcpPortforwardAdapter
participant RemoteAdbServer
Client->>AdbClient: invoke adb subcommand
AdbClient->>AdbClient: check persistent tunnel state
alt Tunnel active and usable
AdbClient->>TcpPortforwardAdapter: reuse existing tunnel (host,port)
else No active tunnel
AdbClient->>TcpPortforwardAdapter: create ephemeral forward (remote ADB -> local TCP)
end
TcpPortforwardAdapter->>RemoteAdbServer: establish port forward
RemoteAdbServer-->>TcpPortforwardAdapter: return local (host,port)
AdbClient->>AdbClient: set ANDROID_ADB_SERVER_ADDRESS/PORT for subprocess
AdbClient->>Client: run local adb subprocess with env
AdbClient-->>Client: return adb result
sequenceDiagram
participant TestClient
participant AndroidEmulatorClient
participant AdbServer
participant AndroidEmulatorPower
participant EmulatorProcess
TestClient->>AndroidEmulatorClient: set_headless(true)
AndroidEmulatorClient->>AndroidEmulatorPower: store headless flag
TestClient->>AndroidEmulatorClient: request adb_device()
AndroidEmulatorClient->>AdbServer: forward_adb(port=0)
AdbServer->>EmulatorProcess: open tunnel to emulator ADB
EmulatorProcess-->>AdbServer: return (host,port)
AndroidEmulatorClient->>AndroidEmulatorClient: create adbutils.AdbClient(host,port)
AndroidEmulatorClient->>AndroidEmulatorClient: poll getprop sys.boot_completed until "1"
AndroidEmulatorClient->>AndroidEmulatorClient: list_devices() and validate presence
AndroidEmulatorClient-->>TestClient: yield adb_device context
TestClient->>TestClient: run device tests (push/pull/shell)
TestClient->>AndroidEmulatorClient: exit context (cleanup)
AndroidEmulatorClient->>AdbServer: close tunnel
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (15)
python/packages/jumpstarter-driver-adb/examples/exporter.yaml (1)
1-19: LGTM! Clear example configuration.The exporter config is well-documented with helpful comments explaining prerequisites. The configuration correctly specifies the ADB server connection parameters. The omission of
adb_pathis intentional and will use the default resolution via PATH.Minor: Add trailing newline
Consider adding a trailing newline for POSIX text file compliance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-adb/examples/exporter.yaml` around lines 1 - 19, Add a POSIX-compliant trailing newline to the YAML example for the ExporterConfig named "adb-local" (apiVersion jumpstarter.dev/v1alpha1, kind ExporterConfig) by ensuring the file ends with a single newline character after the final "port: 15037" line; this is a purely formatting change to the examples/exporter.yaml content so text tools and linters recognize it as a POSIX text file.python/packages/jumpstarter-driver-androidemulator/.gitignore (1)
1-3: LGTM! Standard Python ignore patterns.The ignore patterns correctly exclude Python cache and coverage artifacts. Optionally, consider adding a trailing newline for POSIX compliance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-androidemulator/.gitignore` around lines 1 - 3, The .gitignore contents correctly list __pycache__/, .coverage, and coverage.xml but the file lacks a trailing newline; open the file (python/packages/jumpstarter-driver-androidemulator/.gitignore) and ensure you add a single POSIX-compliant newline character at the end of the file so the last entry (coverage.xml) is terminated by a newline.python/packages/jumpstarter-driver-androidemulator/examples/local-exporter.yaml (1)
1-15: LGTM! Correct emulator configuration.The exporter config properly instantiates the AndroidEmulator driver with appropriate parameters. The omission of
emulator_pathis intentional and will use the default value. Theheadless: falsesetting is useful for local development and debugging. Port settings are consistent with Android emulator defaults and the ADB driver example.Optional: Add trailing newline
Consider adding a trailing newline for POSIX text file compliance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-androidemulator/examples/local-exporter.yaml` around lines 1 - 15, Add a POSIX-compliant trailing newline to the YAML file by ensuring the final line of the ExporterConfig (resource name "android-emulator" / kind ExporterConfig) ends with a newline character; simply update the file to include a single newline at EOF so the file ends with a blank line.python/packages/jumpstarter-driver-adb/.gitignore (1)
1-3: LGTM! Standard Python ignore patterns.The ignore patterns correctly exclude Python cache and coverage artifacts. Optionally, consider adding a trailing newline for POSIX compliance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-adb/.gitignore` around lines 1 - 3, The .gitignore file includes standard patterns (__pycache__/, .coverage, coverage.xml) but lacks a trailing newline; open the file named .gitignore in the python/packages/jumpstarter-driver-adb package and add a single trailing newline at EOF so the file ends with a newline character to satisfy POSIX conventions while keeping the existing ignore patterns unchanged.python/examples/android-emulator/pyproject.toml (1)
1-16: LGTM! Well-structured example package configuration.The package metadata and dependencies are appropriate for an Android emulator testing example. The pytest configuration with live logging at INFO level will help with debugging. The use of the
[python-api]extra for the androidemulator driver is a good practice.Optional: Add trailing newline
Consider adding a trailing newline for POSIX text file compliance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/examples/android-emulator/pyproject.toml` around lines 1 - 16, The file ends without a POSIX-compliant trailing newline; open the pyproject.toml that contains the [project] table (e.g., the entry with name = "jumpstarter-example-android-emulator") and add a single newline character at the end of the file so the file terminates with a trailing newline.python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver_test.py (1)
32-39: Add a test for non-integer port validation.
driver.pyvalidates both range and type; this file currently covers range only. Adding a non-int case will close an important config-validation branch.Suggested test addition
def test_invalid_port_too_high(): with pytest.raises(ConfigurationError): AdbServer(port=70000) +def test_invalid_port_type(): + with pytest.raises(ConfigurationError, match="Port must be an integer"): + AdbServer(port="5037")Based on learnings: Each driver package must include comprehensive tests in
driver_test.py.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver_test.py` around lines 32 - 39, Add a new pytest that asserts AdbServer rejects non-integer ports: create a test function (e.g., test_invalid_port_non_integer) that calls AdbServer(port="not-an-int") (or port=1.5) inside with pytest.raises(ConfigurationError) to cover the type-check branch in the AdbServer constructor/driver.py validation logic.python/packages/jumpstarter-driver-androidemulator/examples/exporter.yaml (1)
13-13: Consider making the example headless by default.Line 13 sets
headless: false; this often fails on CI/remote Linux nodes without a display server. Consider defaulting totrueand mentioning how to enable GUI mode when needed.Suggested tweak
- headless: false + headless: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-androidemulator/examples/exporter.yaml` at line 13, Update the example to make the emulator run headlessly by default: change the headless setting in exporter.yaml from false to true (the `headless:` key), and add a short note in the example/comments explaining how to enable GUI mode when needed (e.g., set `headless: false` or provide DISPLAY/Xvfb instructions) so CI/remote Linux users won’t fail on systems without a display server.python/examples/android-emulator/jumpstarter_example_android_emulator/conftest.py (1)
19-22: Harden teardown with atry/finallyaround power lifecycle.If
client.power.on()fails after partially starting the emulator, Line 22 won’t run today. Wrappingon/yield/offintry/finallymakes cleanup more reliable.Suggested refactor
from jumpstarter.common.utils import serve +from contextlib import suppress @@ with serve(driver) as client: - client.power.on() - yield client - client.power.off() + try: + client.power.on() + yield client + finally: + with suppress(Exception): + client.power.off()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/examples/android-emulator/jumpstarter_example_android_emulator/conftest.py` around lines 19 - 22, The current serve(driver) context block calls client.power.on(), yields the client, then calls client.power.off() but if client.power.on() or anything before the finally fails the power.off() won’t run; wrap the on/yield/off sequence in a try/finally so that after obtaining client (inside the with serve(driver) as client: block) you call client.power.on() then try: yield client finally: client.power.off() to guarantee cleanup even on errors in client.power.on(), during yield, or downstream; update the block around serve(driver) and the client.power lifecycle (client.power.on, yield client, client.power.off) accordingly.python/packages/jumpstarter-driver-androidemulator/jumpstarter_driver_androidemulator/driver.py (1)
154-162: Inconsistent ADB path resolution in off() method.The
off()method resolvesadbpath separately usingshutil.which("adb"), while the parent already has anAdbServerchild with a resolvedadb_path. Consider reusing the already-resolved path for consistency.♻️ Proposed fix
# Try graceful shutdown via ADB try: - adb_path = shutil.which("adb") or "adb" + adb_child = self.parent.children["adb"] + adb_path = adb_child.adb_path subprocess.run( [adb_path, "-s", f"emulator-{self.parent.console_port}", "emu", "kill"],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-androidemulator/jumpstarter_driver_androidemulator/driver.py` around lines 154 - 162, The off() method currently calls shutil.which("adb") locally; change it to reuse the already-resolved ADB path from the parent's AdbServer instance (e.g. use self.parent.adb_server.adb_path) when constructing the subprocess.run call, keeping the same env override for "ANDROID_ADB_SERVER_PORT" and the same arguments (including f"emulator-{self.parent.console_port}") so ADB invocation is consistent with the rest of the codebase.python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver.py (2)
90-107: Consider adding a timeout to subprocess.run for kill_server.Same concern as
start_server- the call could block indefinitely without a timeout.♻️ Proposed fix
try: result = subprocess.run( [self.adb_path, "kill-server"], check=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, env=self._adb_env(), + timeout=30, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver.py` around lines 90 - 107, The kill_server method calls subprocess.run without a timeout which can block; add a timeout argument (consistent with the timeout used in start_server) to the subprocess.run call inside kill_server and handle subprocess.TimeoutExpired by catching it (in addition to subprocess.CalledProcessError), logging an appropriate error via self.logger.error (including the timeout info and self.port), and returning self.port as before; update the subprocess.run invocation in kill_server (which uses self.adb_path, "kill-server", env=self._adb_env()) to include the timeout and ensure stderr/stdout handling remains unchanged.
69-88: Consider adding a timeout to subprocess.run for start_server.If the ADB server hangs during startup, this call will block indefinitely. Adding a reasonable timeout (e.g., 30 seconds) would prevent the driver from getting stuck.
♻️ Proposed fix
try: result = subprocess.run( [self.adb_path, "start-server"], check=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, env=self._adb_env(), + timeout=30, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver.py` around lines 69 - 88, The start_server method calls subprocess.run without a timeout which can hang; update the call in start_server to pass a reasonable timeout (e.g., timeout=30) and add an except block for subprocess.TimeoutExpired to log the timeout (using self.logger.error or self.logger.warning) and handle cleanup if needed, then return the port as before; reference the subprocess.run call in start_server and the subprocess.TimeoutExpired exception to implement the new timeout and error handling.python/packages/jumpstarter-driver-androidemulator/jumpstarter_driver_androidemulator/client.py (1)
41-54: Consider adding type hint for theadbparameter.The
adbparameter lacks a type hint, which reduces code clarity. Sinceadbutilsis an optional dependency, you could use a string annotation orAny.♻️ Proposed fix
- def _wait_for_boot(self, adb, timeout: int = 180) -> None: + def _wait_for_boot(self, adb: "adbutils.AdbClient", timeout: int = 180) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-androidemulator/jumpstarter_driver_androidemulator/client.py` around lines 41 - 54, The _wait_for_boot method's adb parameter lacks a type hint; update the signature of _wait_for_boot to annotate adb (e.g., adb: Any or a string forward-reference like 'adbutils.AdbClient') and add the corresponding import from typing (Any) at the top of the file so static checkers and readers know this is an external/optional dependency; ensure the rest of the function uses the same parameter name (_wait_for_boot(self, adb: Any, timeout: int = 180) -> None).python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/client.py (1)
27-50: Potential race condition in tunnel state file operations.Multiple processes could race between reading and writing the tunnel state file. While unlikely in typical usage, consider using file locking or atomic writes for robustness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/client.py` around lines 27 - 50, The tunnel state file read/write/remove functions (_read_tunnel_state, _write_tunnel_state, _remove_tunnel_state) are vulnerable to races; fix them by using atomic file writes and OS-level file locks: when writing in _write_tunnel_state, write to a temp file and atomically replace _TUNNEL_STATE_FILE (os.replace) and acquire an exclusive lock (fcntl.flock or a cross-platform locker) while writing; when reading in _read_tunnel_state, acquire a shared lock before opening/parsing the JSON and before verifying the pid to ensure consistency; similarly acquire an exclusive lock in _remove_tunnel_state before unlinking. Ensure locks are released in finally blocks and keep JSON port stored as int.python/examples/android-emulator/setup.sh (1)
78-87: AVD existence check could have false positives.The grep pattern
"Name: $AVD_NAME"might match partial AVD names (e.g.,jumpstarter_test_oldwhen checking forjumpstarter_test). Consider using word boundaries or exact matching.♻️ Proposed fix for more precise matching
-if ! avdmanager list avd 2>/dev/null | grep -q "Name: $AVD_NAME"; then +if ! avdmanager list avd 2>/dev/null | grep -q "Name: ${AVD_NAME}$"; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/examples/android-emulator/setup.sh` around lines 78 - 87, The current AVD existence check using grep "Name: $AVD_NAME" can match substrings and yield false positives; update the check around avdmanager list avd to perform an exact line match for the AVD name (e.g., match the "Name:" line exactly for AVD_NAME using start/end anchors and optional whitespace) so only the precise AVD name is detected; modify the grep invocation used with avdmanager list avd and the AVD_NAME variable to use a regex like ^Name:[[:space:]]*<AVD_NAME>$ (or an equivalent exact-match approach) to avoid partial matches.python/examples/android-emulator/jumpstarter_example_android_emulator/test_android_emulator.py (1)
31-36: Consider a more robust wait for activity launch.The fixed
time.sleep(2)may be flaky on slower emulators. Consider pollingdumpsysin a loop with a timeout, similar to the boot-wait pattern used elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/examples/android-emulator/jumpstarter_example_android_emulator/test_android_emulator.py` around lines 31 - 36, Replace the fixed time.sleep(2) in test_launch_settings with a polling loop (following the boot-wait pattern) that repeatedly calls adb_device.shell("dumpsys activity activities") and checks for "settings" in the output (case-insensitive) until a configurable timeout (e.g., 20–30s) is reached; if the timeout expires, fail the test with an assertion or exception. Locate the logic in the test_launch_settings function and use adb_device as the source of dumpsys output, sleeping briefly (e.g., 0.5s) between polls to avoid tight spinning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/client.py`:
- Around line 150-169: The tunnel branch currently reports an existing tunnel
without comparing requested CLI host/port; update the logic in the args[0] ==
"tunnel" block to read the requested host and port (the variables passed into
forward_adb) and compare them to the existing state returned by
_read_tunnel_state(); if state exists and matches (compare host strings and port
as int/str as needed) print the "already running" message and return 0, but if
state exists and does not match, print a clear error saying the requested
host/port are in use (include state['host'] and state['port']) and return a
non-zero status instead of silently claiming success; use the same symbols
forward_adb, _read_tunnel_state, _write_tunnel_state, and _remove_tunnel_state
to locate and implement the checks.
In `@python/packages/jumpstarter-driver-adb/README.md`:
- Around line 163-164: Fix the typo in the README sentence that currently reads
"You can also preform interactions via ADB using the
[`adbutils`](https://github.com/openatx/adbutils) Python package." — change
"preform" to "perform" so the sentence correctly reads "You can also perform
interactions via ADB using the [`adbutils`](https://github.com/openatx/adbutils)
Python package." Ensure this edit is applied in the README.md near the adbutils
mention.
In
`@python/packages/jumpstarter-driver-androidemulator/jumpstarter_driver_androidemulator/driver_test.py`:
- Around line 13-15: Tests patch the global shutil.which for AdbServer usage;
change those `@patch` decorators to target the module where AdbServer imports
shutil (use jumpstarter_driver_adb.driver.shutil.which) so the patch applies to
the module-scoped symbol. Update every similar decorator that currently patches
"shutil.which" in this test file (affecting tests that exercise AdbServer and
related functions) to patch "jumpstarter_driver_adb.driver.shutil.which"
instead, keeping the same return_value and order as the other patches.
---
Nitpick comments:
In
`@python/examples/android-emulator/jumpstarter_example_android_emulator/conftest.py`:
- Around line 19-22: The current serve(driver) context block calls
client.power.on(), yields the client, then calls client.power.off() but if
client.power.on() or anything before the finally fails the power.off() won’t
run; wrap the on/yield/off sequence in a try/finally so that after obtaining
client (inside the with serve(driver) as client: block) you call
client.power.on() then try: yield client finally: client.power.off() to
guarantee cleanup even on errors in client.power.on(), during yield, or
downstream; update the block around serve(driver) and the client.power lifecycle
(client.power.on, yield client, client.power.off) accordingly.
In
`@python/examples/android-emulator/jumpstarter_example_android_emulator/test_android_emulator.py`:
- Around line 31-36: Replace the fixed time.sleep(2) in test_launch_settings
with a polling loop (following the boot-wait pattern) that repeatedly calls
adb_device.shell("dumpsys activity activities") and checks for "settings" in the
output (case-insensitive) until a configurable timeout (e.g., 20–30s) is
reached; if the timeout expires, fail the test with an assertion or exception.
Locate the logic in the test_launch_settings function and use adb_device as the
source of dumpsys output, sleeping briefly (e.g., 0.5s) between polls to avoid
tight spinning.
In `@python/examples/android-emulator/pyproject.toml`:
- Around line 1-16: The file ends without a POSIX-compliant trailing newline;
open the pyproject.toml that contains the [project] table (e.g., the entry with
name = "jumpstarter-example-android-emulator") and add a single newline
character at the end of the file so the file terminates with a trailing newline.
In `@python/examples/android-emulator/setup.sh`:
- Around line 78-87: The current AVD existence check using grep "Name:
$AVD_NAME" can match substrings and yield false positives; update the check
around avdmanager list avd to perform an exact line match for the AVD name
(e.g., match the "Name:" line exactly for AVD_NAME using start/end anchors and
optional whitespace) so only the precise AVD name is detected; modify the grep
invocation used with avdmanager list avd and the AVD_NAME variable to use a
regex like ^Name:[[:space:]]*<AVD_NAME>$ (or an equivalent exact-match approach)
to avoid partial matches.
In `@python/packages/jumpstarter-driver-adb/.gitignore`:
- Around line 1-3: The .gitignore file includes standard patterns (__pycache__/,
.coverage, coverage.xml) but lacks a trailing newline; open the file named
.gitignore in the python/packages/jumpstarter-driver-adb package and add a
single trailing newline at EOF so the file ends with a newline character to
satisfy POSIX conventions while keeping the existing ignore patterns unchanged.
In `@python/packages/jumpstarter-driver-adb/examples/exporter.yaml`:
- Around line 1-19: Add a POSIX-compliant trailing newline to the YAML example
for the ExporterConfig named "adb-local" (apiVersion jumpstarter.dev/v1alpha1,
kind ExporterConfig) by ensuring the file ends with a single newline character
after the final "port: 15037" line; this is a purely formatting change to the
examples/exporter.yaml content so text tools and linters recognize it as a POSIX
text file.
In `@python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/client.py`:
- Around line 27-50: The tunnel state file read/write/remove functions
(_read_tunnel_state, _write_tunnel_state, _remove_tunnel_state) are vulnerable
to races; fix them by using atomic file writes and OS-level file locks: when
writing in _write_tunnel_state, write to a temp file and atomically replace
_TUNNEL_STATE_FILE (os.replace) and acquire an exclusive lock (fcntl.flock or a
cross-platform locker) while writing; when reading in _read_tunnel_state,
acquire a shared lock before opening/parsing the JSON and before verifying the
pid to ensure consistency; similarly acquire an exclusive lock in
_remove_tunnel_state before unlinking. Ensure locks are released in finally
blocks and keep JSON port stored as int.
In
`@python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver_test.py`:
- Around line 32-39: Add a new pytest that asserts AdbServer rejects non-integer
ports: create a test function (e.g., test_invalid_port_non_integer) that calls
AdbServer(port="not-an-int") (or port=1.5) inside with
pytest.raises(ConfigurationError) to cover the type-check branch in the
AdbServer constructor/driver.py validation logic.
In `@python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver.py`:
- Around line 90-107: The kill_server method calls subprocess.run without a
timeout which can block; add a timeout argument (consistent with the timeout
used in start_server) to the subprocess.run call inside kill_server and handle
subprocess.TimeoutExpired by catching it (in addition to
subprocess.CalledProcessError), logging an appropriate error via
self.logger.error (including the timeout info and self.port), and returning
self.port as before; update the subprocess.run invocation in kill_server (which
uses self.adb_path, "kill-server", env=self._adb_env()) to include the timeout
and ensure stderr/stdout handling remains unchanged.
- Around line 69-88: The start_server method calls subprocess.run without a
timeout which can hang; update the call in start_server to pass a reasonable
timeout (e.g., timeout=30) and add an except block for subprocess.TimeoutExpired
to log the timeout (using self.logger.error or self.logger.warning) and handle
cleanup if needed, then return the port as before; reference the subprocess.run
call in start_server and the subprocess.TimeoutExpired exception to implement
the new timeout and error handling.
In `@python/packages/jumpstarter-driver-androidemulator/.gitignore`:
- Around line 1-3: The .gitignore contents correctly list __pycache__/,
.coverage, and coverage.xml but the file lacks a trailing newline; open the file
(python/packages/jumpstarter-driver-androidemulator/.gitignore) and ensure you
add a single POSIX-compliant newline character at the end of the file so the
last entry (coverage.xml) is terminated by a newline.
In `@python/packages/jumpstarter-driver-androidemulator/examples/exporter.yaml`:
- Line 13: Update the example to make the emulator run headlessly by default:
change the headless setting in exporter.yaml from false to true (the `headless:`
key), and add a short note in the example/comments explaining how to enable GUI
mode when needed (e.g., set `headless: false` or provide DISPLAY/Xvfb
instructions) so CI/remote Linux users won’t fail on systems without a display
server.
In
`@python/packages/jumpstarter-driver-androidemulator/examples/local-exporter.yaml`:
- Around line 1-15: Add a POSIX-compliant trailing newline to the YAML file by
ensuring the final line of the ExporterConfig (resource name "android-emulator"
/ kind ExporterConfig) ends with a newline character; simply update the file to
include a single newline at EOF so the file ends with a blank line.
In
`@python/packages/jumpstarter-driver-androidemulator/jumpstarter_driver_androidemulator/client.py`:
- Around line 41-54: The _wait_for_boot method's adb parameter lacks a type
hint; update the signature of _wait_for_boot to annotate adb (e.g., adb: Any or
a string forward-reference like 'adbutils.AdbClient') and add the corresponding
import from typing (Any) at the top of the file so static checkers and readers
know this is an external/optional dependency; ensure the rest of the function
uses the same parameter name (_wait_for_boot(self, adb: Any, timeout: int = 180)
-> None).
In
`@python/packages/jumpstarter-driver-androidemulator/jumpstarter_driver_androidemulator/driver.py`:
- Around line 154-162: The off() method currently calls shutil.which("adb")
locally; change it to reuse the already-resolved ADB path from the parent's
AdbServer instance (e.g. use self.parent.adb_server.adb_path) when constructing
the subprocess.run call, keeping the same env override for
"ANDROID_ADB_SERVER_PORT" and the same arguments (including
f"emulator-{self.parent.console_port}") so ADB invocation is consistent with the
rest of the codebase.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bcc1f39e-ac4c-4741-aa5c-941074321a4a
⛔ Files ignored due to path filters (1)
python/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
python/docs/source/reference/package-apis/drivers/adb.mdpython/docs/source/reference/package-apis/drivers/androidemulator.mdpython/docs/source/reference/package-apis/drivers/index.mdpython/examples/android-emulator/README.mdpython/examples/android-emulator/jumpstarter_example_android_emulator/__init__.pypython/examples/android-emulator/jumpstarter_example_android_emulator/conftest.pypython/examples/android-emulator/jumpstarter_example_android_emulator/test_android_emulator.pypython/examples/android-emulator/pyproject.tomlpython/examples/android-emulator/setup.shpython/packages/jumpstarter-driver-adb/.gitignorepython/packages/jumpstarter-driver-adb/README.mdpython/packages/jumpstarter-driver-adb/examples/exporter.yamlpython/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/__init__.pypython/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/client.pypython/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver.pypython/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver_test.pypython/packages/jumpstarter-driver-adb/pyproject.tomlpython/packages/jumpstarter-driver-androidemulator/.gitignorepython/packages/jumpstarter-driver-androidemulator/README.mdpython/packages/jumpstarter-driver-androidemulator/examples/exporter.yamlpython/packages/jumpstarter-driver-androidemulator/examples/local-exporter.yamlpython/packages/jumpstarter-driver-androidemulator/jumpstarter_driver_androidemulator/__init__.pypython/packages/jumpstarter-driver-androidemulator/jumpstarter_driver_androidemulator/client.pypython/packages/jumpstarter-driver-androidemulator/jumpstarter_driver_androidemulator/driver.pypython/packages/jumpstarter-driver-androidemulator/jumpstarter_driver_androidemulator/driver_test.pypython/packages/jumpstarter-driver-androidemulator/pyproject.tomlpython/pyproject.toml
...ackages/jumpstarter-driver-androidemulator/jumpstarter_driver_androidemulator/driver_test.py
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
python/packages/jumpstarter-driver-androidemulator/jumpstarter_driver_androidemulator/driver_test.py (3)
32-37: Consider adding test coverage foradb_server_portvalidation.The driver validates both
console_portandadb_server_portin the same loop (per context snippet 2), but onlyconsole_portvalidation is tested here. Adding a test for invalidadb_server_port(e.g.,-1or65536) would improve coverage.📝 Example additional test
`@patch`("jumpstarter_driver_androidemulator.driver.shutil.which", return_value="/usr/bin/emulator") `@patch`("jumpstarter_driver_adb.driver.shutil.which", return_value="/usr/bin/adb") `@patch`("subprocess.run", return_value=_mock_adb_ok()) def test_init_invalid_adb_server_port(mock_run, mock_adb_which, mock_emu_which): with pytest.raises(ConfigurationError, match="Invalid adb_server_port"): AndroidEmulator(avd_name="test_avd", adb_server_port=-1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-androidemulator/jumpstarter_driver_androidemulator/driver_test.py` around lines 32 - 37, Add a parallel test that asserts invalid adb_server_port values raise ConfigurationError: create a new test (mirroring test_init_invalid_port) that patches jumpstarter_driver_androidemulator.driver.shutil.which and jumpstarter_driver_adb.driver.shutil.which and subprocess.run, then calls AndroidEmulator(avd_name="test_avd", adb_server_port=-1) inside pytest.raises(ConfigurationError, match="Invalid adb_server_port"); this ensures the adb_server_port validation path in AndroidEmulator is covered.
15-15: Consider module-scoped patch forsubprocess.runfor consistency.The
shutil.whichpatches correctly target their respective modules, butsubprocess.runis patched globally. For consistency and to avoid potential issues in more complex test scenarios, consider usingjumpstarter_driver_adb.driver.subprocess.run(where AdbServer's__post_init__likely calls it for validation).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-androidemulator/jumpstarter_driver_androidemulator/driver_test.py` at line 15, The global patch of subprocess.run should be changed to a module-scoped patch so it mocks the call used by AdbServer; update any `@patch`("subprocess.run", ...) decorators in driver_test.py to target the driver module's subprocess (e.g., `@patch`("jumpstarter_driver_adb.driver.subprocess.run", return_value=_mock_adb_ok())) so that AdbServer.__post_init__ and other calls within jumpstarter_driver_adb.driver use the mocked subprocess.run; keep the same return_value and behavior but change only the patch target string.
115-115: Consider movingTimeoutExpiredimport to module level.The
from subprocess import TimeoutExpiredimport is placed inside the test function. Moving it to the module-level imports would follow standard Python conventions and make the import visible at a glance.📝 Suggested change
-from unittest.mock import MagicMock, patch +from subprocess import TimeoutExpired +from unittest.mock import MagicMock, patchThen remove the import from line 115.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-androidemulator/jumpstarter_driver_androidemulator/driver_test.py` at line 115, Move the `from subprocess import TimeoutExpired` import from inside the test function to the module-level imports at the top of driver_test.py and remove the in-function import; update the existing top import block to include `TimeoutExpired` so references in the test (the test that currently contains the inline import) use the module-level symbol instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@python/packages/jumpstarter-driver-androidemulator/jumpstarter_driver_androidemulator/driver_test.py`:
- Around line 32-37: Add a parallel test that asserts invalid adb_server_port
values raise ConfigurationError: create a new test (mirroring
test_init_invalid_port) that patches
jumpstarter_driver_androidemulator.driver.shutil.which and
jumpstarter_driver_adb.driver.shutil.which and subprocess.run, then calls
AndroidEmulator(avd_name="test_avd", adb_server_port=-1) inside
pytest.raises(ConfigurationError, match="Invalid adb_server_port"); this ensures
the adb_server_port validation path in AndroidEmulator is covered.
- Line 15: The global patch of subprocess.run should be changed to a
module-scoped patch so it mocks the call used by AdbServer; update any
`@patch`("subprocess.run", ...) decorators in driver_test.py to target the driver
module's subprocess (e.g.,
`@patch`("jumpstarter_driver_adb.driver.subprocess.run",
return_value=_mock_adb_ok())) so that AdbServer.__post_init__ and other calls
within jumpstarter_driver_adb.driver use the mocked subprocess.run; keep the
same return_value and behavior but change only the patch target string.
- Line 115: Move the `from subprocess import TimeoutExpired` import from inside
the test function to the module-level imports at the top of driver_test.py and
remove the in-function import; update the existing top import block to include
`TimeoutExpired` so references in the test (the test that currently contains the
inline import) use the module-level symbol instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c3fd439e-2878-4564-9719-215f49252759
📒 Files selected for processing (3)
python/packages/jumpstarter-driver-adb/README.mdpython/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/client.pypython/packages/jumpstarter-driver-androidemulator/jumpstarter_driver_androidemulator/driver_test.py
mangelajo
reviewed
Apr 6, 2026
| ] | ||
|
|
||
| [project.optional-dependencies] | ||
| python-api = ["adbutils>=2.8.7"] |
Member
There was a problem hiding this comment.
I would perhaps even make it mandatory unless adbutils is too huge
mangelajo
approved these changes
Apr 6, 2026
Member
mangelajo
left a comment
There was a problem hiding this comment.
It looks great, let's bump that dependency to mandatory in a later PR :)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request adds basic support for Android devices through the
jumpstarter-driver-adbandjumpstarter-driver-androidemulator. I have also added an example underpython/examples/android-emulatorthat shows how we can utilize these drivers together to demonstrate Android support.The ADB driver is indented to be used in more complex Android testing tools such as Android Studio, Trade Federation (tradefed), or Appium to prepare the remote target device so those tools can behave as if the device is connected physically. In the future, we might be able to offer deeper integration with these tools.
The Android Emulator power driver provides the ability to launch an Android Virtual Device (AVD) just like the QEMU driver does for QEMU virtual machines. This enables us to write basic tests against Android devices for demonstration purposes.