fix: let the WebDriverAgent build be serialised against other native builds - #126
Open
mpretty-cyro wants to merge 26 commits into
Open
fix: let the WebDriverAgent build be serialised against other native builds#126mpretty-cyro wants to merge 26 commits into
mpretty-cyro wants to merge 26 commits into
Conversation
`resolveDevnetServices` already probes the backend and validates its key, so this only decides whether the clients are told about it, keyed off the same `TEST_PRO_BACKEND` switch Desktop uses. Both values travel together: the pubkey is what libSession verifies other users' proofs against, so a device given the QA URL on the production key reads a QA-signed proof as invalid and stores the sender as non-Pro. Android's extras are persisted rather than applied live, so the app is relaunched once after the session starts and before the spec is handed the device.
The derivation is pinned to libsession's committed test vectors, runnable without a device or a backend via `pnpm test-pro-keys`. `makeAccountPro` also now checks that the recovery phrase it was handed belongs to the account under test. The dev route creates a `users` row for whatever key it is given, so a wrong-account mint answers `redeemed=true` with a real expiry and only surfaces later as the client reporting `never`. Mints against the discovered QA backend rather than the compiled-in production URL, which has no `/dev` routes by design.
Locators match on accessibility id rather than display text so one set serves both platforms. Two traps are documented where they sit: an id lands on iOS as the element's `name`, pushing display text to `label`, and a combined accessibility element can swallow what is being matched -- the status banner reports a progress value instead of its message, and the conversation header collapses the Pro badge out of the tree entirely. The settings-state specs drive the launch-arg mocks, which are display-level and per-device: they cover how the UI renders Pro state without a backend. The badge visibility spec needs a real grant instead, because the assertion is another user's client verifying a cryptographic proof. `Pro Activated CTA` moves to the mocks on iOS for the same reason. That also removes a race the real grant cannot win: the client skips a status refresh if the last one was under a minute ago, and an account's first fetch necessarily happens during onboarding, before there is anything to buy.
`pnpm create-emulators <n>` creates and boots N Pixel 6 / API 34 AVDs, pins them to the udids the suite expects, patches the camera to virtualscene and writes the SDK paths to `.env`. Appium will not boot emulators itself. `setup_virtual_scene` reads ANDROID_SDK_ROOT but never loaded `.env`, so the documented step only worked if the shell happened to export it. Also adds `pnpm test-pro-keys`, the device-free Pro key derivation check.
Every other account a test touches is generated per run and discarded, so a grant is disposable. The moderation account is a fixed identity reused by every run and job, on a Pro backend that is shared and never reset — so granting it Pro would be permanent and would renew on every re-run. The account-under-test check cannot catch this: a spec that deliberately passes the admin phrase derives a matching account id and sails through.
Room operations are minutes apart -- allocate at the start of a test, release at the end -- so a pooled keep-alive socket is routinely closed by uWSGI before it is picked up again, and the reuse surfaced as `read ECONNRESET` on a request the server never saw. `Connection: close` removes the pool, so there is no socket to go stale; retrying would not do, as `POST /rooms` is not idempotent. `fetch` also reported every transport failure as a bare `TypeError: fetch failed` with the detail hidden in `cause`, which Playwright never prints -- refused, reset, timed out and DNS were indistinguishable. The cause is now folded into the message along with the request that provoked it.
`adb` is not on PATH -- which is why APPIUM_ADB_FULL_PATH exists -- so every `adb` call in `utilities.ts` failed, including the force-stop/restart pair. `runScriptAndLog` logs a failure and returns rather than throwing, so this was silent: on Android the app was never restarted, never re-fetched its Pro status, and stayed on whatever it had cached from before the grant. That took out every Android spec that mints Pro and restarts to observe it.
The signing key regenerates on container recreate, and a stale one is not a connection error -- the client reads every proof as invalid and silently strips Pro content, which reads as an app bug. The local backend also runs on a compressed clock, so proofs live ~300s rather than ~30 days. It is written into config.ini by the entrypoint rather than set as an environment variable, so `docker inspect` reports the opposite.
`scripts/ci.sh` declared the API level, image target, build tools and device profile a second time, with nothing keeping them in step with the local script. Both now read `scripts/android_config.ts`, which is the only place a version changes. The ABI stays detected rather than declared, since it is a property of the host rather than a choice, and a wrong one fails immediately instead of drifting. Every ci.sh function moves behind a flag: --provision, --snapshot-save, --start-snapshots and --kill. Two behaviour changes on CI. Emulators are now started with `-port` rather than relying on start order, so a leftover emulator can no longer renumber the fleet and leave the suite talking to the wrong device. And the AVDs take the name prefix the local script already used, so --provision has to run once per worker to recreate them.
The New Message screen is a bottom sheet, and `scrollDown` is a raw swipe rather than a scroll-container operation, so it dragged the sheet itself. Next stayed present and findable -- the click therefore succeeded and threw nothing -- while the tap landed outside its clickable region. No error, no navigation, nothing in the device log. That took out every Android spec reaching a conversation this way: the eight message-length specs went 0/8, and 7/8 now pass (the eighth fails during onboarding, unrelated). iOS was unaffected, where the same gesture acts on a scroll view. The swipe was vestigial in any case -- it predates the Next button learning to avoid the keyboard, and `community_links` and `ons_resolve` already clicked Next without it. `hideKeyboard` states the original intent instead, and is best-effort because both drivers throw when there is no keyboard up and "nothing to dismiss" is the desired end state.
`getAttribute('value')` is iOS-only. UiAutomator2 has no `value` attribute and
rejects the request outright, so the Android run died in the driver before any
assertion could report anything useful.
Reading the toggle and asserting the badge move to `pro_badge.ts`, so the next
spec needing either gets the platform difference handled rather than repeating
it -- badge visibility is a separate per-user setting that a grant never
touches, so anything asserting a peer can see a badge has to turn it on first.
`@session-foundation/mnemonic` was already present as a transitive dependency, so making it direct costs nothing and replaces a hand-rolled base-1626 decoder -- along with the wordset, the prefix matching and the checksum word, which are now its problem rather than ours. `pnpm test-pro-keys` still matches libsession's committed vectors. Also drops the `uuid` dependency, which nothing imported: the only UUID in the repo already comes from `node:crypto`.
The backend does honour SESH_PRO_BACKEND_PROVIDER_TESTING_ENV; the compose stack just never sets it, writing provider_testing_env into config.ini directly. The practical warning is unchanged -- the container env reads as "not in testing mode" -- but the reason given for it was wrong, which would send anyone checking to the wrong place. Also records the shape the mode actually applies, and that Android now has the renewal floor the other two clients already had.
Nearly every failure in this suite is "element not found", and a screenshot cannot separate the three things that produce it: the element is absent, it is present but not hittable, or it is present under a different identifier than the locator expects. Only the tree distinguishes them. Attached by path rather than as a body, because an in-memory attachment survives only if a reporter persists it and the default local reporter does not -- the tree would exist in the report stream and nowhere a developer could open it. Devices are also registered as soon as their session exists rather than when the opener returns. Everything in between -- the QA relaunch, onboarding, the first wait for the landing screen -- was a blind spot with no screenshot, page source or device log, because capture is keyed on the registry and nothing was in it yet. Registration deduplicates by udid so the existing calls become no-ops. The first capture from it identified an intermittent onboarding failure as an Android "System UI isn't responding" dialog covering the app, rather than anything in the client or the locators.
An Android "isn't responding" dialog covers the app, so ordinary elements drop out of the tree and every lookup fails as "element not found" -- attributed to whichever spec was unlucky rather than to the machine. The one that prompted this was com.android.systemui freezing under host load, not Session. One dialog is dismissed with "Wait" (not "Close app", which would kill the app under test when the ANR is ours) and the lookup retried. A second is not: it throws naming the ANR and the device, so a saturated host is reported as a saturated host in the results instead of being absorbed into a spec failure.
Session_Android now reads `sessionProBackendStatus` / `sessionProLoadingState` as intent extras, so the mock-driven Pro settings specs are no longer iOS-only. `ProMockContext` holds the two fields both platforms understand and lives in its own module rather than in either platform's capabilities, since both consume it and neither owns it. `IOSTestContext` extends it, which makes the boundary explicit: exactly two fields cross over and the rest are iOS's. `Pro settings screen (subscribed)` and `Pro settings entry (expired)` are now cross-platform. The other two stay iOS-only, with the reasons recorded where they are declared -- Android has no status-banner identifier, and its loading/error mocks only apply while the Pro state is forced, so a non-Pro user cannot reach those banners the way iOS can. The shared 30-day expiry is a cross-repo constant: iOS takes a timestamp while Android uses a fixed debug offset, and that offset was moved to match so one spec can assert one rendered string.
On iOS an accessibility identifier becomes the element's `name` and displaces the display text into `label`, so `text` -- which reads the value -- silently stops matching the moment an element gains an id. Asserting "this identifier AND this message" was therefore inexpressible, and the status banner locator had dropped to xpath to say it. A reviewer flagged that xpath as fragile and likely to break if the specs are reused for a future client, which is fair: it encodes attribute layout rather than identity. `label` states the intent directly and the locator goes back to an accessibility id. The two remaining xpath/uiautomator locators in this file are a different problem -- `pro-badge-text` and `action-item-subtitle` are deliberately shared across rows, so they are scoped by parent to disambiguate. Those need unique ids in the apps, which is raised separately.
`ProBadgeText` renders the display name unconditionally and only the badge icon behind `if (showBadge)`, so scoping the Android locator to `pro-badge-text` matched every conversation whether the sender was Pro or not. The assertion could not fail, which is worse than a missing test because it reads as coverage. iOS was unaffected -- it matches the Pro wordmark in the label rather than the element's presence. NOT verified end to end: the seeder is currently failing to find freshly created accounts on the local devnet, so the spec cannot reach the assertion. The locator targets the element that is conditional on Pro, which is the property the old one lacked.
Both elements now carry unique identifiers in the apps, so neither locator needs to scope by parent. That removes the last xpath and -android uiautomator selectors from this file. The two ids they replace were deliberately shared -- `pro-badge-text` is on conversation list rows too, `action-item-subtitle` on every settings row -- so the traversal was disambiguating rather than selecting. A reviewer flagged that as fragile and likely to break if these specs are reused for a future client, which is fair: it encoded a layout relationship rather than identity. The iOS expiry matches on `label` rather than `text`, because the new identifier takes `name` and displaces the rendered duration onto `label`.
`adb emu kill` alone left `create-emulators` unable to boot afterwards, and the gap cost real debugging time twice: a stale emulator drifts into producing failures that look exactly like product bugs, so "restart the emulators" is a common remedy -- and it silently did not work, so the phantom failures appeared to survive the fix. Three things needed doing by hand each time. Processes survive the console kill and hold the ports. The adb server is left wedged, reporting no devices even once emulators are up. And `hardware-qemu.ini.lock` / `multiinstance.lock` outlive the process, after which a fresh emulator boots but never registers -- presenting as "did not report boot_completed", which reads as a broken image rather than a stale lock. All three are scoped to the suite's own AVD prefix, so a developer's emulators and locks are untouched.
The animation check sampled whatever was on screen, so an avatar still showing the generated placeholder failed as "not animated" -- which is true and useless. It is an upload that has not landed yet, not an animation problem, and the two have different owners. Worse, the placeholder colour is picked by `sha512(address) % 7`, so a fresh account shows a different one every run. That made a timing race look like a per-build regression: it was bisected to an unrelated Android commit on the strength of a single passing run before repeat runs showed it was flaky all along. Now the check waits for the placeholder to go, and the two failures read differently: a palette colour says the picture never loaded, a static non-palette image says it loaded but was frozen because Pro was false when it composed. Three consecutive passes on a spec that was failing roughly one run in three.
…ompt From API 33 the POST_NOTIFICATIONS prompt fires the first time the app posts a notification -- whenever a message happens to arrive -- so it lands at an arbitrary point and covers whatever the spec is doing. The harness only handled it at one fixed moment during onboarding, so anything later was unhandled. It surfaced as "element not found" attributed to the step that happened to be running: three separate failures in one sweep were this one dialog, and it had also been misread as a Pro settings problem and as an avatar problem. Granting at app open removes the race rather than reacting to it. `handleNotificationPermissions` stays correct -- it only clicks the dialog if present, so it becomes a no-op. `setNotificationPermission` takes a boolean rather than being a one-way grant, so a spec that wants to assert the prompt can revoke and relaunch. That has to happen before the app is in use: revoking a granted runtime permission makes Android kill the process.
Appium's default `enforceAppInstall: false` skips installation when the package is already present at the same or newer version. Session's versionCode does not change between local builds, so a rebuilt APK silently never reached the device and specs ran against whatever was installed last — while ANDROID_APK, the filename and the version all still looked correct. Two app-side handovers were verified against a three-build-old binary before this was found. Nothing else forced a fresh install: uninstallApp exists but only two specs call it, so the only thing that ever did was recreating the AVDs, which wipes the app as a side effect. That is why "restart the emulators" appeared to cure so much.
…ers on both platforms - `Pro settings entry (expired)` asserted the expiry CTA after tapping into Pro settings, which had the causality backwards: the CTA is app-open behaviour that was still on screen by the time the spec navigated. On Android it also blocks the route to settings, so that shape could never have passed there. Now asserts the CTA at app open, dismisses it through its own Cancel button, then checks the renew row behind it. - New `ProRenewPlanRow` for `pro-settings-renew-plan`, which replaces the update-plan row in the expired state on both platforms. No app change was needed; the id already existed on each. - `Pro status checking state` and `Pro status error state` move from `iosIt` to `bothPlatformsIt` now that Android exposes `pro-settings-status-banner`. One of the two reasons they were parked was never true: the loading/error mock applies to a never-subscribed user, so only the missing id was real. - Correct the claim that these specs need no Pro backend. On Android the fixture overrides only the displayed type; refreshState comes from a real get_pro_status, so a stale TEST_PRO_BACKEND_ED_PK makes them fail looking like an app bug.
Applies the comment house rule to the comments this branch added.
- Drop narration of superseded spec shapes ("an earlier version did", "were iOS-only
until") and restate the same facts as current behaviour. Kept rather than deleted where
a blame reader lands on the superseded commit: `origin/main` is 60/60 merge commits, so
that history is permanent and reachable.
- Remove line-numbered citations into Session_Android; keep the symbols and the contract
they establish. Line numbers in another repo rot with nothing here able to detect it.
- Replace the reference to iOS's unmerged refresh-unification branch with the local
obligation it was standing in for: no mock here writes libSession config, so anything
gated on config is unreachable from this file.
- `android_config.ts` named `scripts/ci.sh` as a second declaration site; that file no
longer exists, so the warning now names the failure instead of the files.
…builds Preparing simulators runs a full `xcodebuild` of WebDriverAgent. On a machine where something else may be compiling a Session client at the same time, that is two native builds at once — the load pattern that pushes a host into swap and produces test timeouts indistinguishable from product bugs. `BUILD_LOCK_CMD` is an optional command prefix that runs the build under whatever mutex the host uses. Unset by default, so a plain checkout and CI are unaffected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Preparing simulators runs a full
xcodebuildof WebDriverAgent. On a machine where something else maybe compiling a Session client at the same time, that is two native builds at once — the load pattern
that pushes a host into swap and produces test timeouts indistinguishable from product bugs.
BUILD_LOCK_CMDis an optional command prefix that runs the build under whatever mutex the host uses.Unset by default, so a plain checkout and CI are unaffected.
Note: This is based on #124