Skip to content

Conversation

fadi-george
Copy link
Contributor

@fadi-george fadi-george commented Oct 21, 2025

Description

One Line Summary

  • run tests in ci workflow

Details

  • adds vitest & happy-dom package and new test scripts in package json
  • adds simple index test file

Motivation

  • want to have more test coverage for sdk packages

Testing

Unit testing

  • ran bun run test and got all tests passing
  • also tested example app on emulators

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
    • If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
  • 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.
    • Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
  • I have reviewed this PR myself, ensuring it meets each checklist item
    • WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

This change is Reviewable

@fadi-george fadi-george force-pushed the fg/add-tests branch 2 times, most recently from 3d3aa84 to ccb9ee9 Compare October 21, 2025 22:59
@fadi-george fadi-george changed the base branch from main to fg/clean-up-build October 21, 2025 22:59
@fadi-george fadi-george force-pushed the fg/add-tests branch 2 times, most recently from fbe4eff to 7d7b0cb Compare October 21, 2025 23:02
Copy link
Contributor

@nan-li nan-li left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's organize the testing structure. Curently there is a first-level /mocks directory and index.test.ts lives among the regular files under /www.

observerCallback();

// should call getPushSubscriptionId
expect(window.cordova.exec).toHaveBeenCalledWith(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Should we include this expect for getPushSubscriptionId?

Copy link
Contributor Author

@fadi-george fadi-george Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.test. next to files would help identify which files have test or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise youd hvea tests/ folder that looks identical e.g.
tests/
index.test.ts
DebugNamespace.test.ts
InAppMessagesNamespace.test.ts
etc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I searched testing structures and found this pattern is termed colocating / colocation..

@fadi-george fadi-george force-pushed the fg/clean-up-build branch 2 times, most recently from 106408f to 9a5d1de Compare October 22, 2025 22:57
@nan-li nan-li self-requested a review October 24, 2025 00:04
Base automatically changed from fg/clean-up-build to main October 24, 2025 00:08
@fadi-george fadi-george merged commit 1c514c2 into main Oct 24, 2025
4 checks passed
@fadi-george fadi-george deleted the fg/add-tests branch October 24, 2025 00:13
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.

2 participants