feat(appium): add Unity SDK support#35
Conversation
|
@claude review |
|
@claude[agent] review |
|
@claude review |
Inside build_unity_ios, the inner 'local stamp' for the Podfile.lock cache was shadowing the outer 'local stamp' for the Unity build cache. The trailing 'echo "$demo_hash" > "$stamp"' would then write the demo hash into .podfile.lock.stamp instead of .unity-build-ios.stamp, defeating the Unity build cache and corrupting the pod stamp. Also fix the --help text to reference Unity 6000.4.6f1 to match the actual default. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
The Proxy returned by withElementInteractionFixes forwarded `then` to its target, which is a ChainablePromiseElement. Because the Proxy was itself thenable, awaiting byTestId/byText adopted the thenable and unwrapped past the Proxy down to the raw Element, silently bypassing the Unity center-tap shim and the Flutter Android getText/setValue fallbacks. Resolve the chainable first so the Proxy wraps a real Element. Co-authored-by: Cursor <cursoragent@cursor.com>
openModal() already awaits waitForExist on the expected element before returning, so the extra wait was dead. Brings 09_trigger in line with the other migrated specs. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Previously the case statement's *.ts arm short-circuited normalization, so --spec=03_iam.spec.ts was passed verbatim to wdio and resolved against APPIUM_DIR instead of tests/specs/, producing 'No specs found'. Strip the optional .spec.ts/.ts suffix before re-prefixing so all three forms (03_iam, 03_iam.ts, 03_iam.spec.ts) resolve identically. Fully-qualified paths and explicit globs still bypass normalization. Co-authored-by: Cursor <cursoragent@cursor.com>
The y >= threshold && y + height <= viewport - threshold predicate (threshold = 12% of viewport) is unsatisfiable whenever the element itself exceeds 76% of the viewport — exactly the case for populated section containers like triggers_section / aliases_section. That burned the full 3-iteration loop on every call. Accept tall elements as-is and fix the misleading catch-block comment, which referenced 'original ref' even though current is reassigned each miss. Co-authored-by: Cursor <cursoragent@cursor.com>
The retry-era name no longer matches the simplified single-wait body and is inconsistent with confirmModal / waitForDisappear / waitForStablePosition. Co-authored-by: Cursor <cursoragent@cursor.com>
The previous broad pkill -f patterns matched any WebDriverAgent or xcodebuild process on the host, so a developer's unrelated WDA session or parallel Xcode UI test would get SIGKILLed. Switch to lsof on port 8100 (WDA's hardcoded default in this repo) so we only kill the specific port-holder. Also move the cleanup after the Appium health check, so a healthy reused Appium session keeps its WDA child. Co-authored-by: Cursor <cursoragent@cursor.com>
…ents Both gates apply to Unity on both platforms but the comments described iOS-only XCUITest behavior. Update to reflect that the same gate fires on Unity Android for different reasons (UiScrollable doesn't traverse into the Unity SurfaceView). Co-authored-by: Cursor <cursoragent@cursor.com>
Previous default of x86_64 forced Apple Silicon hosts through Rosetta on every Unity iOS build and test launch. Mirror the .NET branch's uname -m pickup so arm64 hosts get the native slice. UNITY_IOS_SIM_ARCH remains an override for the rare cross-arch case. Co-authored-by: Cursor <cursoragent@cursor.com>
| case "$SPEC" in | ||
| */*|*\**|*\?*) ;; | ||
| *) | ||
| base="${SPEC%.spec.ts}" | ||
| base="${base%.ts}" | ||
| SPEC="tests/specs/${base}*.spec.ts" | ||
| ;; | ||
| esac |
There was a problem hiding this comment.
🔴 Empty SPEC normalization (line 118-125) rewrites the default SPEC="" to tests/specs/*.spec.ts, which then satisfies the if [[ -n "$SPEC" ]] check in run_tests (lines 1591-1593) and gets forwarded to wdio. This silently defeats the grouped-spec optimization the comment at lines 1587-1589 explicitly warns about, costing ~2-6 min of extra iOS session-setup overhead on every no-flag invocation. Gate the case with if [[ -n "$SPEC" ]] or add a "") ;; arm.
Extended reasoning...
What the bug is
SPEC defaults to "" at line 31, and the new --spec normalization at lines 118-125 runs unconditionally on it:
case "$SPEC" in
*/*|*\**|*\?*) ;;
*)
base="${SPEC%.spec.ts}"
base="${base%.ts}"
SPEC="tests/specs/${base}*.spec.ts"
;;
esacThe empty string contains no /, no *, and no ?, so it matches none of the early-exit alternatives and falls into the default arm. Both ${SPEC%.spec.ts} and ${base%.ts} leave base empty, and the final assignment produces SPEC="tests/specs/*.spec.ts".
Why this defeats the explicit optimization
run_tests at lines 1581-1596 reads:
# Only forward --spec when the user explicitly overrode it. Passing --spec
# to wdio re-expands the glob into one runner per file (~10-30s of session
# setup each on iOS) and bypasses the grouped specs in the conf.
local -a wdio_args=("$conf")
if [[ -n "$SPEC" ]]; then
info "Running tests (conf: $conf, spec: $SPEC)..."
wdio_args+=(--spec "$SPEC")
else
info "Running tests (conf: $conf, spec: <conf default>)..."
fiAfter the normalization, SPEC is non-empty for every no-flag invocation, so the [[ -n "$SPEC" ]] branch always wins and --spec tests/specs/*.spec.ts is forwarded to wdio. That is the exact scenario the comment was written to prevent. The grouped-spec configuration in wdio.ios.conf.ts / wdio.android.conf.ts becomes unreachable on the default path, and wdio.shared.conf.ts lines 37-38 explicitly pre-expand the glob into a single array element with a matching comment: "Pre-expand the glob so WDIO groups all specs into one runner and reuses a single Appium session (a raw glob or --spec flag spawns one runner per file, costing ~10-30s of iOS session setup each)."
Step-by-step proof
- User runs
./scripts/run-local.sh --platform=ios --sdk=unity(no--specflag). - Line 31:
SPEC="". No CLI override. - Lines 118-125: case statement runs. Empty string matches none of
*/*,*\**,*\?*(each requires the literal char to appear), so falls into default arm. base="${SPEC%.spec.ts}"→base="".base="${base%.ts}"→base="".SPEC="tests/specs/${base}*.spec.ts"→SPEC="tests/specs/*.spec.ts".- Later in
run_tests: line 1591[[ -n "$SPEC" ]]→ true.wdio_args+=(--spec "tests/specs/*.spec.ts"). - wdio re-expands the glob and spawns one runner per spec file (~12 specs on iOS × ~10-30s session setup = ~2-6 min wasted per run).
Verified locally: bash -c 'SPEC=""; case "$SPEC" in */*|*\**|*\?*) ;; *) base="${SPEC%.spec.ts}"; base="${base%.ts}"; SPEC="tests/specs/${base}*.spec.ts" ;; esac; echo "$SPEC"' prints tests/specs/*.spec.ts.
Impact
Every no-flag invocation — by far the most common path — now incurs ~2-6 min of extra iOS session-setup overhead and the grouped-spec configuration in the conf files becomes dead code. Functional tests still pass; this is a perf regression, but a measurable one that directly contradicts the explicit intent in the same area of code (the warning comment lives just five lines above the forwarding code).
Fix
Either gate the case on a non-empty SPEC:
if [[ -n "$SPEC" ]]; then
case "$SPEC" in
*/*|*\**|*\?*) ;;
*)
base="${SPEC%.spec.ts}"
base="${base%.ts}"
SPEC="tests/specs/${base}*.spec.ts"
;;
esac
fiOr add an empty-string arm to the case:
case "$SPEC" in
""|*/*|*\**|*\?*) ;;
*)
base="${SPEC%.spec.ts}"
base="${base%.ts}"
SPEC="tests/specs/${base}*.spec.ts"
;;
esacEither way the no-flag path keeps SPEC empty so run_tests falls through to the grouped specs in the conf, restoring the optimization the comment promises.
| scrollToEl, | ||
| waitForAppReady, | ||
| } from '../helpers/app'; | ||
| import { checkInAppMessage, checkTooltip, scrollToEl, waitForAppReady } from '../helpers/app'; |
There was a problem hiding this comment.
🔴 The 'can pause iam' test calls isWebViewVisible() on lines 41 and 51, but the symbol is no longer in the named imports on line 1 (only checkInAppMessage, checkTooltip, scrollToEl, waitForAppReady are imported). This causes a TypeScript compile error (TS2304) and a runtime ReferenceError, breaking the entire spec. Add isWebViewVisible back to the import list.
Extended reasoning...
What the bug is
appium/tests/specs/03_iam.spec.ts at HEAD has:
import { checkInAppMessage, checkTooltip, scrollToEl, waitForAppReady } from '../helpers/app';But the body of the 'can pause iam' test uses isWebViewVisible() twice:
- Line 41:
await driver.waitUntil(async () => !(await isWebViewVisible()), { ... })— drains a lingering IAM context before asserting that pause is effective. - Line 51:
expect(await isWebViewVisible()).toBe(false);— the actual pause-effective assertion.
The PR description says 'Remove unused isWebViewVisible import', which was correct at the time the import was removed. A subsequent edit re-introduced the pause-effective assertion (and a drain check) without restoring the import.
Why existing code doesn't prevent it
isWebViewVisible is exported from appium/tests/helpers/app.ts (export async function isWebViewVisible() at app.ts:869), so the symbol exists in the module graph. The spec file just never pulls it in. There is no fallback / global definition: TypeScript will flag this as TS2304 (Cannot find name 'isWebViewVisible') and Node/tsx will throw a ReferenceError at runtime as soon as the test function executes.
Code path that triggers it
wdio run wdio.ios.conf.tsloadstests/specs/03_iam.spec.tsvia the tsx loader.- tsx type-checks the file (or at minimum module-resolves it). TS2304 fires on lines 41 and 51 — the build fails before any test runs.
- Even if type checking is skipped (transpile-only mode), at runtime the
'can pause iam'test callsisWebViewVisible(), hits a ReferenceError, and Mocha records the test as failed.
Impact
Breaks the entire 03_iam.spec.ts spec on every run for every SDK/platform combination. The PR check that runs this file will fail. Other tests in the same describe block ('should show correct tooltip info', the four IAM-type cases) may still execute, but the 'can pause iam' test will always fail.
Step-by-step proof
Take any SDK/platform pair where this spec runs (e.g. Unity iOS):
- Test runner starts
it('can pause iam', ...)(line 32). - Lines 33-38: scroll to and click the pause toggle.
- Line 41: JavaScript evaluates the arrow function.
isWebViewVisibleis not in scope (no import, no global, novar/let/constdeclaration).ReferenceError: isWebViewVisible is not definedis thrown. driver.waitUntilsurfaces the rejection. Mocha marks the test failed.- Even if line 41 somehow passed, line 51 would throw the same ReferenceError.
Fix
One-line change at line 1:
-import { checkInAppMessage, checkTooltip, scrollToEl, waitForAppReady } from '../helpers/app';
+import {
+ checkInAppMessage,
+ checkTooltip,
+ isWebViewVisible,
+ scrollToEl,
+ waitForAppReady,
+} from '../helpers/app';No other changes needed — isWebViewVisible is already exported from ../helpers/app.
Co-authored-by: Cursor <cursoragent@cursor.com>
Description
Adds Unity SDK support to the Appium test suite alongside a series of fixes and refactors that make the cross-platform test runs more reliable.
Details
Unity support
*.java)6000.4.6f1and deduplicatexcodebuildinvocationsisUnitySDKconstant for clarityHelpers and reliability
openModalhelper; simplify it and drop the Unity-specific retrywaitForDisappearhelper (renamed fromwaitForTestIdNotDisplayed)isWebViewVisibleimportDependencies
vite-plusto0.1.20Scope
Changes are limited to the
appium/directory: helpers, selectors, specs, the local run script, and dependency manifests. No production SDK code is touched.Made with Cursor