ReviewOS
Reload

also looking at this

craft-native/craft

fix(ios): schedule a notification with the data the page gave it

#262
Merged glennmichael123 wants to merge fix/ios-notification-data into main
8 files +242 -22

Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.

scripts/mobile-e2e/driver.htmlmodified+14-0
@@ -18,6 +18,7 @@
1818 <p id="notification-permission"></p>
1919 <p id="notification"></p>
2020 <p id="notification-received"></p>
21 <p id="notification-local"></p>
2122 <script>
2223 // The page drives itself and reports through the bridge's own `log` action,
2324 // which is the one channel both runtimes already forward off-device: iOS
@@ -510,6 +511,10 @@
510511 // what onReceive handed it, and how many taps it had by then, since an
511512 // arrival is not a tap.
512513 //
514 // #258: opened by the local link, the page schedules a notification of
515 // its own, with data, a few seconds out. The test kills the app before it
516 // fires and taps it, and this same report says what the tap carried.
517 //
513518 // Permission is asked for only when the link asks, because nothing
514519 // answers the prompt on the launches simctl makes. iOS only: Android has
515520 // no tap path to test yet.
@@ -520,6 +525,15 @@
520525 var said = function (text) { document.getElementById('notification-permission').textContent = 'CRAFT-E2E-NOTIFY-PERMISSION ' + text }
521526 window.craft.permissions.request('notifications').then(said, function (error) { said('rejected ' + describeError(error).message) })
522527 }
528 if (launch && launch.url.indexOf('://e2e/local') !== -1) {
529 var scheduled = function (text) { document.getElementById('notification-local').textContent = 'CRAFT-E2E-LOCAL-SCHEDULED ' + text }
530 window.craft.notifications.schedule({
531 title: 'Craft E2E',
532 body: NONCE + ' local',
533 delay: 8000,
534 data: { craftE2E: NONCE, stage: 'local', nested: { kept: true } }
535 }).then(function () { scheduled('ok') }, function (error) { scheduled('rejected ' + describeError(error).message) })
536 }
523537 var onTap = window.craft.notifications && window.craft.notifications.onTap
524538 var seen = { dispatched: 0, taps: [], onTap: typeof onTap, unsubscribe: null }
525539 if (typeof onTap === 'function') {
scripts/mobile-e2e/ios-uitests/NotificationTapColdStartTests.swiftmodified+40-10
@@ -1,8 +1,9 @@
11import XCTest
22
3/// Taps a pushed notification that launches the probe app, then has a second
4/// push arrive while it is open, and reports what its page was handed each
5/// time (#255, #256).
3/// Taps a pushed notification that launches the probe app, has a second push
4/// arrive while it is open, then taps a local notification the page scheduled
5/// with data, and reports what its page was handed each time (#255, #256,
6/// #258).
67///
78/// A person's sequence, end to end: the app asks for permission and is
89/// allowed, the app is killed, a real push arrives through `simctl push`, and
@@ -10,6 +11,8 @@ import XCTest
1011/// `simctl` runs on the host, so the test prints `CRAFT-E2E-PUSH-READY cold`
1112/// once the app is gone and `CRAFT-E2E-PUSH-READY foreground` once the page
1213/// is listening, and the harness sends each push when it reads the line.
14/// Last, a link has the page schedule a notification of its own, the app is
15/// killed before it fires, and its banner is tapped.
1316///
1417/// As with the deep-link test, no assertions about the answer live here. The
1518/// page's report is printed and `scripts/mobile-e2e/protocol.ts` judges it.
@@ -18,6 +21,8 @@ final class NotificationTapColdStartTests: XCTestCase {
1821 private var bundleId: String { environment["PROBE_BUNDLE_ID"] ?? "" }
1922 private var permissionLink: String { environment["PROBE_NOTIFY_LINK"] ?? "" }
2023 private var pushBody: String { environment["PROBE_PUSH_BODY"] ?? "" }
24 private var localLink: String { environment["PROBE_LOCAL_LINK"] ?? "" }
25 private var localBody: String { environment["PROBE_LOCAL_BODY"] ?? "" }
2126
2227 override func setUp() {
2328 // Each step stands on the one before it; carrying on past a failure
@@ -25,10 +30,12 @@ final class NotificationTapColdStartTests: XCTestCase {
2530 continueAfterFailure = false
2631 }
2732
28 func testPushesReachThePage() throws {
33 func testNotificationsReachThePage() throws {
2934 XCTAssertFalse(bundleId.isEmpty, "PROBE_BUNDLE_ID is not set")
3035 XCTAssertFalse(permissionLink.isEmpty, "PROBE_NOTIFY_LINK is not set")
3136 XCTAssertFalse(pushBody.isEmpty, "PROBE_PUSH_BODY is not set")
37 XCTAssertFalse(localLink.isEmpty, "PROBE_LOCAL_LINK is not set")
38 XCTAssertFalse(localBody.isEmpty, "PROBE_LOCAL_BODY is not set")
3239
3340 let app = XCUIApplication(bundleIdentifier: bundleId)
3441 let springboard = XCUIApplication(bundleIdentifier: "com.apple.springboard")
@@ -60,7 +67,8 @@ final class NotificationTapColdStartTests: XCTestCase {
6067 // case that always worked.
6168 app.terminate()
6269 XCTAssertTrue(app.wait(for: .notRunning, timeout: 15), "the app did not stop, so this would not be a cold start")
63 tapColdPush(springboard, app: app)
70 print("CRAFT-E2E-PUSH-READY cold")
71 tapBanner(springboard, app: app, containing: pushBody)
6472
6573 let report = app.staticTexts
6674 .containing(NSPredicate(format: "label BEGINSWITH 'CRAFT-E2E-NOTIFICATION '"))
@@ -84,9 +92,32 @@ final class NotificationTapColdStartTests: XCTestCase {
8492 .firstMatch
8593 _ = arrived.waitForExistence(timeout: 30)
8694 print("CRAFT-E2E-NOTIFICATION-RECEIVED-RESULT \(receipts.label)")
95
96 // #258: a notification the page scheduled itself, with data. The
97 // local link has the page schedule it a few seconds out, and the app
98 // is killed before it fires, so its tap is a cold start like the push.
99 app.terminate()
100 XCTAssertTrue(app.wait(for: .notRunning, timeout: 15), "the app did not stop before the local link")
101 XCUIDevice.shared.system.open(URL(string: localLink)!)
102 if open.waitForExistence(timeout: 5) { open.tap() }
103 XCTAssertTrue(app.wait(for: .runningForeground, timeout: 60), "the local link did not start the app")
104 let scheduled = app.staticTexts
105 .containing(NSPredicate(format: "label BEGINSWITH 'CRAFT-E2E-LOCAL-SCHEDULED '"))
106 .firstMatch
107 XCTAssertTrue(scheduled.waitForExistence(timeout: 60), "the page never said whether it scheduled the notification")
108 XCTAssertEqual(scheduled.label, "CRAFT-E2E-LOCAL-SCHEDULED ok")
109 app.terminate()
110 XCTAssertTrue(app.wait(for: .notRunning, timeout: 15), "the app did not stop, so the local tap would not be a cold start")
111 tapBanner(springboard, app: app, containing: localBody)
112
113 let local = app.staticTexts
114 .containing(NSPredicate(format: "label BEGINSWITH 'CRAFT-E2E-NOTIFICATION '"))
115 .firstMatch
116 XCTAssertTrue(local.waitForExistence(timeout: 120), "the page never reported what the local notification's tap handed it")
117 print("CRAFT-E2E-LOCAL-RESULT \(local.label)")
87118 }
88119
89 /// Asks the host for the cold push and taps its banner.
120 /// Taps the banner of a notification that has arrived or is about to.
90121 ///
91122 /// The banner, not Notification Center: on iOS 26 Notification Center is
92123 /// pulled down as the Lock Screen, and a tap there only offers Open. The
@@ -99,12 +130,11 @@ final class NotificationTapColdStartTests: XCTestCase {
99130 /// enough for the banner to go, and the test fails saying the tap could
100131 /// not find it. There is no retrying around that: a tap that misses ends
101132 /// the test method, even inside XCTExpectFailure.
102 private func tapColdPush(_ springboard: XCUIApplication, app: XCUIApplication) {
133 private func tapBanner(_ springboard: XCUIApplication, app: XCUIApplication, containing text: String) {
103134 let banner = springboard.descendants(matching: .any)
104 .matching(NSPredicate(format: "label CONTAINS %@", pushBody))
135 .matching(NSPredicate(format: "label CONTAINS %@", text))
105136 .firstMatch
106 print("CRAFT-E2E-PUSH-READY cold")
107 XCTAssertTrue(banner.waitForExistence(timeout: 120), "no banner for the push ever appeared")
137 XCTAssertTrue(banner.waitForExistence(timeout: 120), "no banner for \(text) ever appeared")
108138 banner.tap()
109139 XCTAssertTrue(app.wait(for: .runningForeground, timeout: 60), "tapping the banner did not start the app")
110140 }
scripts/mobile-e2e/ios.tsmodified+13-5
@@ -3,7 +3,7 @@ import type { LegOutcome, RunnerOptions } from './types'
33import { copyFileSync, existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs'
44import { join } from 'node:path'
55import { bootSimulator, init, pickSimulator } from '../../packages/ios/src/index'
6import { deepLinkProblems, deepLinkResults, evaluateRun, hasTerminated, notificationBody, notificationPayload, notificationReceiptProblems, notificationReceiptReport, notificationTapProblems, notificationTapReport, ZIG_REFUSED_ACTIONS, ZIG_SERVED_ACTIONS, ZIG_TESTED_ACTIONS, zigDispatchedActions, zigHandBacks, zigRefusals } from './protocol'
6import { deepLinkProblems, deepLinkResults, evaluateRun, hasTerminated, localNotificationProblems, notificationBody, notificationPayload, notificationReceiptProblems, notificationReceiptReport, notificationTapProblems, notificationTapReport, ZIG_REFUSED_ACTIONS, ZIG_SERVED_ACTIONS, ZIG_TESTED_ACTIONS, zigDispatchedActions, zigHandBacks, zigRefusals } from './protocol'
77import { command, driverPage, waitForFile } from './support'
88
99/** The scheme the probe registers, and the one its cold-start link uses. */
@@ -38,6 +38,9 @@ const CONFIG = {
3838 enableSpeechRecognition: false,
3939 // For the cold-start link (#198), which the harness opens from an XCUITest.
4040 enableDeepLinks: true,
41 // For the notification the page schedules with data (#258), served by
42 // Swift on the shim leg and by Zig on the runtime leg.
43 enableLocalNotifications: true,
4144 urlSchemes: [DEEP_LINK_SCHEME],
4245}
4346
@@ -68,8 +71,8 @@ const UI_TEST_CLASSES = ['DeepLinkColdStartTests', 'NotificationTapColdStartTest
6871
6972/**
7073 * How long the notification UI test gets in all. It starts a test runner,
71 * launches the app twice and answers a permission prompt; it usually takes
72 * under half a minute.
74 * launches the app four times, answers a permission prompt and waits out a
75 * scheduled notification; it usually takes about a minute.
7376 */
7477const NOTIFICATION_TEST_TIMEOUT_MS = 300_000
7578
@@ -340,6 +343,8 @@ schemes:
340343 TEST_RUNNER_PROBE_BUNDLE_ID: BUNDLE_ID,
341344 TEST_RUNNER_PROBE_NOTIFY_LINK: `${DEEP_LINK_SCHEME}://e2e/notify?run=${encodeURIComponent(nonce)}`,
342345 TEST_RUNNER_PROBE_PUSH_BODY: notificationBody(nonce, 'cold'),
346 TEST_RUNNER_PROBE_LOCAL_LINK: `${DEEP_LINK_SCHEME}://e2e/local?run=${encodeURIComponent(nonce)}`,
347 TEST_RUNNER_PROBE_LOCAL_BODY: notificationBody(nonce, 'local'),
343348 },
344349 stdin: 'ignore',
345350 stdout: Bun.file(notificationLog),
@@ -368,10 +373,13 @@ schemes:
368373 }
369374 else {
370375 failures.push(...notificationTapProblems(notificationTapReport(notificationOutput), nonce, notificationEvidence))
371 if (!pushed.includes('foreground'))
376 if (!pushed.includes('foreground')) {
372377 failures.push(`the notification UI test never got to the push that arrives while the app is open; see ${notificationEvidence}`)
373 else
378 }
379 else {
374380 failures.push(...notificationReceiptProblems(notificationReceiptReport(notificationOutput), nonce, notificationEvidence))
381 failures.push(...localNotificationProblems(notificationTapReport(notificationOutput, 'CRAFT-E2E-LOCAL-RESULT'), nonce, notificationEvidence))
382 }
375383 }
376384
377385 const dispatched = zigDispatchedActions(consoleText)
scripts/mobile-e2e/protocol.test.tsmodified+43-1
@@ -1,7 +1,7 @@
11import { describe, expect, it } from 'bun:test'
22import { readFileSync } from 'node:fs'
33import { join } from 'node:path'
4import { ANDROID_DECLINE_PHRASES, androidDeclines, awaitedNeeds, deepLinkProblems, deepLinkReports, deepLinkResults, DISMISS_SHARE_MENU, elfSectionNames, evaluateRun, hasTerminated, NOTIFICATION_ROUTE, notificationBody, notificationPayload, notificationReceiptProblems, notificationReceiptReport, notificationTapProblems, notificationTapReport, parseDriverOutput, REQUIRED_CASES, requiredCaseProblems, runtimePermissionGranted, shareMenuInFront, strippedLibraryProblems, ZIG_REFUSED_ACTIONS, ZIG_SERVED_ACTIONS, ZIG_TESTED_ACTIONS, zigDispatchedActions, zigHandBacks, zigRefusals } from './protocol'
4import { ANDROID_DECLINE_PHRASES, androidDeclines, awaitedNeeds, deepLinkProblems, deepLinkReports, deepLinkResults, DISMISS_SHARE_MENU, elfSectionNames, evaluateRun, hasTerminated, localNotificationProblems, NOTIFICATION_ROUTE, notificationBody, notificationPayload, notificationReceiptProblems, notificationReceiptReport, notificationTapProblems, notificationTapReport, parseDriverOutput, REQUIRED_CASES, requiredCaseProblems, runtimePermissionGranted, shareMenuInFront, strippedLibraryProblems, ZIG_REFUSED_ACTIONS, ZIG_SERVED_ACTIONS, ZIG_TESTED_ACTIONS, zigDispatchedActions, zigHandBacks, zigRefusals } from './protocol'
55
66const ESC = String.fromCharCode(27)
77
@@ -435,6 +435,48 @@ describe('notification tap cold start', () => {
435435 })
436436})
437437
438describe('tap on a notification the page scheduled with data', () => {
439 const RUN = 'craft-e2e-ios-runtime-1'
440 const tap = { craftE2E: RUN, stage: 'local', nested: { kept: true } }
441 const pushed = { craftE2E: RUN, stage: 'cold', route: NOTIFICATION_ROUTE }
442 const output = (report: object) => [
443 `CRAFT-E2E-NOTIFICATION-RESULT CRAFT-E2E-NOTIFICATION ${JSON.stringify({ dispatched: 1, taps: [pushed], onTap: 'function', unsubscribe: 'function' })}`,
444 `CRAFT-E2E-LOCAL-RESULT CRAFT-E2E-NOTIFICATION ${JSON.stringify(report)}`,
445 ].join('\n')
446 const judge = (report: object) => localNotificationProblems(notificationTapReport(output(report), 'CRAFT-E2E-LOCAL-RESULT'), RUN)
447
448 it('passes a tap that hands back the data it was scheduled with, nested values included', () => {
449 expect(judge({ dispatched: 1, taps: [tap], onTap: 'function', unsubscribe: 'function' })).toEqual([])
450 })
451
452 // The bug: neither scheduler set userInfo.
453 it('fails a tap that hands the page {}', () => {
454 expect(judge({ dispatched: 1, taps: [{}], onTap: 'function', unsubscribe: 'function' }))
455 .toEqual(['a tap on a scheduled notification handed the page {}: the data it was scheduled with never reached userInfo'])
456 })
457
458 it('fails lost nesting, another run\'s data, a double delivery, and a tap never dispatched', () => {
459 expect(judge({ dispatched: 1, taps: [{ ...tap, nested: '[object Object]' }], onTap: 'function', unsubscribe: 'function' })[0]).toContain('nested data did not survive')
460 expect(judge({ dispatched: 1, taps: [{ ...tap, craftE2E: 'an-earlier-run' }], onTap: 'function', unsubscribe: 'function' })[0]).toContain('expected the data it was scheduled with once')
461 expect(judge({ dispatched: 1, taps: [tap, tap], onTap: 'function', unsubscribe: 'function' })[0]).toContain('expected the data it was scheduled with once')
462 expect(judge({ dispatched: 0, taps: [], onTap: 'function', unsubscribe: 'function' })).toEqual(['the app never dispatched the tap on the scheduled notification'])
463 })
464
465 it('reads each tap report by its own label', () => {
466 // The push's report is not the scheduled notification's, and a run that
467 // never got to the local stage has no local report at all.
468 expect(notificationTapReport(output({ dispatched: 1, taps: [tap] }))?.taps).toEqual([pushed])
469 expect(notificationTapReport(output({ dispatched: 1, taps: [tap] }), 'CRAFT-E2E-LOCAL-RESULT')?.taps).toEqual([tap])
470 expect(localNotificationProblems(notificationTapReport(output({}).split('\n')[0]!, 'CRAFT-E2E-LOCAL-RESULT'), RUN))
471 .toEqual(['the tap on the scheduled notification never reported; see xcodebuild-notification.log'])
472 })
473
474 it('finds the local banner by text the push banners do not share', () => {
475 expect(notificationBody(RUN, 'local')).toBe(`${RUN} local`)
476 expect(notificationBody(RUN, 'cold')).not.toContain(notificationBody(RUN, 'local'))
477 })
478})
479
438480describe('notification that arrives while the app is open', () => {
439481 const RUN = 'craft-e2e-ios-shim-1'
440482 const arrival = { aps: { alert: { title: 'Craft E2E', body: `${RUN} foreground` } }, craftE2E: RUN, stage: 'foreground', route: NOTIFICATION_ROUTE }
scripts/mobile-e2e/protocol.tsmodified+39-6
@@ -453,8 +453,11 @@ export const NOTIFICATION_ROUTE = '/e2e/notification-tap'
453453 */
454454export type PushStage = 'cold' | 'foreground'
455455
456/** The banner text of a stage's push, which the UI test finds the banner by. */
457export function notificationBody(run: string, stage: PushStage): string {
456/**
457 * The banner text of a stage's notification, which the UI test finds the
458 * banner by: a pushed stage, or `local`, the one the page schedules (#258).
459 */
460export function notificationBody(run: string, stage: PushStage | 'local'): string {
458461 return `${run} ${stage}`
459462}
460463
@@ -485,12 +488,15 @@ export interface NotificationTapReport {
485488 * The page's report the UI test printed, read out of xcodebuild's output.
486489 *
487490 * `ios-uitests/NotificationTapColdStartTests.swift` writes one
488 * `CRAFT-E2E-NOTIFICATION-RESULT CRAFT-E2E-NOTIFICATION {json}` line. None,
489 * or one whose JSON does not parse, is null: the test never got that far.
491 * `<label> CRAFT-E2E-NOTIFICATION {json}` line per tap: labelled
492 * `CRAFT-E2E-NOTIFICATION-RESULT` for the push, `CRAFT-E2E-LOCAL-RESULT` for
493 * the scheduled notification. None, or one whose JSON does not parse, is
494 * null: the test never got that far.
490495 */
491export function notificationTapReport(output: string): NotificationTapReport | null {
496export function notificationTapReport(output: string, label = 'CRAFT-E2E-NOTIFICATION-RESULT'): NotificationTapReport | null {
492497 for (const line of output.split('\n')) {
493 const match = line.match(/CRAFT-E2E-NOTIFICATION-RESULT CRAFT-E2E-NOTIFICATION (\{.*\})\s*$/)
498 const at = line.indexOf(`${label} CRAFT-E2E-NOTIFICATION `)
499 const match = at === -1 ? null : line.slice(at + label.length + 1).match(/^CRAFT-E2E-NOTIFICATION (\{.*\})\s*$/)
494500 if (!match) continue
495501 try {
496502 const report = JSON.parse(match[1]!) as NotificationTapReport
@@ -534,6 +540,33 @@ export function notificationTapProblems(
534540 return problems
535541}
536542
543/**
544 * Why the tap on the notification the page scheduled is not a pass (#258).
545 *
546 * The page scheduled it with `data`, and the tap must hand that back once,
547 * nested values included. The bug handed the page `{}`: neither scheduler put
548 * `data` into userInfo, so there was nothing for the tap to carry.
549 */
550export function localNotificationProblems(
551 report: NotificationTapReport | null,
552 run: string,
553 evidence = 'xcodebuild-notification.log',
554): string[] {
555 if (!report)
556 return [`the tap on the scheduled notification never reported; see ${evidence}`]
557 if (report.dispatched === 0)
558 return ['the app never dispatched the tap on the scheduled notification']
559 if (report.taps.length === 1 && Object.keys(report.taps[0]!).length === 0)
560 return ['a tap on a scheduled notification handed the page {}: the data it was scheduled with never reached userInfo']
561
562 const ours = report.taps.filter(tap => tap.craftE2E === run && tap.stage === 'local')
563 if (ours.length !== 1 || report.taps.length !== 1)
564 return [`a tap on the scheduled notification handed the page ${JSON.stringify(report.taps)}, expected the data it was scheduled with once`]
565 if ((ours[0]!.nested as { kept?: unknown } | undefined)?.kept !== true)
566 return [`the scheduled notification's nested data did not survive: it carried ${JSON.stringify(ours[0])}`]
567 return []
568}
569
537570/** What a page that was open when a push arrived was handed by onReceive. */
538571export interface NotificationReceiptReport {
539572 received: Record<string, unknown>[]