Skip to content

test: [SDK-5538] guard stand-ins and the public API against drift - #2828

Draft
abdulraqeeb33 wants to merge 4 commits into
never-crash/4-never-crash-initfrom
never-crash/5-api-coverage-tests
Draft

abdulraqeeb33 wants to merge 4 commits into
never-crash/4-never-crash-initfrom
never-crash/5-api-coverage-tests

Conversation

@abdulraqeeb33

@abdulraqeeb33 abdulraqeeb33 commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Stack (base feat/never-crash, merged to main as one unit; split out of #2793):

  1. refactor: [SDK-5538] add SdkOperation names for init-gated entry points #2800 SdkOperation names
  2. feat: [SDK-5538] remote kill switch with a placeholder OneSignalImpV2 #2801 Kill switch with a placeholder OneSignalImpV2
  3. feat: [SDK-5538] deferred managers that stand in before init #2802 Deferred managers
  4. fix: [SDK-5538] never crash on invalid init or calls before init #2803 Never-crash init
  5. test: [SDK-5538] guard stand-ins and the public API against drift #2828 Drift tests for the stand-ins and the public API (this PR)

Description

One Line Summary

Tests that fail when a new public method is added without deciding how it behaves before init, and a parity harness that fails when V2 drifts from the legacy implementation after init. externalId, context, and appId are the nullable public arguments.

Details

Motivation

Part of SDK-5538. The compiler already forces every new abstract interface method into its stand-in, but not a method with a default body, not whether a new listener is queued before init, and not whether a new IOneSignal method checks the init gate. These tests cover those gaps.

Scope

Public entry points:

  • externalId on login, loginSuspend, updateUserJwt, and updateUserJwtSuspend, and context on the single-argument initWithContext, are nullable. context and appId on the two-argument init methods already were. Token, listeners, and the user profile stay non-null.
  • V2 checks a null context, appId, or externalId in the method that accepts it and returns. The log names the argument. When the SDK is not initialized, the log says so. A null appId with a cached app ID uses that ID and the log says so. After a successful init, a null externalId is still rejected, without claiming the SDK is down. Empty and null-byte values still log and return.
  • Legacy OneSignalImp throws the same NullPointerException message Kotlin used to throw for a null argument.
  • The isMissing(value, operation, parameter) overload is removed. Call sites pass the log string ("login: externalId").

Tests, three files:

  • DeferredManagersCoverageTests
    • Every member of the six manager interfaces is declared on its stand-in, not inherited from a default body. The failure names the member and the stand-in.
    • Every add...Listener, add...Observer and settable property must be listed in the replay table. A new one fails until someone decides whether it queues before init.
    • Each listed call is queued before init and reaches the real manager exactly once on replay.
  • OneSignalImpV2ApiSweepTests
    • Every IOneSignal member, called with dummy arguments before init and again after a failed init, must not throw or hang past 5 seconds. The deadline is enforced from a separate thread, so a blocking wait inside the member cannot escape it.
    • The same sweep runs over every member of the managers handed out before init (User, pushSubscription, Notifications, InAppMessages, Location, Session).
    • Null sweep: every still-required parameter is passed null in turn (the Java boundary). Nothing may hang, and nothing but a NullPointerException may surface. externalId, context, and appId are nullable: V2 logs and returns, and legacy throws the same NullPointerException.
  • OneSignalImpParityTests
    • Runs the same scenario on OneSignalImp and OneSignalImpV2 with clean prefs and asserts the app-visible results are equal: init then every top-level read; consent and GMS flags set before init reach the config model; a second initWithContext keeps the first app ID; login, tags, logout; empty app ID falls back to the cache; null app ID (suspend) resolves from the cache.

Not covered

  • Members that need a non-null Context are skipped, since they would start a real init. The nullable-context variants are swept.
  • The JWT invalidated listener isn't a manager interface, so only "doesn't throw" is checked for it.
  • IDebugManager is a real object before init; only the debug getter is called.
  • Parity covers behavior after init. The pre-init differences (throw vs. log) are intentional and covered by the V2 suites in fix: [SDK-5538] never crash on invalid init or calls before init #2803.

Testing

Unit testing

Each drift test was checked against a deliberate breakage, and all went red naming it: a new default-body method on ISessionManager, a new default-body listener on IInAppMessagesManager, isShared no longer queued, a stand-in method that throws, logout() throwing before init, and a suspend getter that hangs. The parity harness went red on test pollution from the 200 ms prefs flush before the scenarios were isolated, which is the failure shape it is meant to produce. The earlier full run was 1854 tests across core, location, in-app-messages, and notifications, with spotless, detekt, and apiCheck.

The nullable-argument commit re-ran InputGuardTests, OneSignalImpTests, OneSignalImpV2Tests, and OneSignalImpV2ApiSweepTests (49 tests, 0 failures), plus spotless and detekt.

This commit re-ran OneSignalImpV2Tests (18) and OneSignalImpTests (27), plus spotless and detekt. The new test checks that a null context and a null externalId name the argument, and that the log says the SDK is not initialized only when it is not.

Manual testing

Not needed. Behavior is covered by the unit tests above.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
  • I have reviewed this PR myself, ensuring it meets each checklist item

@github-actions

github-actions Bot commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

📊 Diff Coverage Report

Diff Coverage Report (Changed Lines Only)

Gate: aggregate coverage on changed executable lines must be ≥ 80% (JaCoCo line data for lines touched in the diff).

Changed Files Coverage

  • IOneSignal.kt: 1/1 touched executable lines (100.0%) (11 touched lines in diff)
  • OneSignal.kt: 0/1 touched executable lines (0.0%) (11 touched lines in diff)
    • 1 uncovered touched lines in this file
  • InputGuard.kt: 15/15 touched executable lines (100.0%) (all lines — could not resolve diff)
  • OneSignalImp.kt: 3/5 touched executable lines (60.0%) (10 touched lines in diff)
    • 2 uncovered touched lines in this file
  • OneSignalImpV2.kt: 4/9 touched executable lines (44.4%) (14 touched lines in diff)
    • 5 uncovered touched lines in this file

Overall (aggregate gate)

23/31 touched executable lines covered (74.2% — requires ≥ 80%)

Per-file detail (informational; gate is aggregate above):

  • OneSignal.kt: 0.0% (1 uncovered touched lines)

  • OneSignalImp.kt: 60.0% (2 uncovered touched lines)

  • OneSignalImpV2.kt: 44.4% (5 uncovered touched lines)

❌ Coverage Check Failed

Aggregate coverage on touched lines is 74.2% (minimum 80%).

📥 View workflow run

AR Abdul Azeez and others added 2 commits November 5, 2026 15:08
DeferredManagersCoverageTests: every member of the six manager interfaces
must be declared on its stand-in, not inherited from a default body. Every
add...Listener/Observer and every var must have a replay case, and each case
must queue before init and reach the real manager on replay.

OneSignalImpV2ApiSweepTests: every IOneSignal member, and every member of the
managers handed out before init, is called reflectively with dummy args and
must not throw or hang past 5s. Members that need a non-null Context are
skipped, since they would start a real init.

Co-authored-by: Cursor <cursoragent@cursor.com>
…egacy and V2 behavior

The sweep now runs after a failed init as well, enforces its deadline from a
separate thread so a blocking wait cannot escape withTimeout, and passes null
for every required parameter to pin the Java boundary: nothing hangs and
nothing but the Kotlin NullPointerException surfaces.

OneSignalImpParityTests runs the same scenarios on OneSignalImp and
OneSignalImpV2 and asserts the app-visible results match.

Co-authored-by: Cursor <cursoragent@cursor.com>
AR Abdul Azeez and others added 2 commits November 5, 2026 16:20
…y points

V2 logs and returns. Legacy still throws the same NullPointerException.
Drop the isMissing overload that only built the log string.

Co-authored-by: Cursor <cursoragent@cursor.com>
V2 returns after the log. When the SDK is not initialized, the log says so.
After a successful init, a null externalId is still rejected without that sentence.

Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant