Skip to content

feat: add --install-only and --pick-device to Appium runners - #65

Merged
fadi-george merged 5 commits into
mainfrom
fadi/add-install-and-devices-flags
Oct 29, 2026
Merged

fadi-george merged 5 commits into
mainfrom
fadi/add-install-and-devices-flags

Conversation

@fadi-george

Copy link
Copy Markdown
Contributor

Description

Adds --install-only to run-local.sh and --pick-device to run-local.sh and run-all.sh, so you can install a demo app without running tests and choose which simulator or emulator to use.

Details

  • --install-only: builds, launches the device, resets, and installs the app (adb / simctl / devicectl), then exits without starting Appium or running tests. It skips the Appium/vpx checks. With --skip-build it reinstalls the existing artifact. It's not in run-all.sh because every demo has the bundle ID com.onesignal.example, so each install would overwrite the last.
  • --pick-device: shows a numbered list of iOS simulators (newest iOS first, iPhones first) or Android AVDs, with booted/running ones marked. run-local.sh asks after the platform is chosen. run-all.sh asks once up front, only for platforms that have SDKs to run (e.g. --sdks=ios only asks for a simulator), and forwards --device (plus --udid on iOS) to each run.
  • --device=NAME now overrides AVD_NAME / IOS_SIMULATOR from .env.
  • OS_VERSION is reset to the running device's OS version, so Appium's platformVersion check passes on a non-default device.
  • No breaking changes. Default runs are unchanged.

Testing: bash -n and vp check pass. I tested the picker against local simulators and AVDs, tested the iOS OS version lookup, and tested run-all.sh asking only for the relevant platforms with --sdks=ios, android, and ios,flutter. Not yet tested: a full test run on a picked device, and --install-only on a device.

🤖 Generated with Claude Code

fadi-george and others added 2 commits October 29, 2026 10:44
Builds, launches the device, resets, and installs the app, then exits
without starting Appium or running tests.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Prompts for an iOS simulator and/or Android AVD in run-local.sh and,
up front, in run-all.sh. Also lets --device override .env device names
and syncs OS_VERSION with the running device.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@fadi-george
fadi-george requested a review from a team as a code owner October 29, 2026 18:23
abdulraqeeb33
abdulraqeeb33 previously approved these changes Oct 29, 2026

@abdulraqeeb33 abdulraqeeb33 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice addition, both flags are genuinely useful and the implementation follows the existing runner conventions closely. Two things worth a look before merge, neither blocking.

Also three minor notes:

  • run-local/runtime.sh:409 — a failed version lookup and "version already matches" are both a silent return 0. When adb getprop times out or the simctl/python read fails, version is empty, the regex fails, and OS_VERSION quietly stays at the default. That is the exact condition the function exists to prevent, and it surfaces later as a confusing Appium platformVersion mismatch rather than "couldn't read the device OS version". Worth splitting the empty-lookup case out with a warn.
  • README.md:3 — the intro still reads "builds the app, boots a simulator/emulator, starts Appium, resets app state, and runs the E2E test suite." With --install-only that is no longer the only path. The options table covers the flag, just the lead sentence that is stale.
  • README.md:138-148 — the run-all.sh section does not mention --pick-device, though run-all.sh --help documents it.

Comment thread appium/scripts/run-local/config.sh Resolved

@sherwinski sherwinski left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments and potential issues to consider.

Comment thread appium/scripts/run-all.sh Resolved
Comment thread appium/scripts/run-local/runtime.sh Resolved
Comment thread appium/scripts/run-local/runtime.sh Resolved
Comment thread appium/scripts/run-local/runtime.sh Resolved
Comment thread appium/scripts/run-all.sh Resolved
Comment thread appium/scripts/run-local/runtime.sh Resolved
Comment thread appium/scripts/run-all.sh Resolved
- Fail the device picker on EOF instead of looping
- Make --device fully override .env on iOS (UDID, runtime)
- Forward --install-only from run-all, one SDK per platform
- Resolve the booted simulator instead of simctl's `booted` target
- Warn when the device OS version can't be read
- Extract ios_sim_version and combo_runs helpers

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@fadi-george

Copy link
Copy Markdown
Contributor Author

Thanks @abdulraqeeb33. Addressed the three notes in d99ae07: empty version lookup now warns, README intro mentions --install-only, and the run-all section shows --pick-device and --install-only.

sherwinski
sherwinski previously approved these changes Oct 29, 2026

@sherwinski sherwinski left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that the second fallback block in run-local/runtime.sh:82 still splices $IOS_SIMULATOR into Python source inside single quotes. The same quote-in-value failure applies as in #65 (comment). That code is outside the scope of this PR so probably worth addressing in a separate PR, or just make the update here.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@fadi-george

Copy link
Copy Markdown
Contributor Author

Thanks @sherwinski, fixed the fallback lookup here in 3897959, simulator name is passed as argv now.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@fadi-george
fadi-george merged commit 6ff5cef into main Oct 29, 2026
@fadi-george
fadi-george deleted the fadi/add-install-and-devices-flags branch October 29, 2026 22:51

@abdulraqeeb33 abdulraqeeb33 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at a8547f8. My earlier pass was against 328f6c3, so this supersedes it.

Approving. The three commits since then fix the serious half of what I had, including the one finding that would have bitten in CI.

Fixed since 328f6c3

  • The EOF busy-loop is gone. read failure now returns 1 instead of spinning. This was the one I'd have blocked on: both call sites run the function in a || / if ! context, which suppresses errexit for the whole body, so run-local.sh's set -euo pipefail wasn't saving it either.
  • --device= now clears IOS_RUNTIME and start_ios_simulator sorts runtimes newest-first, so an explicitly named device is no longer silently replaced by whichever simulator happens to be booted. Nice that the empty-filter-matches-any approach avoids the trap where clearing the runtime to '' would have matched everything unsorted.
  • ${UDID:-booted} is gone from the install path. resolve_booted_simulator pins the target on --skip-device, and install_app now hard-errors rather than inferring. The 0-booted and multiple-booted cases both get a real message.
  • $UDID and $IOS_SIMULATOR are passed via argv instead of interpolated into Python string literals.
  • The OS-version sync warns instead of silently keeping a stale value, which was the whole point of the function.
  • run-all.sh parses --install-only and guards against installing several SDKs over one bundle id, instead of warning "Unknown option" and then running the full matrix.

I checked the new code rather than taking the commit messages for it. The [[ "$PLATFORM" == "ios" ]] && resolve_booted_simulator line in start_device is errexit-safe under set -euo pipefail (tested, a non-final && component failing doesn't trip it), and run-all.sh's error() only echoes, so the exit 2 after it is load-bearing rather than dead. combo_runs is a good extraction; my truth table on the old inline form said it was correct, and the named version is much easier to confirm.

Still open, none blocking

The vpx one is inline and is the only one I'd prioritize. Three things that don't anchor to a diff line:

No test harness, and that's load-bearing here. No bats, no shellcheck, no bash -n anywhere in .github/workflows, across roughly 2,500 lines of shell that CI never invokes. Worth noting bash -n wouldn't have caught the EOF loop, the sort, or the UDID quoting, so a lint step alone isn't the fix. The seam: list_ios_simulators / list_android_avds that only print udid|name|label to stdout, and a choose_from_list taking those lines plus a prompt, with the simctl transforms as programs reading JSON on stdin and taking arguments via argv. Now that ios_sim_version already takes its UDID that way, it's close to directly testable with fixture JSON. combo_runs is a free one.

The README environment table doesn't reflect the new precedence. It still presents DEVICE, IOS_SIMULATOR, IOS_RUNTIME, AVD_NAME, and OS_VERSION as the source of truth, with no note that --device= now clears the middle three or that the OS version is re-read from the running device. That table is outside the diff, so I couldn't anchor it.

config.sh is at 466 lines and runtime.sh at 492. Moving preflight / preflight_test_tooling into a run-local/preflight.sh, sibling to device-picker.sh, and the USAGE heredoc into its own file, would take roughly 170 lines out of config.sh and leave it as parse plus resolve.

Residual risk

Still nothing run end-to-end against a device, so the two paths you flagged stay unverified, and resolve_booted_simulator is new since the last pass. Specifically open: whether xcrun simctl install against a shutdown simulator succeeds, which decides what --pick-device --skip-device actually does on iOS now that it pins a UDID and then never boots it.

# calls `vpx wdio run` directly on BrowserStack — so these checks are
# local-only by construction.
preflight() {
if [[ "$INSTALL_ONLY" != true ]]; then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still skips all of preflight_test_tooling for --install-only, and that includes the vpx check, which install-only does need.

build.sh calls vp run build and vpx cap sync for Cordova, Capacitor, and Expo. So ./run-local.sh --install-only --sdk=capacitor without the vpx symlink spends the entire web build and then dies on a bare vpx: command not found, which is the failure the comment just above says this block exists to prevent. vp can exist while the vpx symlink doesn't, and that's exactly the case the error message in preflight_test_tooling is written for.

Appium, the driver check, the Appium node_modules bootstrap, and the Node-version WDIO_USE_NATIVE_FETCH export are all genuinely test-only and right to skip. vpx is a build dependency, so the split is one line off.

Suggestion: keep the vpx block unconditional and gate the rest behind [[ "$INSTALL_ONLY" != true ]].

error "--release and --main are mutually exclusive"
fi

if [[ "$PICK_DEVICE" == true ]]; then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small gap left over: this block unconditionally overwrites DEVICE, AVD_NAME, IOS_SIMULATOR, and UDID, so --pick-device --device=X and --device=X --pick-device both end on the picked device and the explicit --device= is dropped with no warning. Erroring on the combination would match the --device-real rejection on the line below.

Minor follow-on from the new DEVICE_FLAG handling: when both are passed, IOS_RUNTIME_ANY stays true from the --device= branch even though the picker supplies a UDID. Harmless today, since a non-empty UDID takes the early branch in start_ios_simulator and never consults the runtime, but it's the kind of leftover that stops being harmless if that early branch ever changes.

if d["state"] == "Booted":
label += " [booted]"
rows.append((ver, d["name"], d["udid"], label))
for ver, name, udid, label in sorted(rows, key=lambda r: (tuple(-v for v in r[0]), not r[1].startswith("iPhone"), r[1])):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Newest-first breaks when runtime ids differ in length, because (-18, 0) sorts before (-18, 0, -1). I confirmed 18.0 lists above 18.0.1. Equal-length ids sort correctly, so this needs a three-component runtime installed to show up, and live ids are usually two components.

Padding before negating fixes it, e.g. ver + (0,) * (4 - len(ver)).

Worth noting start_ios_simulator's new version() helper has the same shape, though it's only used to order candidates for an exact name match, so the consequence there is smaller.

label += " [booted]"
rows.append((ver, d["name"], d["udid"], label))
for ver, name, udid, label in sorted(rows, key=lambda r: (tuple(-v for v in r[0]), not r[1].startswith("iPhone"), r[1])):
print(udid + "|" + name + "|" + label)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: | as the field separator breaks on a simulator renamed to contain one. iPhone | QA reads back as name iPhone , and boot plus the Appium capability then use the truncated name. UDIDs from simctl never contain |, so only the name is at risk.

A tab or \x1f separator, or printing just udid and name and rebuilding the label in bash, would close it.

info "Installed $BUNDLE_ID"
}

prepare_install_only() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two sequences have now actually diverged, which is the thing I was worried about a pass ago: sync_device_os_version was dropped from here but kept in prepare_runtime. That's the right call on the merits, since nothing downstream of install-only reads OS_VERSION, but nothing in the code records that it was a decision rather than an omission, and the next step added to prepare_runtime has the same coin-flip.

Suggestion: extract the shared head (validate_existing_app, start_device, check_android_storage, reset_app) into one prepare_device, and keep the version sync, test tooling, and explicit install as mode-specific tails. Then the difference between the two paths is visible in one place instead of inferred from two lists.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants