test(pro): the Pro transfer warning on Clear All Data - #157
Closed
Bilb wants to merge 9 commits into
Closed
Conversation
Pro-specific copy on an irreversible flow, read by nothing until now. It is the only thing telling a subscriber their Pro does not survive the wipe, and it is a second-stage dialog: opening Clear Data shows the generic copy, pressing Clear re-renders the same slot with the warning, and only a further press deletes anything. Every case here reads the copy and cancels - the destructive action is never taken. Desktop gets all four cells of the grid the app actually branches on (`deleteMode` crossed with `useCurrentUserHasPro()`). Each Pro case asserts two runs of copy because neither alone identifies a cell: the transfer warning is word-for-word identical in both Pro tokens, and the network branch's opening sentence is word-for-word `clearDeviceAndNetworkConfirm`, the standard copy. Android gets the device branch only, and no standard-account control - `SettingsViewModel` calls `clearDataDeviceOnly()` straight from the first Clear press for a standard account, so the control would wipe the app rather than assert anything. iOS gets nothing. See below. `localizedRuns` is new and is the reason these assert runs rather than tokens. `tStripped` collapses a `<br/>` to a single space while every client renders one as no character at all, so the stripped string is a run of text that exists nowhere in the UI and can never match. Both Pro tokens span two breaks. This splits the raw token on its breaks and returns the stripped runs, so the assertion still comes from the localizer rather than a hardcoded English string. Two client-side tag gaps this could not work around, both worth fixing: - iOS has no test identifier anywhere in the flow. The settings row is a `ListItemInfo` built with no `accessibility:`, and `NukeDataModal` sets none on its radios, its buttons or its labels; the `ConfirmationModal` carrying the copy gets only an `accessibilityLabel`. Nothing here is addressable by identifier, so there is no iOS spec. - Android's two clear-data radios are `RadioOption`s built with no `qaTag` in `SettingsScreen.kt`, so the network branch cannot be selected. Device-only is the default selection, which is the only reason that half is reachable. The Android Clear/Cancel ids are the English display strings, because `AlertDialog` falls back to a button's own text when the call site gives it no `qaTag`. Real ids rather than text matches, but ones that would move with the locale - noted on the locators. Verified: 4/4 desktop (12.0s), 1/1 android (30.1s).
The clients now tag what this needed, so the mobile spec becomes `bothPlatformsIt` and gains the network branch and a control. iOS was previously unreachable: the settings row carried no identifier and neither did anything in `NukeDataModal`. Android's two radios had no `qaTag`, so only the device branch - the default selection - could be reached. Both are fixed in the client repos; nothing here works around them. One thing that only shows up on a device. The two stages are the same dialog on Android, whose text swaps, but two STACKED modals on iOS: the confirmation is presented over `NukeDataModal` rather than replacing it, so both sit in the accessibility tree at once. Giving them the same identifiers had the locator match whichever came first - the one underneath - and all three iOS specs failed asserting the confirmation copy while reading the pre-confirmation copy. Hence `ClearDataDialogDescription` and the iOS half of `ClearDataConfirmButton`, which name the first stage only. Adds a standard-account control on the NETWORK branch. There cannot be one on the device branch: both clients delete straight from the first Clear press there, with no confirmation to read. Verified on all three: iOS 3/3 (31.5s, 31.7s, 31.1s), Android 3/3 (29.7s, 28.3s, 28.4s), Desktop 4/4 (18.2s). The iOS run needed a local build of the client change - the prebuilt app predates it.
An id says the client rendered the right control; only the copy says it rendered the right words in it, and the two fail independently - a control keeps its identifier through a copy change, so an id-only lookup stays green against a wrong, empty or swapped string. On a flow whose two actions are Clear and Cancel that is the difference between reading a warning and wiping the account. So every control this spec presses is now checked both ways, and CLAUDE.md carries the rule for everything after it. Where the copy lives is per-platform, and finding that out took a device: - iOS puts it on `label`; an identifier becomes the element's `name` and displaces the display text. - Desktop takes `text`, already a substring match. - Android Compose CONTROLS report no text of their own - the label is a child node, so the node addressed by id has nothing to compare. Verified from a page-source dump: the Clear button resolves to a node with no text, while a sibling `TextView` carries "Clear". Only text-bearing nodes (a dialog body, a heading) can be checked in place. `expectControlCopy` does that split and skips loudly on Android rather than passing quietly. Where an Android id is itself derived from the display string - `AlertDialog` falls back to a button's own text with no `qaTag` - the id lookup already covers the copy, and the comment says so. Also corrects the standard-account note. Desktop confirms for every account on both branches; Android and iOS confirm on both branches only for a PRO account, and send a standard account's device-only clear straight to deletion. Confirmed on Android by running that case once - the app came back on the onboarding screen. The iOS half of that is read from `clearDeviceOnly()`, not observed. Verified with the assertions in place: iOS 3/3, Android 3/3, Desktop 4/4.
Two the previous commit wrote the rule for and then did not follow. `ClearDataMenuItem` was tapped on its id alone, three call sites and no copy check anywhere - and it is the case the rule exists for, since its id is a hand-written tag rather than the display string, so the lookup says nothing about what the row reads. `ClearDeviceOnlyRadio` was used zero times: added speculatively and never wired up. It now carries its half of the first-stage assertion, so both branches the dialog offers are checked rather than only the one a given case goes on to tap. The preselected radio is never pressed, so nothing else would have read it. Each locator's doc now says whether its id carries the copy, so the next caller does not have to work it out: hand-written tag (the row), slug (the radios), or derived from the display string (Android's dialog buttons, where the lookup already covers it). Verified: iOS 3/3 - the platform these assertions actually bite on, since Android's Compose labels are child nodes and `expectControlCopy` skips there.
Drops the three standard-account cases, leaving two per platform. The mobile one was written against behaviour that is being removed: both clients used to delete straight from the first Clear press on the device branch, and now confirm for every account as Desktop always has. Testing the old shape would have been testing something on its way out. The two desktop ones were duplicates. `clearDataOnWindow` in `linked_device_group.spec.ts` already walks the same modal on a standard account and asserts `clearDeviceDescription` on the confirmation, via `checkModalStrings` - which is an exact match, so it also rules the Pro copy out. What that costs, stated so it is a decision and not an oversight: nothing left here separates "shows the Pro copy to Pro users" from "shows the Pro copy to everyone". Worth a control once the mobile behaviour has settled, and cheap to add then. Verified: iOS 2/2, Android 2/2, Desktop 2/2.
`pressWithCopy` reinvented `clickOnWithText`, and the hand-rolled `waitForElement` on `modal-description` reinvented `checkModalStrings` - which is the better of the two anyway, since it scopes to `[data-modal-id="deleteAccountModal"]` and pins the heading at the same time. A bare `modal-description` wait would be satisfied by any modal carrying that slot. Both were sitting in `clearDataOnWindow` (`linked_device_group.spec.ts`) the whole time. The consequence is the interesting part: `checkModalStrings` reads `innerText`, where a `<br/>` renders as a newline, and then collapses whitespace - landing on exactly what `tStripped` produces for the same token. So the `<br/>`-spanning problem is a MOBILE one, and Desktop can assert whole tokens. `localizedRuns` is gone from this spec; keeping it would have been carrying a workaround for another platform's constraint. Verified against `proClearAllDataDevice`, which spans two breaks. Both cases still pass, and faster: 3.6s and 2.6s against 6.1s and 3.4s. CLAUDE.md records both - reach for the desktop primitives, and the `<br/>` rule is mobile-only.
Adds the standard-account device case back, on all three platforms, now that it exists to test. It could not before on mobile: a standard account pressing Clear with device-only selected had its data deleted on that press - Android's `SettingsViewModel.clearData` fell through to `clearDataDeviceOnly()`, iOS's `clearDeviceOnly()` to `clearLocalAccount()` - so there was no confirmation to read and the test would have wiped the app instead of asserting. The client PRs change that, and this is what stops it regressing to a one-tap wipe: reaching the assertion at all means the confirmation was there. It also closes the gap the previous commit deliberately left open. With Pro cases alone, nothing separated "shows the Pro copy to Pro users" from "shows the Pro copy to everyone". Device branch only. That is the branch the clients changed, and the same `useCurrentUserHasPro()` drives both, so a network control would pin nothing new. No existing spec needed updating: this is the only mobile spec that clears data, and the three desktop ones that touch the flow (`delete_account`, `linked_device_group`, `enforce_localized_str`) sit on behaviour that did not change. Verified: iOS 3/3, Android 3/3, Desktop 3/3.
`localizedRuns` existed because a copy spanning a `<br/>` can never match a locator's text filter: `tStripped` puts a single space where the break was, and the rendered value has a newline (mobile) or no character at all (a DOM `textContent`). Splitting the token into runs was one way round that. Collapsing the whitespace of the value being compared is a better one. `readDialogBody` now does it, and the specs assert the WHOLE `tStripped` token - which pins more than a fragment did, since a fragment could not say the two halves were in the right order or that anything sat between them. Verified on both mobile platforms; Desktop already got this for free from `checkModalStrings`, which reads `innerText` and normalises the same way. So the helper is gone rather than mobile-only, and CLAUDE.md now describes the normalisation instead of pointing at a file that no longer exists. Also completes the grid. Each of the four cells - `deleteMode` crossed with `useCurrentUserHasPro()` - carries its own token, so the standard-account NETWORK case is the only thing asserting the copy that warns a standard user their messages cannot be restored. `delete_account.spec.ts` walks that branch but never reads the confirmation. Previously left out on the grounds that it "pins nothing new", which was wrong. Verified: iOS 4/4, Android 4/4, Desktop 4/4.
Two things, both replacing hand-rolled work with harness machinery that already existed. `readDialogBody` reimplemented the whitespace normalisation the matchers already do. `findMatchingTextInElementArray` and `findMatchingLabelInElementArray` both collapse whitespace before comparing, which is exactly what makes copy spanning a `<br/>` comparable to the single space `tStripped` leaves in its place. So the locator's own `text`/`label` filter does the whole job, and `withCopy` picks the right one per platform - `label` on iOS, where an identifier displaces the display text; `text` on Android, where a text-bearing node still carries its own. That match is EXACT rather than a substring, which is strictly stronger and removes the separate "and not the Pro copy" assertions: a body that equals `clearDeviceDescription` cannot also contain the transfer warning. `withCopy` sits beside `expectControlCopy`, and the pair is the real distinction - a text-bearing node can be checked on both platforms, a Compose control only on iOS. Separately, `cancelClearData` was passing `PRESENT_MAX_WAIT` to `verifyElementNotPresent`, which sleeps its `maxWait` UNCONDITIONALLY before looking. That was a flat ten seconds per case rather than a bound a fast dismiss escapes. On ABSENT_MAX_WAIT the Android cases drop from ~29s to ~20s. Verified against builds carrying the client behaviour change, not dev: iOS 4/4, Android 4/4.
Collaborator
Author
|
Superseded by #165 and closed in its favour. Nothing is dropped: all 12 clear-data tests are in that branch, verified 4/4 on each of Desktop, Android and iOS against clients built from the merged Merging it there rather than here because #159 depends on #164 and all three touch the same state-builder plumbing — they conflict pairwise, so one branch is both easier to review and the only way to run them together. |
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.
Reads the Pro-specific warning on Clear All Data — the only thing that tells a subscriber their Pro
does not survive the wipe. Nothing tested it before.
Every case reads the copy and cancels. The destructive action is never taken.
Coverage — 12 tests, all passing
The app picks the copy from a two-by-two:
deleteModecrossed withuseCurrentUserHasPro(). Eachcell has its own token, so each is asserted on each platform.
proClearAllDataDeviceproClearAllDataNetworkclearDeviceDescriptionclearDeviceAndNetworkConfirmDesktop 4/4 in 12.3s, Android 4/4 in 2.0m, iOS 4/4 in 2.4m.
The standard-account device case could not exist on mobile before this work: pressing Clear there
deleted the account on that press, so the test would have wiped the app instead of asserting.
Reaching its assertion is now the enforcement that the confirmation exists. The standard network
case is the only thing anywhere asserting the "you will not be able to restore your messages" copy —
delete_account.spec.tswalks that branch but never reads the confirmation.No existing spec needed updating. This is the only mobile spec that clears data, and the three desktop
ones that touch the flow (
delete_account,linked_device_group,enforce_localized_str) sit onbehaviour that did not change.
Depends on two client PRs
data deleted on the first press, with no confirmation. Every other combination on every platform
asks first. Verified both directions on a device: before, the app came back on the onboarding
screen; after, the confirmation shows and Cancel leaves the account intact.
The tag PRs this also needed — session-ios #763 and session-android #2195 — have merged.
Address by id, then assert the copy
New rule, in CLAUDE.md and applied here. An id says the client rendered the right control; only the
copy says it rendered the right words. They fail independently, so an id-only lookup stays green
against a wrong, empty or swapped string — and on a flow whose two actions are Clear and Cancel, that
is the difference between reading a warning and wiping the account.
Where the copy lives is per-platform, and finding that out took a device:
label; an identifier becomes the element'snameand displaces the display text.clickOnWithTextandcheckModalStrings, both of which already existed.button resolves to a node with no text, while a sibling
TextViewcarries "Clear". Onlytext-bearing nodes can be checked in place, so
expectControlCopyskips loudly there.Copy that spans a
<br/>tStrippedputs a single space where the break was; the rendered value has a newline (mobile) or nocharacter at all (a DOM
textContent). So a locator's text filter never matches, and both Pro tokensspan two breaks.
The fix is to collapse the whitespace of the value being compared and assert the whole token —
which pins more than a fragment, since a fragment cannot say the halves are in the right order or that
nothing sits between them. Desktop gets this free from
checkModalStrings, which readsinnerTextand normalises the same way.
One thing worth reading the locators for
The two stages are the same dialog on Android, whose text swaps — but two stacked modals on iOS,
where the confirmation is presented over
NukeDataModalrather than replacing it. Both sit in theaccessibility tree at once, so shared identifiers had the locator match the one underneath and every
iOS spec asserted the confirmation copy while reading the pre-confirmation copy. Hence
ClearDataDialogDescriptionand the iOS half ofClearDataConfirmButton, which name the first stageonly.