test: [SDK-4333] expand Appium E2E test coverage#29
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a large test infrastructure PR — the change from autoAcceptAlerts: true to false in the iOS config is a behavioral change that affects all specs, and it's worth a human confirming the alert-handling migration in waitForAppReady is complete.
Extended reasoning...
Overview
The PR adds 5 new Appium spec files (outcomes, triggers, custom events, location, live activities), renames existing specs to zero-padded filenames, replaces UI-based log assertions with native device logs (syslog/logcat), and adds several new helper utilities.
Security risks
No security-sensitive code is touched. Changes are confined to test infrastructure and a demo tooltip JSON file.
Level of scrutiny
This warrants human review. The most impactful change is disabling autoAcceptAlerts in wdio.ios.conf.ts — previously alerts were silently accepted; now they must be explicitly handled. The new waitForAppReady logic handles a first-time alert via shared store, but it is unclear whether all specs that previously depended on auto-acceptance (e.g. push permission prompts in specs 02-07) are covered by this one-time guard or whether any could now hang on an unhandled alert.
Other factors
The two bugs flagged by the automated review are both nits (stale README filenames, a typo in a test name) and would not block the PR on their own. The substantive concern is the alert-handling behavioral change in iOS config.
|
@claude review |
| const checkSharedButton = await scrollToEl('CHECK LOCATION SHARED', { by: 'text' }); | ||
| await checkSharedButton.click(); | ||
|
|
||
| let snackbar = await byText('Location shared: false'); | ||
| await snackbar.waitForDisplayed({ timeout: 5_000 }); | ||
| expect(snackbar).toBeDefined(); | ||
|
|
||
| // toggle location sharing on | ||
| const shareButton = await scrollToEl('Share device location', { by: 'text', partial: true }); | ||
| await shareButton.click(); | ||
|
|
||
| // verify it's now shared | ||
| await checkSharedButton.click(); | ||
| snackbar = await byText('Location shared: true'); | ||
| await snackbar.waitForDisplayed({ timeout: 5_000 }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🔴 In the can share location test, checkSharedButton is captured once and reused after an intervening scrollToEl('Share device location', ...) call that may scroll 'CHECK LOCATION SHARED' off-screen. In Flutter, elements scrolled out of the viewport are removed from the semantics/accessibility tree, invalidating their Appium element IDs. The second checkSharedButton.click() at line 41 would then throw a StaleElementReferenceException, masking whether the location-sharing toggle actually works. Fix: re-fetch checkSharedButton after the intermediate scroll before the second click.
Extended reasoning...
What the bug is and how it manifests
In the can share location test (11_location.spec.ts lines 29–44), the element reference checkSharedButton is obtained via scrollToEl('CHECK LOCATION SHARED', { by: 'text' }) and stored. It is then used twice — once before, and once after, an intervening scrollToEl('Share device location', { by: 'text', partial: true }) call. If the 'Share device location' toggle is below the 'CHECK LOCATION SHARED' button in the UI (a natural ordering: first you check, then you toggle), the second scrollToEl will scroll the view downward to bring the toggle into view, pushing 'CHECK LOCATION SHARED' above the viewport.
The specific code path that triggers it
const checkSharedButton = await scrollToEl('CHECK LOCATION SHARED', { by: 'text' });
await checkSharedButton.click(); // use #1 — OK
let snackbar = await byText('Location shared: false');
await snackbar.waitForDisplayed({ timeout: 5_000 });
const shareButton = await scrollToEl('Share device location', { by: 'text', partial: true });
await shareButton.click(); // scrollToEl may scroll view down
await checkSharedButton.click(); // use #2 — potentially staleWhy existing code does not prevent it
Flutter maintains a separate semantics (accessibility) tree distinct from the widget tree. Elements that scroll off-screen are pruned from this tree, and their Appium-assigned element IDs are no longer resolvable. WebdriverIO does not transparently retry .click() on a pre-resolved Element object with a stale elementId — it issues the W3C /element/{id}/click request directly using the cached handle. When Appium looks up that ID in the now-updated Flutter semantics tree and cannot find it, it throws a StaleElementReferenceException (or an element-not-interactable error depending on the driver version).
What the impact would be
The second assertion — verifying that the location sharing state is now true — fails with a spurious element staleness error rather than a meaningful test failure. This masks whether the 'Share device location' toggle actually works and makes the test unreliable in any device or emulator configuration where both elements do not fit on screen simultaneously.
How to fix it
Re-fetch checkSharedButton after the intermediate scroll:
// verify it's now shared — re-fetch to avoid stale reference after scroll
const checkSharedButton2 = await scrollToEl('CHECK LOCATION SHARED', { by: 'text' });
await checkSharedButton2.click();
snackbar = await byText('Location shared: true');
await snackbar.waitForDisplayed({ timeout: 5_000 });Step-by-step proof
- Test calls
scrollToEl('CHECK LOCATION SHARED', ...)→ resolves to element ID"abc123"in Flutter semantics tree; element is on screen. checkSharedButton.click()succeeds; snackbar showsLocation shared: false.scrollToEl('Share device location', ...)does not find the toggle on screen, so it callsswipeMainContent('down', 'normal'), scrolling the view down. 'CHECK LOCATION SHARED' moves above the viewport.- Flutter removes 'CHECK LOCATION SHARED' from its semantics tree; element ID
"abc123"is no longer valid. shareButton.click()toggles location sharing on.checkSharedButton.click()issuesPOST /element/abc123/click— Appium cannot find"abc123"in the live accessibility tree and throwsStaleElementReferenceException.- The test fails at step 6, not at the actual assertion, making it impossible to tell whether location sharing was toggled correctly.
| async function checkActivity(options: { orderId?: string; status: string; message: string }) { | ||
| const { orderId = 'ORD-1234', status, message } = options; | ||
|
|
||
| // lock screen Lock Screen with Live Activity | ||
| await lockScreen(); | ||
|
|
||
| const statusEl = await $(`-ios predicate string:label CONTAINS "${status}"`); | ||
| await statusEl.waitForDisplayed({ timeout: 10_000 }); | ||
|
|
||
| const messageEl = await $(`-ios predicate string:label CONTAINS "${message}"`); | ||
| expect(messageEl).toBeDisplayed(); | ||
|
|
||
| const orderEl = await $(`-ios predicate string:label CONTAINS "${orderId}"`); | ||
| expect(orderEl).toBeDisplayed(); | ||
|
|
||
| await returnToApp(); | ||
| } |
There was a problem hiding this comment.
🔴 In checkActivity() (12_activity.spec.ts lines 22–38), lockScreen() sets defaultActiveApplication to com.apple.springboard, but if statusEl.waitForDisplayed({ timeout: 10_000 }) throws a TimeoutError, returnToApp() is never called — leaving the Appium session permanently scoped to SpringBoard. Any subsequent driver commands within the same test (e.g. the second checkActivity() call and clickUpdateButton()) will query SpringBoard's accessibility tree and fail with element-not-found errors. Fix: wrap all post-lock assertions in a try/finally block so returnToApp() is always called.
Extended reasoning...
What the bug is and how it manifests
In checkActivity() (12_activity.spec.ts lines 22–38), the function calls lockScreen(), which executes driver.updateSettings({ defaultActiveApplication: 'com.apple.springboard' }) — this switches the Appium element-lookup context from the demo app to the SpringBoard (lock screen) process. After locking, the function immediately calls await statusEl.waitForDisplayed({ timeout: 10_000 }). If the iOS Live Activity does not appear on the lock screen within 10 seconds (e.g., the SDK failed to start it silently, or there is a timing issue), WebdriverIO throws a TimeoutError. This exception propagates up through checkActivity() without ever reaching returnToApp().
The specific code path that triggers it
async function checkActivity(...) {
await lockScreen(); // sets defaultActiveApplication = 'com.apple.springboard'
const statusEl = await $(`-ios predicate string:label CONTAINS "${status}"`);
await statusEl.waitForDisplayed({ timeout: 10_000 }); // throws TimeoutError on timeout
// ... other assertions ...
await returnToApp(); // NEVER REACHED if waitForDisplayed throws
}returnToApp() is responsible for calling driver.updateSettings({ defaultActiveApplication: bundleId }) and driver.execute('mobile: activateApp', { bundleId }). Without it, the session context remains set to com.apple.springboard.
Why existing code doesn't prevent it
There is no try/finally guard around the lock-screen assertions. The lockScreen() + returnToApp() calls are paired sequentially in the happy path, but the error path has no cleanup. This is a well-known pitfall for resource-acquisition patterns: any interrupt between acquisition (locking) and release (returning to app) leaves the resource in an inconsistent state.
What the impact would be
In the single test 'can start a live, update, and exit activity', checkActivity() is called twice. If the first call fails on waitForDisplayed, the session stays scoped to SpringBoard. The clickUpdateButton('ON THE WAY') call that follows then queries SpringBoard for the update button element — finding nothing — and fails with an element-not-found error. The second checkActivity() call similarly fails. The actual root cause (Live Activity not showing within 10s) is obscured by a cascade of misleading element-not-found errors. While 12_activity.spec.ts is the last numbered spec file (limiting cross-file cascade), the within-test corruption is real and diagnostically confusing.
How to fix it
Wrap the assertions in a try/finally block so returnToApp() is always called:
async function checkActivity(options: { orderId?: string; status: string; message: string }) {
const { orderId = 'ORD-1234', status, message } = options;
await lockScreen();
try {
const statusEl = await $(`-ios predicate string:label CONTAINS "${status}"`);
await statusEl.waitForDisplayed({ timeout: 10_000 });
const messageEl = await $(`-ios predicate string:label CONTAINS "${message}"`);
expect(messageEl).toBeDisplayed();
const orderEl = await $(`-ios predicate string:label CONTAINS "${orderId}"`);
expect(orderEl).toBeDisplayed();
} finally {
await returnToApp();
}
}Step-by-step proof
- Test 'can start a live, update, and exit activity' runs and calls
checkActivity({ status: 'Preparing', ... }). lockScreen()executesdriver.updateSettings({ defaultActiveApplication: 'com.apple.springboard' })— Appium now queries SpringBoard.- The SDK fails to start the Live Activity (or takes >10s), so no element with label containing 'Preparing' appears.
statusEl.waitForDisplayed({ timeout: 10_000 })throwsTimeoutError: Element ... not displayed after 10000ms.- The error propagates out of
checkActivity();returnToApp()is skipped. - Mocha catches the error and marks this assertion as failed.
- Test execution continues with
clickUpdateButton('ON THE WAY'). This callsscrollToEl('UPDATE → ON THE WAY', { by: 'text' }), which callsbyText('UPDATE → ON THE WAY'), which resolves via-ios predicate string:label == "UPDATE → ON THE WAY". Appium executes this query against the SpringBoard accessibility tree — the button does not exist there — and after exhausting scroll attempts throwsError: Element "UPDATE → ON THE WAY" not found after 10 scrolls. - The same pattern repeats for every subsequent driver command in the test.
| const messageEl = await $(`-ios predicate string:label CONTAINS "${message}"`); | ||
| expect(messageEl).toBeDisplayed(); | ||
|
|
||
| const orderEl = await $(`-ios predicate string:label CONTAINS "${orderId}"`); | ||
| expect(orderEl).toBeDisplayed(); |
There was a problem hiding this comment.
🔴 In checkActivity() (12_activity.spec.ts lines 32 and 35), expect(messageEl).toBeDisplayed() and expect(orderEl).toBeDisplayed() are missing await. Since toBeDisplayed() in expect-webdriverio is an async matcher returning Promise<void>, these are floating promises that are never awaited — the test always passes regardless of whether the correct message or orderId are visible on the lock screen. Fix: add await before both assertions.
Extended reasoning...
What the bug is and how it manifests
In checkActivity() (appium/tests/specs/12_activity.spec.ts lines 32 and 35), two calls to the WebdriverIO async matcher toBeDisplayed() are made without await:
const messageEl = await $(`-ios predicate string:label CONTAINS "${message}"`);
expect(messageEl).toBeDisplayed(); // line 32 — floating promise
const orderEl = await $(`-ios predicate string:label CONTAINS "${orderId}"`);
expect(orderEl).toBeDisplayed(); // line 35 — floating promiseIn expect-webdriverio, toBeDisplayed() calls element.isDisplayed() internally and returns a Promise<void>. Without await, these are detached (floating) promises that are never awaited by the test runner.
The specific code path that triggers it
checkActivity() is an async function. After lines 32 and 35, it proceeds to await returnToApp(). Mocha tracks test outcomes through the promise returned by the async test function. The two floating promises are never part of that chain, so any rejection they produce is silently swallowed. Whether the live activity is showing the correct message/orderId or nothing at all, the test will pass.
Why existing code doesn't prevent it
TypeScript allows unawaited async function calls without a compile error. The @typescript-eslint/no-floating-promises ESLint rule would catch this, but it is not configured for this project's test files. There is also no runtime safeguard in Mocha for unhandled promise rejections in most default configurations.
What the impact would be
The 'can start a live, update, and exit activity' test never actually validates that the message ('Your order is being prepared', 'Driver is heading your way') and orderId ('ORD-1234') are visible on the lock screen. The test will pass even if the live activity shows wrong content, stale content, or nothing at all — giving a false sense of E2E coverage.
How to fix it
Add await before both assertions:
await expect(messageEl).toBeDisplayed();
await expect(orderEl).toBeDisplayed();Step-by-step proof
- The live activity is started, and
checkActivity({ status: 'Preparing', message: 'Your order is being prepared' })is called. lockScreen()runs; the test context switches to SpringBoard.statusEl.waitForDisplayed({ timeout: 10_000 })on line 29 is correctlyawaited — this passes.- Line 32:
expect(messageEl).toBeDisplayed()—toBeDisplayed()internally callselement.isDisplayed()asynchronously. The returnedPromiseis not awaited. If the element is not visible, the promise will eventually reject, but since it is unconnected to the test's promise chain, Mocha never sees the rejection. - Line 35: same pattern for
orderEl. await returnToApp()completes normally; the async function resolves successfully.- Mocha marks the test as passed, even though the message and orderId were never verified.
| const collectedLogs: string[] = []; | ||
|
|
||
| /** | ||
| * Get the level (info/warn/error) of a specific log entry. | ||
| */ | ||
| export async function getLogLevel(index: number): Promise<string> { | ||
| const levelEl = await byTestId(`log_entry_${index}_level`); | ||
| return levelEl.getText(); | ||
| function drainLogs() { | ||
| const logType = getPlatform() === 'ios' ? 'syslog' : 'logcat'; | ||
| return driver.getLogs(logType); | ||
| } | ||
|
|
||
| /** | ||
| * Check whether any log entry contains the given substring. | ||
| * Scans entries 0..count-1. | ||
| */ | ||
| export async function hasLogContaining(substring: string): Promise<boolean> { | ||
| const count = await getLogCount(); | ||
| for (let i = 0; i < count; i++) { | ||
| const msg = await getLogMessage(i); | ||
| if (msg.includes(substring)) { | ||
| return true; | ||
| async function collectNewLogs(): Promise<void> { | ||
| const entries = await drainLogs(); | ||
| for (const entry of entries) { | ||
| const msg = String((entry as Record<string, unknown>).message ?? entry); | ||
| if (msg.includes(BUNDLE_ID)) { | ||
| collectedLogs.push(msg); | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Wait until a log entry containing the substring appears, | ||
| * polling at the given interval. | ||
| */ | ||
| export function hasLogContaining(substring: string): boolean { | ||
| return collectedLogs.some((msg) => msg.includes(substring)); | ||
| } |
There was a problem hiding this comment.
🟡 The module-scoped collectedLogs array in logger.ts is never reset between tests, so waitForLog(substring) can find log entries produced by earlier tests and return a false positive. Since no spec file currently calls waitForLog or hasLogContaining directly (the code comment already discourages use), there is no active failure today, but the exported API is silently unreliable for any future caller.
Extended reasoning...
What the bug is and how it manifests
collectedLogs is declared at module scope (line 4 of logger.ts) and is never cleared between individual it() blocks or between spec files in a session. Every call to collectNewLogs() appends newly drained device log entries to this persistent array. Because hasLogContaining(substring) scans the entire accumulated history, any call to waitForLog(substring) can return immediately after finding a log entry that was produced by an earlier test — not the current one.
The specific code path that triggers it
driver.getLogs(logType) is a draining call: the Appium server returns only log entries accumulated since the last call and empties its buffer. So drainLogs() does correctly fetch only new entries. The problem is that every drained entry is then push()-ed into collectedLogs, which is never trimmed or reset. hasLogContaining() calls collectedLogs.some(…) against the full session-wide history, not just entries collected since the current test began.
Why existing code does not prevent it
There is no beforeEach or afterEach hook that calls collectedLogs.length = 0 or reassigns the array. WebdriverIO runs each spec file in the same Node.js module instance (within a single worker), so the module-level variable persists across all it() blocks in the file and across spec files sharing a worker.
Impact
Concrete scenario: Test A (e.g., in 10_event.spec.ts) sends a custom event with name rn_ios_no_props and the SDK prints a log line containing that string. Later, Test B calls waitForLog('rn_ios_no_props') expecting to verify the same event fired this time. waitForLog calls collectNewLogs() (which correctly drains only post-test-A entries) and then calls hasLogContaining('rn_ios_no_props') — which scans all of collectedLogs back to session start, finds Test A's entry, and returns success immediately even if Test B's action never produced the log. The test passes silently despite a real failure.
The current impact is zero: a search of all 12 spec files confirms no caller of waitForLog or hasLogContaining, and the function carries an explicit comment: "Avoid using this function and rely on snackbars instead". However, both functions are exported as public API, meaning any future test author who calls them will inherit silently unreliable behaviour.
How to fix it
Clear collectedLogs at the start of each waitForLog call (capturing only entries produced after the action under test), or add a clearCollectedLogs() export to be called from a beforeEach hook:
export async function waitForLog(substring, timeoutMs = 30_000, pollMs = 1_000) {
collectedLogs.length = 0; // reset so only logs from this point forward count
const deadline = Date.now() + timeoutMs;
while (Date.now() < deadline) {
await collectNewLogs();
if (hasLogContaining(substring)) return;
await driver.pause(pollMs);
}
throw new Error(`Timed out waiting for log containing "${substring}"`);
}Step-by-step proof
- Session starts;
collectedLogs = []. - Test A (
10_event.spec.ts, firstitblock) taps "TRACK EVENT", sets name torn_ios_no_props, taps "Track". The SDK emits:[com.onesignal.example] trackEvent: rn_ios_no_props. IfcollectNewLogs()were called at this point it would append this entry tocollectedLogs. - Test B (a hypothetical second
itblock) callswaitForLog('rn_ios_no_props').collectNewLogs()drains only new entries (correctly none at this instant).hasLogContaining('rn_ios_no_props')scanscollectedLogs— finds the entry appended in step 2 — and returnstrueimmediately. - Test B passes even though its own action never triggered the SDK call.
fadi-george
force-pushed
the
fadi/sdk-4333-appium-tests-2
branch
from
bebfcfb to
16d72cb
Compare
|
|
||
| const key1 = await byTestId('Key_input_1'); | ||
| await key1.setValue('test_trigger_key_3'); | ||
|
|
||
| const value1 = await byTestId('Value_input_1'); | ||
| await value1.setValue('test_trigger_value_3'); | ||
|
|
||
| let confirmButton = await byText('Add All'); |
There was a problem hiding this comment.
🔴 In addMultipleTriggers() (09_trigger.spec.ts), key1 and value1 are set via setValue() without first calling waitForDisplayed(), creating an intermittent failure risk on slow devices or Android. Add await key1.waitForDisplayed({ timeout: 5_000 }) (and optionally value1) after the addRowButton.click() — matching the pattern already used in 07_tag.spec.ts and 04_alias.spec.ts for their row-1 inputs.
Extended reasoning...
What the bug is and how it manifests
In addMultipleTriggers() (09_trigger.spec.ts lines 5–31), after clicking 'Add Row', the code immediately calls key1.setValue() and value1.setValue() on the newly-added second row without waiting for either element to be displayed. The only waitForDisplayed() call in this function is key0.waitForDisplayed(), which targets the first row's input — a row that was already rendered when the multi-trigger dialog opened (before 'Add Row' was clicked at all). As a result, key0.waitForDisplayed() returns essentially immediately, providing zero synchronization for the newly-rendered Row 1.
The specific code path that triggers it
await addRowButton.click(); // triggers async Flutter render of Row 1
const key0 = await byTestId('Key_input_0');
await key0.waitForDisplayed({ timeout: 5_000 }); // Row 0 was already visible → returns immediately
await key0.setValue('test_trigger_key_2');
const value0 = await byTestId('Value_input_0');
await value0.setValue('test_trigger_value_2'); // no wait
const key1 = await byTestId('Key_input_1');
await key1.setValue('test_trigger_key_3'); // BUG: no waitForDisplayed
const value1 = await byTestId('Value_input_1');
await value1.setValue('test_trigger_value_3'); // BUG: no waitForDisplayedWhy existing code doesn't prevent it
key0.waitForDisplayed() looks like a synchronization guard but it is not: Row 0 is present from the moment the dialog opens. The setValue() calls on key0 and value0 between the click and the key1 access take some wall-clock time, but on Android (newly added as a supported platform in this same PR) Flutter widget rendering can be significantly slower, and there is no guaranteed lower bound on how long those intermediate calls take. The Flutter semantics tree may not yet include Key_input_1 by the time key1.setValue() fires.
What the impact would be
Both 'can add multiple triggers' and 'can clear all triggers' call addMultipleTriggers(), so both tests are affected. The failure mode is an 'element not interactable' or 'element not found' error on key1.setValue() — intermittent, timing-dependent, and especially likely on Android where this PR is adding support for the first time.
How to fix it
Add waitForDisplayed on key1 before calling setValue, mirroring the established pattern:
const key1 = await byTestId('Key_input_1');
await key1.waitForDisplayed({ timeout: 5_000 }); // add this
await key1.setValue('test_trigger_key_3');Step-by-step proof
- addMultipleTriggers() opens the multi-trigger dialog; Row 0 (Key_input_0, Value_input_0) is already rendered.
- addRowButton.click() triggers an async Flutter rebuild that adds Row 1 to the widget/semantics tree.
- key0.waitForDisplayed() is called — Key_input_0 is already visible, so this resolves immediately with no delay.
- key0.setValue() and value0.setValue() execute; on a fast device these may complete before Row 1 is ready, but there is no guarantee.
- key1.setValue() is called; if Flutter has not yet added Key_input_1 to the semantics tree, Appium throws 'element not interactable' or a stale/missing element error.
- Compare 07_tag.spec.ts lines 39–41: after addRowButton.click(), the code calls await key1.waitForDisplayed({ timeout: 5_000 }) before key1.setValue(). Compare 04_alias.spec.ts lines 37–42: await label1.waitForDisplayed() and await id1.waitForDisplayed() are both present. The triggers spec is the only multi-row spec missing this guard.
| { | ||
| platformName: 'Android', |
There was a problem hiding this comment.
🟡 wdio.android.conf.ts now defaults to 'Samsung Galaxy S26' / Android 16, while run-local.sh still defaults DEVICE to 'Google Pixel 8' and OS_VERSION to '14', and scripts/README.md documents the same old defaults. A developer running 'bunx wdio run wdio.android.conf.ts' directly (without run-local.sh) will target a different device than the documentation implies.
Extended reasoning...
What the bug is and how it manifests
This PR updates wdio.android.conf.ts lines 8–9 to use fallback defaults of Samsung Galaxy S26 (deviceName) and 16 (platformVersion). However, two other locations were not updated: run-local.sh (lines 138–139) still defaults DEVICE to Google Pixel 8 and OS_VERSION to 14, and scripts/README.md still documents the DEVICE default as iPhone 17 / Google Pixel 8. This creates an inconsistency: the conf file and the documentation describe different default device configurations.
The specific code path that triggers it
The divergence only matters when wdio.android.conf.ts is invoked directly without run-local.sh — for example, via bunx wdio run wdio.android.conf.ts in a CI pipeline or BrowserStack context where DEVICE/OS_VERSION are not set. In that case, process.env.DEVICE || 'Samsung Galaxy S26' resolves to Samsung Galaxy S26 / Android 16. A developer reading scripts/README.md or run-local.sh would expect Google Pixel 8 / Android 14.
Addressing the refutation
The refutation correctly notes that when run-local.sh is used (the documented local dev path), it explicitly exports DEVICE=Google Pixel 8 and OS_VERSION=14 before invoking bunx wdio run, so those env vars override the conf defaults and the local test path always works correctly. The conf defaults are therefore irrelevant for the run-local.sh workflow. However, this does not resolve the documentation inconsistency: scripts/README.md documents DEVICE default as iPhone 17 / Google Pixel 8, which is now stale relative to the conf file. A developer setting up a BrowserStack or raw CI wdio run without run-local.sh, or simply reading the conf file to understand Android device targeting, will encounter contradictory information.
Why existing code does not prevent it
There is no automated check that validates consistency between wdio conf file fallback values and the defaults documented in README or used in run-local.sh. The conf file change was intentional (targeting a newer BrowserStack device) but the documentation was not updated to match.
What the impact would be
The impact is limited to developer confusion and potential misconfiguration of direct wdio invocations (BrowserStack/CI paths that do not go through run-local.sh). No local dev workflow is broken because run-local.sh always passes explicit DEVICE/OS_VERSION values. This is a documentation/configuration nit, not a functional regression.
How to fix it
Either: (a) update scripts/README.md to document the Android DEVICE default as Samsung Galaxy S26 / OS_VERSION as 16, or (b) update wdio.android.conf.ts to match the run-local.sh defaults (Google Pixel 8 / 14), depending on the intended CI target device. If the conf change is intentional for BrowserStack, option (a) is the right fix.
Step-by-step proof
wdio.android.conf.tsdiff shows:'appium:deviceName': process.env.DEVICE || 'Samsung Galaxy S26'and'appium:platformVersion': process.env.OS_VERSION || '16'.run-local.shlines 138–139 show:DEVICE="${DEVICE:-Google Pixel 8}"andOS_VERSION="${OS_VERSION:-14}".scripts/README.mdEnvironment Variables table showsDEVICEdefault asiPhone 17 / Google Pixel 8.- Run
bunx wdio run wdio.android.conf.tswithout setting DEVICE/OS_VERSION → targets Samsung Galaxy S26 / Android 16. - Read
scripts/README.md→ expects Google Pixel 8 / Android 14. - The contradiction is unambiguous: two different device configurations are implied by two authoritative sources.
There was a problem hiding this comment.
The PR updated run-local.sh Android defaults from 'Google Pixel 8' / '14' to 'Android 16' / '16', which partially addresses this comment. However, scripts/README.md still documents the old Android defaults in the Environment Variables table:
| DEVICE | iPhone 17 / Google Pixel 8 | ...
| OS_VERSION | 26.2 / 14 | ...
This is now doubly stale: run-local.sh uses 'Android 16' / '16', and wdio.android.conf.ts uses 'Samsung Galaxy S26' / '16'. The README should be updated to reflect the current Android defaults.
There was a problem hiding this comment.
scripts/README.md still documents the old Android defaults (Google Pixel 8 / 14) in the Environment Variables table, while the actual code now uses different values in two separate places: run-local.sh uses 'Android 16' / '16', and wdio.android.conf.ts uses 'Samsung Galaxy S26' / '16'. The README was not updated as part of this PR's changes to those files.
The DEVICE row should be updated from iPhone 17 / Google Pixel 8 to reflect the current Android default, and OS_VERSION from 26.2 / 14 to 26.2 / 16.
| await snackbar.waitForDisplayed({ timeout: 5_000 }); | ||
| }); | ||
|
|
||
| it('can send an outcome with value', async () => { | ||
| const sendButton = await scrollToEl('SEND OUTCOME', { by: 'text' }); | ||
| await sendButton.click(); | ||
|
|
||
| const withValueRadio = await byText('Outcome with Value'); | ||
| await withValueRadio.click(); | ||
|
|
||
| const nameInput = await byTestId('outcome_name_input'); | ||
| await nameInput.waitForDisplayed({ timeout: 5_000 }); | ||
| await nameInput.setValue('test_valued'); | ||
|
|
||
| const valueInput = await byTestId('outcome_value_input'); | ||
| await valueInput.waitForDisplayed({ timeout: 5_000 }); | ||
| await valueInput.setValue('3.14'); | ||
|
|
||
| const sendBtn = await byTestId('outcome_send_button'); | ||
| await sendBtn.click(); | ||
|
|
||
| const snackbar = await byText('Outcome sent: test_valued = 3.14'); | ||
| await snackbar.waitForDisplayed({ timeout: 5_000 }); | ||
| }); |
There was a problem hiding this comment.
🟡 In the 'can send an outcome with value' test, withValueRadio.click() is called immediately after sendButton.click() without waiting for the dialog to appear; the sibling tests ('can send a normal outcome', 'can send a unique outcome') both gate on nameInput.waitForDisplayed({ timeout: 5_000 }) before interacting with any dialog element. On a slow device or emulator where the dialog takes time to animate open, the premature radio click will throw element-not-interactable. Fix: move nameInput.waitForDisplayed({ timeout: 5_000 }) before withValueRadio.click().
Extended reasoning...
What the bug is and how it manifests
In 08_outcome.spec.ts, the 'can send an outcome with value' test (lines 50–70) calls await withValueRadio.click() immediately after await sendButton.click() with no synchronization guard. byText('Outcome with Value') returns a lazy element selector — the actual accessibility-tree lookup happens on .click(). If the outcome dialog takes any time to animate open (real device, slow emulator, CI runner), the click will fail with an element-not-found or element-not-interactable error before the dialog is visible.
The specific code path that triggers it
// 'can send an outcome with value'
await sendButton.click();
const withValueRadio = await byText('Outcome with Value');
await withValueRadio.click(); // ← no dialog-ready guard here
const nameInput = await byTestId('outcome_name_input');
await nameInput.waitForDisplayed({ timeout: 5_000 }); // ← wait comes too lateCompare with the established pattern in both sibling tests:
// 'can send a normal outcome' and 'can send a unique outcome'
await sendButton.click();
const nameInput = await byTestId('outcome_name_input');
await nameInput.waitForDisplayed({ timeout: 5_000 }); // ← gate FIRST
await nameInput.setValue('test_normal');
const normalRadio = await byText('Normal Outcome');
await normalRadio.click(); // ← radio AFTER guardWhy existing code doesn't prevent it
In WebdriverIO with Appium, element.click() on a non-displayed element fails immediately without retry. The only safeguard in the 'with value' test is the WebdriverIO implicit waitforTimeout (3 000 ms per wdio.shared.conf.ts), whereas the other two tests use an explicit 5 000 ms wait — a stronger reliability guarantee. On a slow device where the dialog takes more than 3 seconds to animate in, withValueRadio.click() will throw before the dialog is ready.
What the impact would be
Intermittent test failure in CI or on real/slow devices: the test aborts at the radio-click step with 'element not found' or 'element not interactable', making it impossible to determine whether the outcome-with-value flow itself works. The failure is timing-dependent and therefore difficult to reproduce consistently.
How to fix it
Move nameInput.waitForDisplayed({ timeout: 5_000 }) before the radio click, matching the pattern in the other two tests:
await sendButton.click();
const nameInput = await byTestId('outcome_name_input');
await nameInput.waitForDisplayed({ timeout: 5_000 }); // gate: dialog is open
const withValueRadio = await byText('Outcome with Value');
await withValueRadio.click(); // safe to click nowStep-by-step proof
- Test clicks 'SEND OUTCOME'; the outcome dialog begins animating open.
withValueRadio = await byText('Outcome with Value')resolves a lazy selector (no DOM check yet).await withValueRadio.click()issuesPOST /element/{id}/click— if the dialog is still animating, Appium cannot find the element in the accessibility tree and throwsError: element not found(ornot interactableon Android).- The test fails at step 3, never reaching
nameInput.setValueor the snackbar assertion. - With the fix, step 2 becomes
await nameInput.waitForDisplayed({ timeout: 5_000 }), which blocks until the dialog is fully open, and the radio click in step 3 always succeeds.
| }); | ||
|
|
||
| // end live activity | ||
| const endButton = await scrollToEl('END LIVE ACTIVITY', { by: 'text' }); | ||
| await endButton.click(); | ||
| await driver.pause(3_000); | ||
| await lockScreen(); | ||
|
|
||
| const activityEl = await $(`-ios predicate string:label CONTAINS "ORD-1234"`); | ||
| await activityEl.waitForDisplayed({ timeout: 5_000, reverse: true }); | ||
|
|
||
| await returnToApp(); | ||
| }); |
There was a problem hiding this comment.
🟡 At lines 80-85 of 12_activity.spec.ts, lockScreen() sets defaultActiveApplication to com.apple.springboard, but activityEl.waitForDisplayed({ timeout: 5_000, reverse: true }) runs without a try/finally guard. If the live activity is still visible after 5s, returnToApp() is never called — the same missing-cleanup pattern as the confirmed bug in checkActivity(), but at a distinct location in the inline test body.
Extended reasoning...
What the bug is and how it manifests
In the inline end-of-test body (12_activity.spec.ts lines 80-85), after clicking END LIVE ACTIVITY and pausing 3s, lockScreen() is called — executing driver.updateSettings({ defaultActiveApplication: "com.apple.springboard" }), switching the Appium element-lookup context to SpringBoard. Immediately after, activityEl.waitForDisplayed({ timeout: 5_000, reverse: true }) is called to confirm the Live Activity has been dismissed. If the Live Activity is still visible after 5 seconds, WebdriverIO throws a TimeoutError. There is no try/finally block, so returnToApp() at line 85 is never reached.
The specific code path that triggers it
await lockScreen(); // sets defaultActiveApplication = "com.apple.springboard"
const activityEl = await $(`-ios predicate string:label CONTAINS "ORD-1234"`);
await activityEl.waitForDisplayed({ timeout: 5_000, reverse: true }); // throws if still visible
await returnToApp(); // NEVER REACHED on TimeoutErrorThis is the same structural omission as the confirmed bug in checkActivity() (already reported in this PR review at line 38), but at a different code location — the inline test body rather than inside the helper function.
Why existing code does not prevent it
There is no try/finally guard protecting the session-context cleanup. lockScreen() and returnToApp() are paired only on the happy path. Any exception thrown between those two calls leaves defaultActiveApplication permanently set to com.apple.springboard for the remainder of the Appium session.
Addressing the refutation: why this is still worth fixing
The refutation correctly notes that this is the last it() block in the last spec file. After this test — Mocha finishes and WebdriverIO tears down the Appium session. There are no subsequent driver commands, so the stale defaultActiveApplication has zero immediate runtime consequence. This reasoning is valid and is why this is filed as nit rather than normal severity.
However, the defect is still real: (1) tests can be run in isolation via --spec, after which afterAll/cleanup hooks that issue driver commands would still be affected; (2) if new tests are appended to this file (a natural extension given this PR adds five new spec files), the unguarded lockScreen will immediately cause cascade failures in those tests; (3) it is structurally identical to the already-confirmed checkActivity() bug, so consistency argues for applying the same fix here.
How to fix it
Wrap the post-lock assertion in a try/finally block:
await lockScreen();
try {
const activityEl = await $(`-ios predicate string:label CONTAINS "ORD-1234"`);
await activityEl.waitForDisplayed({ timeout: 5_000, reverse: true });
} finally {
await returnToApp();
}Step-by-step proof
- Test calls endButton.click() and pauses 3s, then calls lockScreen() at line 80.
- lockScreen() executes driver.updateSettings({ defaultActiveApplication: "com.apple.springboard" }) — Appium now queries SpringBoard.
- The SDK fails to remove the Live Activity from the lock screen within 5 seconds.
- activityEl.waitForDisplayed({ timeout: 5_000, reverse: true }) throws TimeoutError.
- returnToApp() at line 85 is skipped; defaultActiveApplication remains com.apple.springboard.
- In the current codebase this has no observable consequence since no driver commands follow. But any afterAll hooks or future tests appended to this file would query SpringBoard instead of the demo app, producing misleading element-not-found errors.
| const alertHandled = await browser.sharedStore.get('alertHandled'); | ||
| if (!alertHandled) { | ||
| const alert = await waitForAlert(); | ||
| if (alert) await driver.acceptAlert(); | ||
| await browser.sharedStore.set('alertHandled', true); | ||
| } |
There was a problem hiding this comment.
🔴 On Android, waitForAppReady() calls waitForAlert() unconditionally, which internally issues driver.execute('mobile: alert', { action: 'getButtons' }) — an XCUITest-only command. On Android/UiAutomator2 this throws on every poll, causing driver.waitUntil to spin for the full 10-second timeout before returning null. Since wdio.android.conf.ts already sets autoGrantPermissions: true, no permission alert will ever appear on Android — the wait is entirely wasted. Fix: guard the waitForAlert block with if (getPlatform() === 'ios') { ... }; getPlatform() is already imported in app.ts.
Extended reasoning...
What the bug is and how it manifests
In waitForAppReady() (app.ts lines 136–141), when alertHandled is not yet set in sharedStore (i.e., first test of an Android session), waitForAlert(timeoutMs=10_000) is called with no platform check. Inside waitForAlert(), the poll function executes driver.execute('mobile: alert', { action: 'getButtons' }). This is an XCUITest-specific Appium command — on Android/UiAutomator2 it is not supported and throws an error on every invocation. The inner catch silently returns false, so driver.waitUntil continues polling at 250ms intervals until the full 10-second timeout elapses, at which point the outer catch returns null. The net result is a guaranteed ~10-second dead wait injected into every Android test session.
The specific code path that triggers it
// app.ts ~line 136
const alertHandled = await browser.sharedStore.get('alertHandled');
if (!alertHandled) {
const alert = await waitForAlert(); // <-- spins 10s on Android
if (alert) await driver.acceptAlert();
await browser.sharedStore.set('alertHandled', true);
}Inside waitForAlert():
const buttons = await driver.execute('mobile: alert', { action: 'getButtons' });
// ↑ throws NotImplementedError on UiAutomator2 every 250ms for 10 secondsWhy existing code doesn't prevent it
The alertHandled guard in sharedStore means this stall occurs once per Android session (not per spec file), but it is still 10 seconds of dead time on every run. There is no platform check before calling waitForAlert(). The inner catch in the poll function is intentionally silent (to handle the case where no alert exists on iOS), but it also silently swallows the Android-incompatible command failure on every poll.
What the impact would be
Every Android test session starts with a 10-second stall in waitForAppReady(). Since autoGrantPermissions: true is already set in wdio.android.conf.ts, no permission alert will ever appear on Android — making the wait completely pointless in addition to being slow. In CI environments running Android against BrowserStack, this wastes 10 seconds of billable device time per session.
How to fix it
Add a platform guard around the waitForAlert block. getPlatform() is already imported in app.ts (line 5) and used elsewhere in the same file (lines 45, 230, 255):
const alertHandled = await browser.sharedStore.get('alertHandled');
if (!alertHandled) {
if (getPlatform() === 'ios') {
const alert = await waitForAlert();
if (alert) await driver.acceptAlert();
}
await browser.sharedStore.set('alertHandled', true);
}Step-by-step proof
- Android session starts; first spec file calls
waitForAppReady(). browser.sharedStore.get('alertHandled')returnsnull(not yet set).waitForAlert(10_000)is called. Inside,driver.waitUntilbegins polling every 250ms.- Each poll executes
driver.execute('mobile: alert', { action: 'getButtons' }). UiAutomator2 does not implement this command and throwsError: Not yet implemented(or similar) on every call. - The inner
catchreturnsfalseon every poll.driver.waitUntilnever sees atrueand runs for the full 10,000ms before throwing a timeout error. - The outer
catchinwaitForAlert()returnsnull. - Back in
waitForAppReady(),alertisnullsodriver.acceptAlert()is skipped.alertHandledis set totrue. - Total wasted time: ~10 seconds. The first Android test now begins 10 seconds late.
| AVD_NAME="${AVD_NAME:-${DEVICE// /_}}" | ||
| fi | ||
|
|
||
| # ── 1. Build app ───────────────────────────────────────────────────────────── |
There was a problem hiding this comment.
🔴 The Android DEVICE default in run-local.sh was set to 'Android 16' — an OS version codename — instead of a valid device name; this cascades to AVD_NAME='Android_16' via the spaces-to-underscores substitution, causing emulator -avd Android_16 to immediately fail with 'AVD not found' for any developer running ./run-local.sh --platform=android without explicitly setting --device. Fix by changing the default to a valid device name such as 'Samsung Galaxy S26' (matching wdio.android.conf.ts) and updating the --help text on line 56 which still documents the old 'Google Pixel 8' default, creating a direct contradiction in the same file.
Extended reasoning...
What the bug is and how it manifests
In run-local.sh lines 139–141, the Android platform defaults block was changed from DEVICE=Google Pixel 8 / AVD_NAME=Pixel_8 to DEVICE=Android 16 / AVD_NAME=${DEVICE// /_}. 'Android 16' is an Android OS version codename (analogous to 'Android Pie' or 'Android Oreo'), not a device model or AVD name. No physical device, BrowserStack device, or locally-created AVD would ever be named 'Android 16'.
The specific code path that triggers it
The substitution AVD_NAME="${AVD_NAME:-${DEVICE// /_}}" converts spaces to underscores, producing AVD_NAME='Android_16'. The start_android_emulator() function then executes emulator -avd "Android_16". Since no standard Android SDK AVD is named 'Android_16', the emulator command exits immediately with 'PANIC: Missing emulator engine program for 'x86_64' CPU' or 'AVD 'Android_16' not found'. Any developer running ./run-local.sh --platform=android without explicitly setting --device hits this failure on the very first step after the build, before any test can run.
Why existing code doesn't prevent it
There is no validation that checks whether DEVICE is a plausible device model name versus an OS version string. The shell parameter expansion ${DEVICE// /_} mechanically replaces spaces and trusts the value is a valid device name. The companion wdio.android.conf.ts defaults to 'Samsung Galaxy S26', which shows the intended target device, but run-local.sh was not updated consistently.
The help text contradiction
The --help output (line 56, introduced in this PR) reads --device=NAME Device/simulator/AVD name (default: iPhone 17 / Google Pixel 8), still documenting the old 'Google Pixel 8' default. A developer reading --help before running the script will see 'Google Pixel 8' documented as the Android default, but the actual behavior uses 'Android 16'. This double inconsistency (wrong default + outdated help text) makes diagnosis harder.
Impact
Every developer who runs ./run-local.sh --platform=android without explicitly passing --device will receive an immediate emulator failure. Since Android support was just added by this PR (the previous code hard-errored on Android entirely), every developer attempting to use the new Android feature for the first time will hit this on their first attempt. The failure also affects BrowserStack paths if DEVICE is not set, since 'Android 16' is not a valid BrowserStack device name either.
How to fix it
Change the default from 'Android 16' to a valid device name — 'Samsung Galaxy S26' matches the wdio.android.conf.ts default and is a real BrowserStack device. The AVD_NAME default can remain dynamic (${DEVICE// /_}), but should also document a real AVD name in the .env.example comment. The --help text on line 56 must be updated to read 'iPhone 17 / Samsung Galaxy S26'.
Step-by-step proof
- Developer clones repo, sets ONESIGNAL credentials in .env, runs
./run-local.sh --platform=android --sdk=flutter. - Script reaches the platform defaults block at line 139:
DEVICE="${DEVICE:-Android 16}"— DEVICE is unset, so DEVICE='Android 16'. - Line 141:
AVD_NAME="${AVD_NAME:-${DEVICE// /_}}"— AVD_NAME='Android_16'. - After building the APK,
start_android_emulator()runs:emulator -avd "Android_16" -no-audio -no-boot-anim. - Android SDK emulator reports 'PANIC: Cannot find AVD system path. Please define ANDROID_SDK_ROOT' or equivalently 'AVD Android_16 not found'. The script fails here.
- No tests run. The developer sees a confusing error that looks like an SDK setup issue rather than a wrong default value.
- Reading
--helpshows 'Google Pixel 8' as the documented Android default, adding confusion since the script clearly used 'Android 16' based on the error message.
fadi-george
force-pushed
the
fadi/sdk-4333-appium-tests-2
branch
from
16d72cb to
f5ee1a1
Compare
| ...(process.env.BUNDLE_ID ? { 'appium:appPackage': process.env.BUNDLE_ID } : {}), | ||
| 'appium:autoGrantPermissions': true, | ||
| 'appium:autoGrantPermissions': false, |
There was a problem hiding this comment.
🔴 This PR sets 'appium:autoGrantPermissions': false in wdio.android.conf.ts, which causes Android 13+ push notification permission dialogs to appear at runtime. The new waitForAlert() helper cannot dismiss them on Android because it uses driver.execute('mobile: alert', { action: 'getButtons' }), an XCUITest-only command that throws on UiAutomator2 on every poll — so the dialog stays on screen blocking all subsequent scrollToEl/byTestId calls. Fix: restore 'appium:autoGrantPermissions': true for Android, or implement UiAutomator2-compatible dialog handling (e.g. UISelector for Android system dialogs).
Extended reasoning...
What the bug is and how it manifests
This PR introduces a compound breaking change on Android. In wdio.android.conf.ts (lines 14–15), 'appium:autoGrantPermissions' is changed from true to false. Previously, UiAutomator2 used ADB to pre-grant all app permissions before the session started, so no runtime permission dialog ever appeared. With this change disabled, Android 13+ POST_NOTIFICATIONS permission dialogs (and location dialogs) will appear when the app starts. The newly added waitForAlert() helper is invoked from waitForAppReady() to handle these dialogs, but it is incompatible with Android.
The specific code path that triggers it
In app.ts waitForAppReady() (around lines 136–141), when alertHandled is not yet set in sharedStore, waitForAlert(10_000) is called with no platform guard. Inside waitForAlert(), driver.waitUntil polls every 250ms executing: driver.execute('mobile: alert', { action: 'getButtons' }). This is an XCUITest-specific Appium mobile command — on Android/UiAutomator2 it throws a NotImplementedError on every invocation. The inner catch silently returns false, so driver.waitUntil spins for the full 10-second timeout before the outer catch returns null. Back in waitForAppReady(), alert is null so driver.acceptAlert() is never called and the permission dialog remains on screen.
Why existing code doesn't prevent it
There is no platform guard before calling waitForAlert() — getPlatform() is already imported and used elsewhere in app.ts (lines 45, 230, 255) but not applied here. The silent catch inside the poll function was designed to handle the case where no alert is present on iOS, but it also swallows the Android-incompatible command failure on every single poll, making the 10-second burn invisible. Additionally, since noReset: true is set, the first run after a fresh install (or after run-local.sh's reset_app() uninstalls the app) is guaranteed to hit the POST_NOTIFICATIONS dialog on Android 13+.
What the impact would be
On the first Android test run (or any run after app uninstall via reset_app()), the push notification permission dialog appears → waitForAlert() silently fails for 10 seconds → dialog is never dismissed → dialog remains covering the app UI → every subsequent scrollToEl/byTestId call fails with element-not-found errors because the permission overlay blocks Flutter's semantics tree. All Android tests in the suite fail after this point. Even if bug_014 (iOS guard around waitForAlert) were fixed first, this bug would remain — there would still be no Android-compatible mechanism to dismiss the dialog.
Step-by-step proof
- Fresh Android install; run-local.sh uninstalls the app via reset_app(), then WebdriverIO installs the APK.
- App launches; Android 13+ shows the POST_NOTIFICATIONS permission dialog.
- waitForAppReady() is called; sharedStore.get('alertHandled') returns null.
- waitForAlert(10_000) begins polling: driver.execute('mobile: alert', { action: 'getButtons' }) throws NotImplementedError on UiAutomator2 every 250ms; catch returns false; total ~10 seconds wasted.
- waitForAlert() returns null; driver.acceptAlert() is skipped; sharedStore.set('alertHandled', true).
- The permission dialog is still on screen.
- The first test calls scrollToEl('user_section') → swipeMainContent → Flutter semantics tree query → Appium finds nothing because the permission overlay is covering the app → throws 'Element not found after 10 scrolls'.
- Every subsequent test fails with the same element-not-found pattern.
How to fix it
Either (a) restore 'appium:autoGrantPermissions': true in wdio.android.conf.ts (the minimal fix), or (b) implement UiAutomator2-compatible dialog handling using UISelector/UiAutomator2 native dialog APIs before disabling autoGrantPermissions. Additionally, add a platform guard in waitForAppReady(): if (getPlatform() === 'ios') { const alert = await waitForAlert(); if (alert) await driver.acceptAlert(); }
| before(async () => { | ||
| await waitForAppReady({ skipLogin: true }); | ||
| await scrollToEl('location_section'); |
There was a problem hiding this comment.
🔴 The 'can prompt for location' and 'can share location' tests in 11_location.spec.ts use XCUITest-only Appium commands (driver.execute('mobile: alert', ...) and the internal waitForAlert() helper), but unlike 12_activity.spec.ts which correctly guards its describe block with if (getPlatform() !== 'ios') { return this.skip(); }, 11_location.spec.ts has no platform guard at all. On Android, waitForAlert() spins for 10 seconds returning null (as mobile: alert throws on UiAutomator2), then expect(null).toContain('location') fails with a misleading null reference error — and the 'Share device location' toggle is also iOS-specific UI. Fix: add if (getPlatform() !== 'ios') { return this.skip(); } in the before() block, matching the pattern in 12_activity.spec.ts.
Extended reasoning...
What the bug is and how it manifests
11_location.spec.ts has no platform guard despite relying exclusively on XCUITest (iOS-only) Appium commands. The PR simultaneously adds Android support via wdio.android.conf.ts changes, meaning these tests will now be executed on Android where they are guaranteed to fail.
The specific code path that triggers it
The before() block navigates to the location section with no platform check:
before(async () => {
await waitForAppReady({ skipLogin: true });
await scrollToEl('location_section');
});In 'can prompt for location', after clicking PROMPT LOCATION, the test calls waitForAlert(). Internally, waitForAlert() polls via driver.execute('mobile: alert', { action: 'getButtons' }) — an XCUITest-exclusive command. On Android/UiAutomator2, this throws a NotImplementedError on every poll; the inner catch swallows it and returns false, so driver.waitUntil spins for the full 10-second timeout before returning null. The test then reaches expect(null).toContain('location'), which throws 'Cannot read properties of null' — never even reaching the direct driver.execute('mobile: alert', ...) call at lines 22–25.
Why existing code doesn't prevent it
12_activity.spec.ts, also newly added in this PR, correctly guards with:
before(async function () {
if (getPlatform() \!== 'ios') {
return this.skip();
}
...
});11_location.spec.ts was not given the same treatment. There is no compile-time or lint check that enforces platform guards when XCUITest-only commands are used.
What the impact would be
Running the Android suite includes 11_location.spec.ts. Both 'can prompt for location' and 'can share location' fail — the first after a 10-second dead wait with a null dereference error; the second because 'Allow While Using App' and the 'Share device location' toggle are iOS UI patterns that don't exist on Android. The failures surface as misleading errors unrelated to the actual root cause (wrong platform), making CI failures difficult to diagnose.
How to fix it
Add the same platform guard used in 12_activity.spec.ts:
before(async function () {
if (getPlatform() \!== 'ios') {
return this.skip();
}
await waitForAppReady({ skipLogin: true });
await scrollToEl('location_section');
});Step-by-step proof
- Android session starts; WebdriverIO runs 11_location.spec.ts.
- before() executes; no platform check, so it proceeds on Android.
- 'can prompt for location' runs: clicks PROMPT LOCATION, then calls waitForAlert().
- waitForAlert() polls driver.execute('mobile: alert', { action: 'getButtons' }) every 250ms for 10 seconds — UiAutomator2 throws on each call; the catch returns false each time.
- After 10 seconds, the outer catch returns null.
- expect(null).toContain('location') throws TypeError: Cannot read properties of null.
- Test fails with a misleading error after a 10-second wasted wait.
- Compare: 12_activity.spec.ts has getPlatform() !== 'ios' check in its before() — the exact same fix is needed here.
|
|
||
| let snackbar = await byText('Location shared: false'); | ||
| await snackbar.waitForDisplayed({ timeout: 5_000 }); | ||
| expect(snackbar).toBeDefined(); |
There was a problem hiding this comment.
🟡 In the 'can share location' test (11_location.spec.ts line 34), expect(snackbar).toBeDefined() is a vacuous assertion that always passes. byText() returns a ChainablePromiseElement — a lazy WebdriverIO query object — which is never null or undefined regardless of whether the element is actually visible. The meaningful check was already performed by snackbar.waitForDisplayed() on the previous line; this assertion provides no real test coverage and should be removed or replaced with await expect(snackbar).toBeDisplayed().
Extended reasoning...
What the bug is: On line 34 of 11_location.spec.ts, after await snackbar.waitForDisplayed({ timeout: 5_000 }) has already passed, the code calls expect(snackbar).toBeDefined(). This assertion is vacuously true and provides no test coverage.
Why the assertion is always truthy: byText() in selectors.ts uses WebdriverIO's $(...) selector function (either -ios predicate string:label == "${text}" for iOS or android=new UiSelector().text("${text}") for Android). This returns a ChainablePromiseElement — a lazy proxy/wrapper object representing a query that will be executed later. The object itself is always defined; it is not the result of a DOM lookup. Whether or not the element exists in the accessibility tree has no bearing on whether the reference is defined. Jest's toBeDefined() simply checks that the value is not undefined, which is trivially true for any object instance.
The specific code path: snackbar is assigned via await byText('Location shared: false') (line 33). At this point, snackbar holds a resolved ChainablePromiseElement instance — but the await here resolves the byText() async function itself, not a DOM query. The element reference is still lazy. await snackbar.waitForDisplayed({ timeout: 5_000 }) on line 33 performs the actual visibility check. Line 34's expect(snackbar).toBeDefined() then checks only that snackbar \!== undefined, which is guaranteed by construction.
Why existing code doesn't prevent it: TypeScript has no way to distinguish a lazy element reference from an actual DOM result at the type level. Jest's toBeDefined() is a synchronous matcher that doesn't interact with the WebdriverIO async element lifecycle. The absence of await in front of expect(snackbar).toBeDefined() is a further signal that this is not using an async matcher like toBeDisplayed().
Impact: The assertion gives a false sense of test coverage. If the snackbar never appears, waitForDisplayed() will throw a TimeoutError — but if that line were ever removed or made non-throwing, the toBeDefined() check would still pass silently even with a broken snackbar display. As written, the assertion is dead code that misleads future readers into thinking it verifies something meaningful.
How to fix it: Either remove the assertion entirely (since waitForDisplayed() already provides the real guard), or replace it with the correct async assertion: await expect(snackbar).toBeDisplayed(). The await is required because toBeDisplayed() in expect-webdriverio returns a Promise<void>.
Step-by-step proof:
byText('Location shared: false')is called — it returns$(-ios predicate string:label == "Location shared: false"), aChainablePromiseElementobject.await byText(...)resolves the async wrapper;snackbaris now aChainablePromiseElementinstance (an object, neverundefined).await snackbar.waitForDisplayed({ timeout: 5_000 })executes the actual Appium query and waits for the element to be visible — this is the real assertion.expect(snackbar).toBeDefined()is evaluated:snackbaris an object, sosnackbar \!== undefinedistrue. Jest marks this as passed regardless of anything in the UI.- The assertion cannot fail under any circumstances, making it misleading dead code.
| // remove tag | ||
| const removeButton = await byTestId(`triggers_remove_test_trigger_key`); | ||
| await removeButton.click(); |
There was a problem hiding this comment.
🟡 The comment at line 58 of 09_trigger.spec.ts reads '// remove tag' instead of '// remove trigger', a copy-paste artifact from 07_tag.spec.ts. The surrounding code correctly operates on triggers (triggers_remove_test_trigger_key), making the stale comment misleading to anyone reading the test.
Extended reasoning...
What the bug is and how it manifests
Inside the 'can add and remove trigger' test in 09_trigger.spec.ts, the comment immediately before the remove operation reads '// remove tag' (line 58). The analogous section in 07_tag.spec.ts also has a '// remove tag' comment — this comment was copy-pasted verbatim without being updated. The actual code that follows the comment is correct and operates entirely on triggers, not tags.
The specific code path
The test ID triggers_remove_test_trigger_key, the describe('Triggers', ...) block name, the preceding '// add trigger' comment, and the expectPairInSection('triggers', ...) call all confirm this block removes a trigger. Only the comment is wrong.
Why existing code does not prevent it
Comments are free-form text with no compile-time or lint-time validation against the surrounding code semantics. The copy-paste from 07_tag.spec.ts was not caught during review.
What the impact would be
The impact is purely cosmetic — the test executes correctly regardless of comment content. However, a developer reading this test to understand what is being removed would be briefly misled into thinking tags are being removed rather than triggers. It also signals an incomplete copy-paste migration, which can undermine confidence in the thoroughness of the new test file.
How to fix it
Change line 58 from '// remove tag' to '// remove trigger'.
Step-by-step proof
- Open appium/tests/specs/07_tag.spec.ts — the 'can add and remove a tag' test has a comment '// remove tag' before calling byTestId('tags_remove_test_tag').
- Open appium/tests/specs/09_trigger.spec.ts — the 'can add and remove trigger' test has the identical comment '// remove tag' at line 58, before calling byTestId('triggers_remove_test_trigger_key').
- The PR diff confirms this line was introduced as '+ // remove tag' in the new file.
- The describe block is 'Triggers', the add comment above reads '// add trigger', and the test ID contains 'triggers_remove_' — every contextual signal says trigger, not tag.
- The comment is unambiguously wrong and was copy-pasted without updating.
|
|
||
| # ── Optional (defaults shown) ──────────────────────────────────────────────── | ||
| # DEVICE=iPhone 17 # iOS default; Android default: Google Pixel 8 | ||
| # OS_VERSION=26.2 # iOS default; Android default: 14 |
There was a problem hiding this comment.
🟡 This PR changed the Android OS_VERSION default from 14 to 16 in run-local.sh (line 140), but two documentation locations were not updated: (1) appium/scripts/.env.example line 11 still reads '# OS_VERSION=26.2 # iOS default; Android default: 14', and (2) run-local.sh --help text (line 69) still reads 'OS_VERSION Platform version (default: 26.2 / 14)'. A developer consulting either location to understand the default will configure OS_VERSION=14 when the actual runtime default is now 16.
Extended reasoning...
What the bug is and how it manifests
This PR updated the Android platform defaults block in run-local.sh, changing OS_VERSION from '14' to '16' (diff: - OS_VERSION="${OS_VERSION:-14}" → + OS_VERSION="${OS_VERSION:-16}"). However, two documentation artifacts that describe this default were not updated alongside the behavioral change, leaving them stale and contradicting the actual runtime behavior.
The two affected locations
Location 1 — appium/scripts/.env.example line 11: The comment reads # OS_VERSION=26.2 # iOS default; Android default: 14. The PR diff shows the adjacent DEVICE line was removed, but the OS_VERSION comment line was left intact with the old value of 14. This file is explicitly intended as a developer reference for understanding configurable defaults.
Location 2 — appium/scripts/run-local.sh --help text (line 69): The help block reads OS_VERSION Platform version (default: 26.2 / 14). The PR diff shows that two neighboring lines in the same help block were removed (DEVICE and AVD_NAME entries), but the OS_VERSION line was not updated to reflect the new Android default.
Why existing code does not prevent it
There is no automated consistency check between the runtime default values assigned in the platform defaults block and the documentation strings in the help text or .env.example comments. Both are free-form strings maintained manually. The localized nature of the change — touching only the assignment line and not the surrounding documentation — made it easy to miss the two other locations.
What the impact would be
A developer reading .env.example to understand what OS_VERSION to set for Android will be told the default is 14. If they set OS_VERSION=14 explicitly (thinking they are matching the default), they will actually get a mismatched BrowserStack configuration against the new 'Samsung Galaxy S26 / Android 16' target device that wdio.android.conf.ts now uses by default. Similarly, a developer running ./run-local.sh --help before configuring their environment will see 14 as the Android OS_VERSION default, which contradicts the behavior they will actually observe.
How to fix it
Update .env.example line 11 from # OS_VERSION=26.2 # iOS default; Android default: 14 to # OS_VERSION=26.2 # iOS default; Android default: 16. Update run-local.sh line 69 from OS_VERSION Platform version (default: 26.2 / 14) to OS_VERSION Platform version (default: 26.2 / 16).
Step-by-step proof
- The PR diff for run-local.sh shows:
- OS_VERSION="${OS_VERSION:-14}"→+ OS_VERSION="${OS_VERSION:-16}"— the actual runtime Android default is now 16. 2. The PR diff for .env.example shows the OS_VERSION comment line was not modified: it still reads# OS_VERSION=26.2 # iOS default; Android default: 14. 3. The PR diff for run-local.sh help block shows only DEVICE and AVD_NAME lines were removed; the OS_VERSION help lineOS_VERSION Platform version (default: 26.2 / 14)was preserved unchanged. 4. A developer running./run-local.sh --helpor reading .env.example sees 14 as the Android OS_VERSION default but will get 16 at runtime — a direct contradiction in the same PR.
| 'appium:platformVersion': process.env.OS_VERSION || '18', | ||
| 'appium:automationName': 'XCUITest', | ||
| ...(process.env.BUNDLE_ID ? { 'appium:bundleId': process.env.BUNDLE_ID } : {}), | ||
| 'appium:autoAcceptAlerts': true, | ||
| 'appium:autoAcceptAlerts': false, |
There was a problem hiding this comment.
🟣 Pre-existing: wdio.ios.conf.ts defaults to 'iPhone 16' as the fallback device name (line 12: process.env.DEVICE || 'iPhone 16'), while run-local.sh defaults DEVICE to 'iPhone 17' and scripts/README.md documents the iOS default as 'iPhone 17'. Any direct wdio invocation where DEVICE is unset (e.g. a BrowserStack CI pipeline) will target iPhone 16 instead of the documented iPhone 17. This PR touches wdio.ios.conf.ts to change autoAcceptAlerts but does not fix this mismatch — consider updating the fallback to 'iPhone 17' to keep it consistent with the documented default.
Extended reasoning...
What the bug is and how it manifests
In appium/wdio.ios.conf.ts (line 12), the iOS device fallback is hard-coded as 'iPhone 16':
'appium:deviceName': process.env.DEVICE || 'iPhone 16',However, two other authoritative sources document iPhone 17 as the iOS default:
appium/scripts/run-local.sh(line 133):DEVICE="${DEVICE:-iPhone 17}"appium/scripts/README.mdEnvironment Variables table:DEVICEdefault listed asiPhone 17 / Google Pixel 8
The specific code path that triggers it
This inconsistency only manifests when wdio.ios.conf.ts is invoked directly without going through run-local.sh — for example, in a BrowserStack CI pipeline running bunx wdio run wdio.ios.conf.ts where the DEVICE environment variable is not set. In that case, process.env.DEVICE || 'iPhone 16' resolves to 'iPhone 16'. A developer reading run-local.sh or scripts/README.md would expect 'iPhone 17'.
Why existing code doesn't prevent it
There is no automated consistency check between the fallback values in the wdio conf files and the defaults documented in run-local.sh or scripts/README.md. Both are maintained manually. The PR changed autoAcceptAlerts on the adjacent line but left the deviceName fallback intact.
What the impact would be
Any CI pipeline or direct wdio invocation that relies on the conf file's fallback (rather than explicitly setting DEVICE) will target iPhone 16 — one model behind the documented default. BrowserStack device availability and capability support differ between iPhone generations, so this mismatch could cause unexpected test failures or differences in test behavior compared to local runs via run-local.sh.
How to fix it
Update the fallback in wdio.ios.conf.ts from 'iPhone 16' to 'iPhone 17' to match the documented defaults:
'appium:deviceName': process.env.DEVICE || 'iPhone 17',Step-by-step proof
- Developer sets up BrowserStack CI;
DEVICEenv var is not exported. - CI runs:
bunx wdio run wdio.ios.conf.ts(withoutrun-local.sh). process.env.DEVICEisundefined, so'appium:deviceName'resolves to'iPhone 16'.- Developer reads
scripts/README.mdand seesDEVICEdefault documented as'iPhone 17'— contradiction. - Developer reads
run-local.shand seesDEVICE="${DEVICE:-iPhone 17}"— same contradiction. - Fix: change
wdio.ios.conf.tsline 12 fallback from'iPhone 16'to'iPhone 17'.
2 participants
Description
Expands the Appium E2E test suite with new test specs for outcomes, triggers, custom events, location, and live activities. Also improves test infrastructure reliability and consistency.
appium_more_tests.mov
Details
New test specs:
08_outcome.spec.ts- outcome sending tests09_trigger.spec.ts- trigger add/remove/clear tests10_event.spec.ts- custom event tracking with and without properties11_location.spec.ts- location prompting and sharing12_activity.spec.ts- iOS live activity start/update/end lifecycleInfrastructure improvements:
autoAcceptAlertsfor iOS to enable explicit alert handling in location testswaitForAlerthelper for manual native alert detectionscrollToElwith edge detection so elements near screen edges get an extra nudge scrollreturnToApphelper for tests that leave the app (e.g. lock screen)byTextandgetTestDataselector helpersOther:
customEventsentry todemo/tooltip_content.jsonMade with Cursor