Skip to content

Conversation

@alibaker-optimizely
Copy link
Contributor

Summary

  • The notification_center module did not have a unit test associated with it.
  • This PR addresses that to ensure 100% coverage of this module.

Test plan

addNotificationListener

  • only valid listener types should have listeners registered
  • the listenerId should be returned upon successful registration
  • cannot register the same callback twice

removeNotificationListener

  • should return true if the listener is successfully removed
  • should only remove the specified listener using the listenerId

clearAllNotificationListeners

  • should remove all listeners of all types

clearNotificationListeners

  • should remove only the specified notification type when there are no listeners registered
  • should remove only the specified notification type and leave all other notification type listeners still registered

sendNotifications

  • checks that each notification type listeners executes the callback with the passed in notification data from the user

Issues

@coveralls
Copy link

coveralls commented Jan 30, 2020

Coverage Status

Coverage increased (+0.1%) to 97.559% when pulling 31df225 on alibaker/add-notification-center-unit-tests into bc84698 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 97.559% when pulling 931dea2 on alibaker/add-notification-center-unit-tests into bc84698 on master.

Copy link
Contributor

@mjc1283 mjc1283 left a comment

Choose a reason for hiding this comment

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

Great work, I appreciate the thorough test coverage here. I have just some minor suggestions - overall this looks good to me.

@mjc1283 mjc1283 merged commit 10ce003 into master Feb 10, 2020
@mjc1283 mjc1283 deleted the alibaker/add-notification-center-unit-tests branch February 10, 2020 20:56
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.

4 participants