Skip to content
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

[debug] Internal dump method infrastucture #1472

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nan-li
Copy link
Contributor

@nan-li nan-li commented Aug 22, 2024

Description

One Line Summary

Add OneSignal.Debug._dump method to help log different services for debugging and testing.

Details

OSDebug namespace will have _dump added, but it will be implemented in the OneSignal module instead of the OneSignalCore module where the primary implementation of OneSignalLog lives. This is so OneSignal can call sub modules to log themselves such as services in the User module.

A new protocol called OSLoggable is added with a single method to logSelf().

Motivation

  • Help debug and test.
  • Can be expanded in the future for SDK clients to get easily shareable information in issues reports.

Scope

Just skeleton for dumping information.

Testing

Unit testing

None

Manual testing

  • Added this method to the Dev App "get tags" button and see it is triggered.

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

This change is Reviewable

* internal for now, put in information you want to log for each product that conforms to OSLoggable
@nan-li
Copy link
Contributor Author

nan-li commented Aug 22, 2024

Hmm the test OneSignalNotificationsTests.testClearBadgesWhenAppEntersForeground() keeps failing in the CI but succeeds locally..

Running the CI in [Fix] Handle incorrect 404, delay making requests to new IDs which passed last week to see if something changed unrelated to this PR, and that PR fails for this now too.

@nan-li nan-li force-pushed the internal_dump_method branch 4 times, most recently from 35677bc to 039479d Compare August 23, 2024 18:54
@nan-li
Copy link
Contributor Author

nan-li commented Aug 23, 2024

Nevermind, I can revisit that test later down the line.

Base automatically changed from improve/delay_updates_after_creates to main August 23, 2024 21:11
@emawby emawby self-assigned this Aug 27, 2024
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