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

Add postDistributedNotifications matcher for testing DistributedNotificationCenter #786

Merged
merged 3 commits into from
Jun 4, 2020

Conversation

fabianmuecke
Copy link
Contributor

@fabianmuecke fabianmuecke commented Jun 3, 2020

This is a workaround for postNotification not working at all on Catalina, due to Catalina not calling any observers subscribed to "nil" name.

Checklist - While not every PR needs it, new features should consider this list:

  • Does this have tests?
  • Does this have documentation?
  • Does this break the public API (Requires major version bump)?
  • Is this a new feature (Requires minor version bump)?

Fabian Muecke added 2 commits June 3, 2020 11:38
…ification (workaround for Catalina not calling observers, when observing name "nil")
@ikesyo
Copy link
Member

ikesyo commented Jun 3, 2020

Thanks for submitting the pull request!

postNotification not working at all on Catalina, due to Catalina not calling any observers subscribed to "nil" name.

We don't see such breakage on our CI tests so I'm confused 🤔 We use GitHub Actions and the environment is Catalina:

https://help.github.com/en/actions/reference/virtual-environments-for-github-hosted-runners#supported-runners-and-hardware-resources

macOS Catalina 10.15

And also I can't find any description about the behavioral change at https://developer.apple.com/documentation/foundation/notificationcenter/1411723-addobserver.

@fabianmuecke
Copy link
Contributor Author

Then it is probably limited to DistributedNotificationCenter. It is indeed an undocumented change, but Apple acknowledged it exists. See: https://mjtsai.com/blog/2019/10/04/nsdistributednotificationcenter-no-longer-supports-nil-names/ for more.

@ikesyo
Copy link
Member

ikesyo commented Jun 3, 2020

Thanks for the input, I didn't know DistributedNotificationCenter at all (I'm an iOS app developer 😄 ). Then I recommend you to add a postNotifications overload which takes DistributedNotificationCenter over NotificationCenter and make names parameter non-nil, leaving the existing matchers as is. That should make the situation/constraint clear (notification names are mandatory for DistributedNotificationCenter).

@fabianmuecke
Copy link
Contributor Author

Sounds good.

@fabianmuecke
Copy link
Contributor Author

Tried to implement your suggestion. Unfortunately users still won't be forced to use the function with the names parameter, as the DistributedNotificationCenter inherits from NotificationCenter and the function without names would still be valid for the compiler. Therefore I added it as a new function called postDistributedNotification, trying to make it clearer that this is intended for working with DistributedNotificationCenter. Also please note that the tests will fail, if not using toEventually, as the DistributedNotificationCenter works async, even if using deliverImmediately: true.

@ikesyo ikesyo changed the title Subscription of specific names for postNotification Add postDistributedNotifications matcher for testing DistributedNotificationCenter Jun 4, 2020
Copy link
Member

@ikesyo ikesyo left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the contribution! 👍

@ikesyo ikesyo merged commit c68b998 into Quick:master Jun 4, 2020
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