-
Notifications
You must be signed in to change notification settings - Fork 114
test: increase code coverage pt 2 #1411
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- disabled config and stylesheet failure return - hiding containers when subscribed and `unsubscribeEnable: false` - injecting markup and click flow (optIn then optOut) with text and class checks
- Different cases where `_getActiveState` returns None, OneSignal, or Third-Party sw - `_haveParamsChanged` branches (no registration, scope mismatch, missing scriptUrl, href changes, same href) - `_shoulInstallWorker` branches (SW support, OneSignal config, state, params changed, needs update) - `_workerNeedsUpdate` where comparing versions
- `_sendPushDeviceRecordUpdate` early return vs triggering `sendOnSessionUpdate` - `_sendOnSessionUpdate` branches: already sent, not first page, not registered, unsubscribed skip, subscribed path schedules upsert and sets flag
| }; | ||
| bell._setCustomColorsIfSpecified(); | ||
| const background = document.querySelector<HTMLElement>('.background')!; | ||
| expect(background.getAttribute('style')).toContain('#111'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add new lines after expects
| const nativeSpy = vi | ||
| .spyOn(pm, '_internalShowNativePrompt') | ||
| .mockResolvedValue(true); | ||
| const slidedownSpy = vi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could just make _internalShowSlidedownPrompt public since it gets mangled anyway
and remove the type assertions
| const pm = new PromptsManager(OneSignal._context); | ||
| const getOptsSpy = vi | ||
| .spyOn( | ||
| pm as unknown as { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same thing here
| pageViews: 0, | ||
| }), | ||
| ); | ||
| const condSpy = vi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_isPageViewConditionMet can be separated out of this class since theres no this usage,
export function isPageViewConditionMet(options?: DelayedPromptOptions): boolean {
if (!options || typeof options.pageViews === 'undefined') {
return false;
}
if (!options.autoPrompt || !options.enabled) {
return false;
}
const localPageViews = getLocalPageViewCount();
return localPageViews >= options.pageViews;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then it test can do:
import * as prompts from './PromptsManager';
const { PromptsManager } = prompts;
...
const slidedownSpy = vi
.spyOn(prompts, 'isPageViewConditionMet')
.mockReturnValue(false);
| TestEnvironment.initialize(); | ||
| sm = new SessionManager(OneSignal._context); | ||
| notifySpy = vi.spyOn(sm, '_notifySWToUpsertSession'); | ||
| deactSpy = vi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same thing, could make public
| // ensure user present | ||
| User._createOrGetInstance(); | ||
|
|
||
| vi.spyOn( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets move function outside of the class _getOneSignalAndSubscriptionIds -> export function getOneSignalAndSubscriptionIds since theres no this usage
| visSpy.mockRestore(); | ||
| focusSpy.mockRestore(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dont really need
| OneSignal._context._dynamicResourceLoader, | ||
| '_loadSdkStylesheet', | ||
| ).mockResolvedValue(ResourceLoadState._Failed); | ||
| await mgr._initialize(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: new line after
|
|
||
| test('_sendOnSessionUpdate flows: already sent, not first page, not registered, skip when unsubscribed, success path sets flag', async () => { | ||
| // Already sent | ||
| (mgr as unknown as { _onSessionSent: boolean })._onSessionSent = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
avoid type assertions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could do this instead mgr['_onSessionSent'] = true;
| (mgr as unknown as { _onSessionSent: boolean })._onSessionSent = true; | ||
| await mgr._sendOnSessionUpdate(); | ||
| // reset | ||
| (mgr as unknown as { _onSessionSent: boolean })._onSessionSent = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mgr['_onSessionSent'] = false;
|
|
||
| // Registered but not subscribed and enableOnSession not true -> skip | ||
| alreadySpy.mockResolvedValue(true); | ||
| const pushSpy = vi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could do:
const pushSub = createPushSub();
pushSub._notification_types = NotificationType._NoNativePermission;
const pushSpy = vi
.spyOn(OneSignal._coreDirector, '_getPushSubscriptionModel')
.mockResolvedValue(pushSub);
| expect(upsertSpy).not.toHaveBeenCalled(); | ||
|
|
||
| // Subscribed path | ||
| pushSpy.mockResolvedValue({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pushSub._notification_types = NotificationType._Subscribed;
pushSub.id = 'sub';
pushSpy.mockResolvedValue(pushSub);
await mgr._sendOnSessionUpdate();
| mgr as unknown as { _workerNeedsUpdate: () => Promise<boolean> }, | ||
| '_workerNeedsUpdate', | ||
| ).mockResolvedValue(false); | ||
| await expect( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
too much type assertion usages
| await expect( | ||
| ( | ||
| mgr as unknown as { _shouldInstallWorker: () => Promise<boolean> } | ||
| )._shouldInstallWorker(), | ||
| ).resolves.toBe(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could do this and similar things for the others:
await expect(mgr['_shouldInstallWorker']()).resolves.toBe(false);
| vi.spyOn(mgr, '_getActiveState').mockResolvedValue( | ||
| helpers.ServiceWorkerActiveState._OneSignalWorker, | ||
| ); | ||
| vi.spyOn( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could make _haveParamsChanged public since it just returns a boolean
Description
1 Line Summary
Adds more tests to increase code coverage.
Details
Adds/expands test across the following modules:
BellPromptsManagerCustomLinkManagerServiceWorkerManagerUpdateManagerSessionManagerBefore
After
Systems Affected
Validation
Tests
Info
Checklist
Programming Checklist
Interfaces:
Functions:
Typescript:
Other:
elem of arraysyntax. PreferforEachor usemapcontextif possible. Instead, we can pass it to function/constructor so that we don't callOneSignal.contextScreenshots
Info
Checklist
Related Tickets
This change is