From ef2286ac5e957a39dc2662f0ded37680e2de2c9e Mon Sep 17 00:00:00 2001 From: jinliu9508 Date: Tue, 20 May 2025 15:00:42 -0400 Subject: [PATCH 1/3] refactor: move mocks to a mock object for NotificaationLifecycleServiceTests --- .../NotificationLifecycleServiceTests.kt | 91 ++++++++++++------- 1 file changed, 56 insertions(+), 35 deletions(-) diff --git a/OneSignalSDK/onesignal/notifications/src/test/java/com/onesignal/notifications/internal/lifecycle/NotificationLifecycleServiceTests.kt b/OneSignalSDK/onesignal/notifications/src/test/java/com/onesignal/notifications/internal/lifecycle/NotificationLifecycleServiceTests.kt index d98d58948a..57845674eb 100644 --- a/OneSignalSDK/onesignal/notifications/src/test/java/com/onesignal/notifications/internal/lifecycle/NotificationLifecycleServiceTests.kt +++ b/OneSignalSDK/onesignal/notifications/src/test/java/com/onesignal/notifications/internal/lifecycle/NotificationLifecycleServiceTests.kt @@ -26,6 +26,59 @@ import io.mockk.spyk import org.json.JSONArray import org.json.JSONObject import org.robolectric.Robolectric +import org.robolectric.android.controller.ActivityController + +private class Mocks { + val context = ApplicationProvider.getApplicationContext() + val applicationService = + run { + val applicationService = ApplicationService() + applicationService.start(context) + applicationService + } + + val mockSubscriptionManager: ISubscriptionManager = + run { + val mockSubManager = mockk() + every { mockSubManager.subscriptions.push } returns + mockk().apply { every { id } returns "UUID1" } + mockSubManager + } + + val notificationLifecycleService = + spyk( + NotificationLifecycleService( + applicationService, + MockHelper.time(0), + MockHelper.configModelStore(), + mockk().apply { + every { onDirectInfluenceFromNotification(any()) } returns Unit + }, + mockSubscriptionManager, + mockk().apply { + every { deviceType } returns IDeviceService.DeviceType.Android + }, + mockk().apply { + coEvery { updateNotificationAsOpened(any(), any(), any(), any()) } returns Unit + }, + mockk(), + mockk().apply { + every { trackOpenedEvent(any(), any()) } returns Unit + }, + ), + ) + + val activity: Activity = + run { + val activityController : ActivityController + Robolectric.buildActivity(Activity::class.java).use { controller -> + controller.setup() // Moves Activity to RESUMED state + activityController = controller + } + activityController.get() + } + +} @RobolectricTest class NotificationLifecycleServiceTests : FunSpec({ @@ -36,41 +89,9 @@ class NotificationLifecycleServiceTests : FunSpec({ test("Fires openDestinationActivity") { // Given - val context = ApplicationProvider.getApplicationContext() - val applicationService = ApplicationService() - applicationService.start(context) - - val mockSubscriptionManager = mockk() - every { mockSubscriptionManager.subscriptions.push } returns - mockk().apply { every { id } returns "UUID1" } - - val notificationLifecycleService = - spyk( - NotificationLifecycleService( - applicationService, - MockHelper.time(0), - MockHelper.configModelStore(), - mockk().apply { - every { onDirectInfluenceFromNotification(any()) } returns Unit - }, - mockSubscriptionManager, - mockk().apply { - every { deviceType } returns IDeviceService.DeviceType.Android - }, - mockk().apply { - coEvery { updateNotificationAsOpened(any(), any(), any(), any()) } returns Unit - }, - mockk(), - mockk().apply { - every { trackOpenedEvent(any(), any()) } returns Unit - }, - ), - ) - val activity: Activity - Robolectric.buildActivity(Activity::class.java).use { controller -> - controller.setup() // Moves Activity to RESUMED state - activity = controller.get() - } + val mocks = Mocks() + val notificationLifecycleService = mocks.notificationLifecycleService + val activity = mocks.activity // When val payload = From 54c0fd1532f6e3238cc54c23b0d7cb1e5f03c85e Mon Sep 17 00:00:00 2001 From: jinliu9508 Date: Thu, 22 May 2025 12:42:55 -0400 Subject: [PATCH 2/3] test: add a test unit to ensure openDestinationActivity not to be blocked --- .../NotificationLifecycleServiceTests.kt | 48 +++++++++++++++++-- 1 file changed, 45 insertions(+), 3 deletions(-) diff --git a/OneSignalSDK/onesignal/notifications/src/test/java/com/onesignal/notifications/internal/lifecycle/NotificationLifecycleServiceTests.kt b/OneSignalSDK/onesignal/notifications/src/test/java/com/onesignal/notifications/internal/lifecycle/NotificationLifecycleServiceTests.kt index 57845674eb..3d18575f93 100644 --- a/OneSignalSDK/onesignal/notifications/src/test/java/com/onesignal/notifications/internal/lifecycle/NotificationLifecycleServiceTests.kt +++ b/OneSignalSDK/onesignal/notifications/src/test/java/com/onesignal/notifications/internal/lifecycle/NotificationLifecycleServiceTests.kt @@ -23,6 +23,8 @@ import io.mockk.coVerify import io.mockk.every import io.mockk.mockk import io.mockk.spyk +import kotlinx.coroutines.delay +import kotlinx.coroutines.withTimeout import org.json.JSONArray import org.json.JSONObject import org.robolectric.Robolectric @@ -59,7 +61,11 @@ private class Mocks { every { deviceType } returns IDeviceService.DeviceType.Android }, mockk().apply { - coEvery { updateNotificationAsOpened(any(), any(), any(), any()) } returns Unit + coEvery { updateNotificationAsOpened(any(), any(), any(), any()) } coAnswers { + // assume every updateNotificationAsOpened call takes 5 ms + delay(5) + Unit + } }, mockk(), mockk().apply { @@ -70,14 +76,13 @@ private class Mocks { val activity: Activity = run { - val activityController : ActivityController + val activityController: ActivityController Robolectric.buildActivity(Activity::class.java).use { controller -> controller.setup() // Moves Activity to RESUMED state activityController = controller } activityController.get() } - } @RobolectricTest @@ -115,4 +120,41 @@ class NotificationLifecycleServiceTests : FunSpec({ ) } } + + test("ensure notificationOpened makes backend updates in a background process") { + // Given + val mocks = Mocks() + val notificationLifecycleService = mocks.notificationLifecycleService + val activity = mocks.activity + + // When + val payload = JSONArray() + for (i in 1..1000) { + // adding 1000 different notifications + payload.put( + JSONObject() + .put("alert", "test message") + .put( + "custom", + JSONObject() + .put("i", "UUID$i"), + ), + ) + } + + withTimeout(500) { + // 1000 notifications should be handled within a small amount of time + notificationLifecycleService.notificationOpened(activity, payload) + } + + // Then + coVerify(exactly = 1) { + // ensure openDestinationActivity is called within the timeout, prove that the increasing + // number of notifications clicked does not delay the main thread proportionally + notificationLifecycleService.openDestinationActivity( + withArg { Any() }, + withArg { Any() }, + ) + } + } }) From 817a9cfe6269617a6636410f95c7f74ca2fe73e9 Mon Sep 17 00:00:00 2001 From: jinliu9508 Date: Thu, 22 May 2025 12:43:53 -0400 Subject: [PATCH 3/3] fix: call updateNotificationAsOpened in a destinate coroutine scope --- .../impl/NotificationLifecycleService.kt | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/OneSignalSDK/onesignal/notifications/src/main/java/com/onesignal/notifications/internal/lifecycle/impl/NotificationLifecycleService.kt b/OneSignalSDK/onesignal/notifications/src/main/java/com/onesignal/notifications/internal/lifecycle/impl/NotificationLifecycleService.kt index b4ae79a5ec..ff85db76dc 100644 --- a/OneSignalSDK/onesignal/notifications/src/main/java/com/onesignal/notifications/internal/lifecycle/impl/NotificationLifecycleService.kt +++ b/OneSignalSDK/onesignal/notifications/src/main/java/com/onesignal/notifications/internal/lifecycle/impl/NotificationLifecycleService.kt @@ -7,6 +7,7 @@ import com.onesignal.common.JSONUtils import com.onesignal.common.events.CallbackProducer import com.onesignal.common.events.EventProducer import com.onesignal.common.exceptions.BackendException +import com.onesignal.common.threading.OSPrimaryCoroutineScope import com.onesignal.core.internal.application.AppEntryAction import com.onesignal.core.internal.application.IApplicationService import com.onesignal.core.internal.config.ConfigModelStore @@ -138,15 +139,17 @@ internal class NotificationLifecycleService( postedOpenedNotifIds.add(notificationId) - try { - _backend.updateNotificationAsOpened( - appId, - notificationId, - subscriptionId, - deviceType, - ) - } catch (ex: BackendException) { - Logging.error("Notification opened confirmation failed with statusCode: ${ex.statusCode} response: ${ex.response}") + OSPrimaryCoroutineScope.execute { + try { + _backend.updateNotificationAsOpened( + appId, + notificationId, + subscriptionId, + deviceType, + ) + } catch (ex: BackendException) { + Logging.error("Notification opened confirmation failed with statusCode: ${ex.statusCode} response: ${ex.response}") + } } }