test(analytics-react-native): native iOS XCTest coverage for connectivity module (SDKRN-5) [2.1/5]#1823
Merged
Merged
Conversation
… (SDKRN-5) PR 1 of a stacked series splitting the SDKRN-5 offline feature for review. Adds the cross-platform pieces of offline mode: - `networkConnectivityCheckerPlugin` (BeforePlugin): seeds and flips `config.offline` from the native connectivity module (or `navigator.onLine` on web), flushing on reconnect; falls back to online when no connectivity source is available. - Client wiring in `_init` (installs before `Destination`, skips on the `OfflineDisabled` sentinel). - Plugin unit tests + end-to-end `offline-integration.test.ts` (native bridge mocked in the jest setups). The native `AmplitudeReactNativeConnectivity` module ships here as a **no-op placeholder** on both platforms (always reports connected, never emits), so the SDK is fully runnable after this PR — offline mode is simply a no-op (identical to pre-feature behavior). Follow-up PRs replace the iOS and Android stubs with the real `NWPathMonitor` / `ConnectivityManager` implementations. Verified: analytics-react-native typecheck + lint clean; jest 106/106 pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… type Stop omitting `offline` from the `ReactNativeConfig` interface now that RN supports offline mode. The property always existed on the underlying core `Config` instance at runtime; the type omission only forced casts. This removes the `OfflineConfig` intersection in the connectivity plugin and the inline cast in the client, and makes `offline` (including the `OfflineDisabled` sentinel) a typed, public init option via ReactNativeOptions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The connectivity handler compares incoming state against the current `config.offline` and skips no-op updates; it is a state-transition guard, not a time-windowed debounce. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Guard the web `online` handler so it only flushes on an actual offline->online transition (mirrors the native path); add a test. - Drop now-stale "offline is omitted from the public type" comments and the casts they justified, since `offline` is now typed on ReactNativeConfig/ReactNativeOptions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Mirror the native path on both directions: the web `offline` handler now early-returns when already offline, so repeated `offline` events are no-ops (matching the `online` guard). Add a test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the fixed setTimeout(50) wait (timing-dependent, flaky on slow CI) with fake timers. Because the react-native jest env deadlocks the async timer helpers (runAllTimersAsync/advanceTimersByTimeAsync) on its setImmediate polyfill, drain the Timeline's setTimeout(0) apply chain by alternating jest.runOnlyPendingTimers() with microtask flushes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…imers Replace the timer-draining loop with an event-driven wait: spy on storageProvider.set and resolve once both offline events are persisted. The Destination persists as it queues each event, so this is fully deterministic with no fixed delay, no timer polling, and no fake timers (whose async helpers deadlock under the react-native jest env anyway). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Unify the web online/offline handlers through handleConnectivityChange with a deferFlush option (web defers the reconnect flush by flushIntervalMillis; native flushes immediately). - Log the incoming connectivity status at debug at the top of the handler. - Raise the failed-status-read and no-connectivity-source logs to warn. - Read navigator off the cached globalScope. - DRY the tests with beforeEach (client install tests; native-mode plugin tests) and rename the "web fallback" suite to "web mode". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PR 2 of the stacked SDKRN-5 offline series (stacked on #1802). Replaces the no-op iOS placeholder with the real connectivity module: - `NWPathMonitor` (Network framework, iOS 12+) with an `SCNetworkReachability` fallback for iOS 10/11 (podspec targets iOS 10). - `currentConnectivity()` reads a fresh `SCNetworkReachability` probe on demand, so the initial state JS seeds via `getNetworkConnectivityStatus` is correct even before the path monitor has started (no shared mutable state to race on). - Emits `AmplitudeNetworkConnectivityChanged` only while there are JS listeners. After this PR: iOS has real offline detection; Android is still the no-op stub (replaced in the next PR), so the SDK remains runnable. The `.m` export and bridging header already shipped in #1802 and are unchanged here. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Drop the SCNetworkReachability monitoring fallback for iOS 10/11. The podspec stays at iOS/tvOS 10 (no breaking deployment-target bump), but offline detection is now best-effort: below iOS/tvOS 12 the module reports connected unconditionally — both the initial seed and change events — which preserves the pre-offline SDK behavior (send always, existing retry handles failures). The seed guard matters: without a monitor running, seeding offline on an iOS 10/11 device that launches offline would buffer events forever. SCNetworkReachability is kept only for the fresh on-demand initial probe on iOS 12+; the C-callback monitoring path is deleted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Resolve getNetworkConnectivityStatus optimistically (always connected) and delete all SystemConfiguration code. The real initial state arrives via NWPathMonitor's first update: nw_path_monitor_set_update_handler is documented to call the handler "with the current path when start is called", so the first emit after JS subscribes corrects the seed within milliseconds — including the offline-at-launch case. The seed-call-plus-change-events bridge contract is kept (rather than seeding via the first event) to mirror the Amplitude-Kotlin and browser plugins: Android answers the seed with a real ConnectivityManager read, ported from Kotlin, since registerDefaultNetworkCallback fires nothing when the device is offline at registration. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Trim the class and seed doc comments to the essentials and link the SDK-header mirror documenting NWPathMonitor's initial-update contract. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Mercy811
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b5b433dcd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
size-limit report 📦
|
Address Copilot review: hasListeners is read by the pathUpdateHandler on monitorQueue but was written from start/stopObserving on the bridge thread, an unsynchronized cross-thread access that could let a queued update read a stale flag and emit after teardown. Write it on monitorQueue so all access is serialized; the monitor lifecycle stays synchronous so cancel() still fires immediately. Also trim the seed comment to drop the third-party SDK-header mirror link flagged in review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Raise the podspec deployment target from 10.0 to 12.0 and drop the now-dead best-effort fallback. iOS 12.0 is the lowest target Xcode 26 accepts (required for App Store uploads as of 2026-04-28) and is exactly NWPathMonitor's minimum, so the monitor can be used unconditionally: remove the #available(iOS 12) guards, the AnyObject workaround, and the canImport(Network) wrapper. No JS/TS change; the bridge contract is unchanged. https://developer.apple.com/news/upcoming-requirements/?id=02032026a Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Per PR review: set hasListeners and start/stop the NWPathMonitor inside a single monitorQueue.sync block instead of writing the flag async and starting/stopping the monitor separately. sync (not async) guarantees the flag is set before the monitor's first update fires and the monitor is torn down before stopObserving returns, so neither method returns with stale state. monitorQueue is distinct from the calling thread and NWPathMonitor.start only schedules async updates, so there is no deadlock. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Mercy811
force-pushed
the
sdkrn-5-offline-3-ios-tests
branch
from
ca12955 to
3826780
Compare
Mercy811
force-pushed
the
sdkrn-5-offline-3-ios-tests
branch
from
3826780 to
b52af25
Compare
Revert local-testing leaks flagged in review on #1819: DEVELOPMENT_TEAM, the iphoneos PRODUCT_BUNDLE_IDENTIFIER, and the run scheme (Release + no debugger) all back to the repo defaults so the shared example project isn't tied to a personal signing identity or Release-by-default. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Remove the stopMonitoring() call at the top of startMonitoring. Per review on #1819: RCTEventEmitter only calls startObserving on the 0->1 listener transition and stopObserving already nils the monitor on 1->0, so pathMonitor is always nil here and the guard never fired. Also revert incidental CocoaPods pod-install churn (empty inputPaths/ outputPaths) from the example app pbxproj so the PR diff stays minimal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Hold one NWPathMonitor for the module's lifetime instead of recreating it per subscribe. It's created (with its pathUpdateHandler) in init, started in startObserving, and cancelled only in deinit — never replaced. stopObserving just clears hasListeners; it must not cancel, since a cancelled NWPathMonitor can't be restarted. Drops the startMonitoring/stopMonitoring helpers. Per review on #1819: removes the pathMonitor overwrite/leak (r3424522147), the replace-on-start pattern (r3416357802), and the off-queue deinit access (r3391719568) — pathMonitor is now a write-once let, so deinit's read is race-free without hopping onto monitorQueue. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
getNetworkConnectivityStatus now resolves isConnected: false instead of true. Seeding offline means startup events buffer until the monitor's first update (delivered right after JS subscribes) reports the real status, rather than risk sending events while the device is actually offline at launch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
crleona
approved these changes
Jun 22, 2026
…ectivity module (SDKRN-5) Test source lives in packages/analytics-react-native/ios/Tests (excluded from the shipped pod via exclude_files); the example app's appTests target compiles it as host/runner, replacing the stale RN template test. rn-smoke runs xcodebuild test after the iOS build on both arch legs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
xcodebuild test leaves its destination simulator booted, so the unconditional simctl boot that follows would fail with "Unable to boot device in current state: Booted". bootstatus -b boots only if needed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Use lock.withLock {} instead of manual lock()/defer unlock() in the
EventCapturingConnectivity test helper.
- Reference the test file group-relative ("<group>") instead of
SOURCE_ROOT in the example app's Xcode project.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Mercy811
force-pushed
the
sdkrn-5-offline-3-ios-tests
branch
from
b52af25 to
ece5968
Compare
The appTests XCTest target imports the module as a separate module, so
the internal `init()` was invisible and EventCapturingConnectivity could
not be constructed ("no accessible initializers"). Mark it public, like
the other members exposed for the test subclass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
getNetworkConnectivityStatus seeds {isConnected: false} (so startup
events buffer until the monitor reports real status). The test still
asserted the old optimistic-connected seed; update it to expect
disconnected and refresh the stale doc comment on the path-update test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Summary
Part of SDKRN-23 (iOS native connectivity module).
PR 2.1 of 5 in the SDKRN-5 offline-mode stack. Stacked on #1819 (base branch
sdkrn-5-offline-2-ios).Adds native iOS XCTest coverage for
AmplitudeReactNativeConnectivity— the follow-up promised in #1819. The key behavior under test:NWPathMonitordelivers the current path as its first update once monitoring starts, the contract the optimistic{isConnected: true}seed ingetNetworkConnectivityStatusrelies on.Where the tests live (and why)
The test source lives in the SDK package —
packages/analytics-react-native/ios/Tests/AmplitudeConnectivityTests.swift— but is compiled and run by the example app's existingappTestsXCTest target, which acts purely as host/runner. A standalone run inside the package isn't possible: the module subclassesRCTEventEmitter, so the tests need React-Core compiled/linked, and RN library pods can't build outside an RN app's pod ecosystem (this is whycreate-react-native-libraryruns native tests via the example app too).s.exclude_files = "ios/Tests/**"keeps the XCTest sources out of the shipped pod (verified: absent from the generated Pods project).Changes
startObserving()→ initialAmplitudeNetworkConnectivityChangedwithisConnected == true(realNWPathMonitor); no events afterstopObserving();supportedEvents. AnEventCapturingConnectivitysubclass overridessendEvent(withName:body:)to capture events without a liveRCTBridge.class→open class; overridden members markedopen/publicso the test target can subclass/call them (Swift also requires the staticrequiresMainQueueSetupoverride to bepubliconce the class is open).appTests.m("Welcome to React", never rendered by this app, Metro-dependent) deleted; the SDK test file wired intoappTestsby file reference;SWIFT_VERSION = 5.0on theappTestsconfigs.Podfile.lockrefreshed (podspec checksum + stale 1.5.54 → 1.5.56 version catch-up).rn-smoke.ymlrunsxcodebuild testafter the iOS build on both arch legs, reusing the same-derivedDataPathso pod compilation is shared; Release config so the host app uses the embedded JS bundle (no Metro).main[5/5]Test plan
xcodebuild test(Release, iPhone 16 simulator): all 4 tests pass.monitor.start(queue:)commented out,testEmitsInitialPathUpdateAfterStartObservingfails at its 10s timeout — the suite genuinely exercises the monitor contract.pnpm --filter @amplitude/analytics-react-native test: jest suite unaffected (108 tests / 11 suites pass).new-archlegs.🤖 Generated with Claude Code