-
-
Notifications
You must be signed in to change notification settings - Fork 14
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
Fix registerRemoteNotifications action would never return #115
Conversation
CC: @nriedman |
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.
Thank you for fixing the issue and adding all the documentation @Supereg! 🚀
There is an error message due to Xcode project version updated beyond the latest stable Xcode release. I tagged this in the diff.
The main UI tests seem to fail on the simulator; I could not reproduce it locally. Might be an issue with our runners?
Sources/Spezi/Capabilities/Notifications/RegisterRemoteNotificationsAction.swift
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #115 +/- ##
==========================================
+ Coverage 90.33% 90.60% +0.27%
==========================================
Files 47 47
Lines 1602 1626 +24
==========================================
+ Hits 1447 1473 +26
+ Misses 155 153 -2
Continue to review full report in Codecov by Sentry.
|
@PSchmiedmayer Executing the UI tests on my local simulator, @nriedman Would you mind, trying this branch and running the TestApp of the UITests, navigating to |
What a strange behavior. I could imagine this has something to do with the missing developer account or the fact that the build agents run in a virtual machine. Elements like Xcode Apple Intelligence Code suggestions and other elements are also disables when Xcode runs in a VM (I suspect Apple doesn't want to make VMs fully operational to avoid an easy way to virtualize macOS for commercial purposes). That would be my educated guess, I can try to verify this tomorrow. |
@Supereg I can verify that you get a timeout on a Simulator with no Developer Account signed in on a more or less blank macOS instance without any virtualization. @nriedman can you verify this on your machine as well? As far as I remember you also weren't signed into your developer account on Xcode. |
I think the best approach here would be to document this behavior (Apple doesn't seem to do that for us ...) and then ensure that clients that use this can catch the Timeout error and catch it appropriately for UI testing behavior (e.g. for our apps using the testing flag to ignore this specific error). I am worried that giving an invalid token that we generate ourselves will lead to too many unexpected behavior down the line and might confuse developers even more ... what is your though on this @Supereg? |
Hi @Supereg and @PSchmiedmayer! I checked out the TestApp on this branch with no development account (I signed out on Xcode and cleared the simulator to make sure no developer account could sneak in there), and I did receive a device token when I selected "Register". It seems to work as intended on my machine. I'm not sure why this is the case, when it doesn't work on the CI or on @PSchmiedmayer's test, but I can confirm that it works for me both with a personal developer account and no developer account at all. |
@PSchmiedmayer I went forward and documented the behavior (as far as we could reproduce it) in the docs. Additionally I we log a warning when throwing a timeout error due to timing out on a simulator device. I also covered catching the TimeoutError in simulator environment the code example so this is easily discoverable. What do you think? |
Markdown lint is currently failing due to a SSL error when requesting |
Nice; thank you for fixing all the issues and adding the documentation! Happy to merge it despite the MD check failing, we can't really fix the reuse.software HTTPs issues 😁 |
Fix registerRemoteNotifications action would never return
♻️ Current situation & Problem
#98 introduced the
registerRemoteNotifications
action. However, the knowledge source that stores the continuation was never persisted in the store. Therefore, the continuation was never resumed and allocated resources forever.This was fixed with this PR. We originally discussed supporting cancellation of the registration processed, however I decided against as it wasn't clear to properly handle cancellation (e.g., automatically call the unregister action? How to handle the delegate call that might arrive later. Especially when another call to the action might incur in the mean time).
This PR changes the behavior when concurrently calling the registerRemoteNotifications action. Now, the second caller will wait for the first call to complete instead of throwing a concurrence access error. This helps to share this resources with, e.g., multiple modules trying to retrieve an device token.
To emphasize:
registerRemoteNotifications
works on simulator devices!⚙️ Release Notes
registerRemoteNotifications
actions would never return.📚 Documentation
Slightly updated docs to provide more guidance around requesting authorization for notifications.
✅ Testing
Added a unit test that verifies that the action returns.
📝 Code of Conduct & Contributing Guidelines
By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines: