OneSignal / OneSignal-Website-SDK

OneSignal is a push notification service for web and mobile apps. This SDK makes it easy to integrate your website with OneSignal Push Notifications. https://onesignal.com
Other
389 stars 115 forks source link

test: explicitly assert for Log.error calls in `registerForPush` #1206

Closed sherwinski closed 4 days ago

sherwinski commented 1 week ago

Description

1 Line Summary

Explicitly tests for an error in the registerForPush test.

Details

When running current tests, the following error is emitted:

  console.error
    PushPermissionNotGrantedError: The user dismissed the permission prompt.
        at SubscriptionManager.subscribeFcmFromPage (/Website-SDK/src/shared/managers/SubscriptionManager.ts:461:17)
        at SubscriptionManager.subscribe (/Website-SDK/src/shared/managers/SubscriptionManager.ts:162:13)
        at Function.internalRegisterForPush (/Website-SDK/src/shared/helpers/SubscriptionHelper.ts:30:35)
        at Function.registerForPush (/Website-SDK/src/shared/helpers/SubscriptionHelper.ts:20:12)
        at Function.registerForPushNotifications (/Website-SDK/src/shared/helpers/InitHelper.ts:104:5)
        at PromptsManager.internalShowNativePrompt (/Website-SDK/src/page/managers/PromptsManager.ts:188:5)
        at NotificationsNamespace.requestPermission (/Website-SDK/src/onesignal/NotificationsNamespace.ts:142:5)
        at PushSubscriptionNamespace.optIn (/Website-SDK/src/onesignal/PushSubscriptionNamespace.ts:96:7)
        at Object.<anonymous> (/Website-SDK/__test__/unit/push/registerForPush.test.ts:33:5) {
      reason: 1
    }

This change adds an assertion since the error is part of the test's expected behavior.

Even though the error occurs, the tests still pass. For that reason, I assumed it's not part of the test's expected behavior. Please feel free to correct me if my understanding there is wrong.

Note: If needed we can add something like expect(Log.error).toHaveBeenCalled() to ensure that we are testing for the existence of the error.

Systems Affected

Validation

Tests

Info

Checklist

Programming Checklist Interfaces:

Functions:

Typescript:

Other:

Screenshots

Info

Checklist


Related Tickets



This change is Reviewable

shepherd-l commented 4 days ago

Even though the error occurs, the tests still pass. For that reason, I assumed it's not part of the test's expected behavior. Please feel free to correct me if my understanding there is wrong.

I was also unsure at first but I think it's part of the test's expected behaviour as discussed here: https://github.com/OneSignal/OneSignal-Website-SDK/pull/1192#pullrequestreview-2298504262

Note: If needed we can add something like expect(Log.error).toHaveBeenCalled() to ensure that we are testing for the existence of the error.

Good idea, this makes sense to do inorder to avoid confusion