Return to using a versioned ably-cocoa release #641
Labels
build
Relates to the tooling used to build or release the contents of this repository.
code-quality
Affects the developer experience when working in our codebase.
Comments
|
➤ Automation for Jira commented: The link to the corresponding Jira issue is https://ably.atlassian.net/browse/SDK-3483 |
lawrence-forooghian
added a commit
that referenced
this issue
Mar 27, 2023
Resolves #539. These are based on the corresponding tests in the Android codebase at commit f6d163f (plus a fix added in eea952b). They stick as closely as possible to the structure and behaviour of those tests, to which I have not applied much of a critical eye. The majority of the log messages and comments are simply copied across. The only structural difference between these tests and the Android ones is that I have removed the caller’s responsibility to call `SubscriberMonitor#close`. There are places in the Android implementation where we forget to call it, and also it isn’t called if an error is thrown by `waitForStateTransition`. By making the subscriber monitor responsible for its own cleanup, we remove these issues. When I started writing these tests, we were still targeting iOS 12 and above, which meant it was not possible to use Swift concurrency. Hence, all of the code is written using either completion handlers or blocking functions. Since then, we’ve decided to set our deployment target to iOS 13 (see #597), which means we could now use Swift concurrency. However, by that point I was well into the writing of these tests and did not want to re-structure them, especially since they require functionality that doesn’t exist out of the box in Swift concurrency (for example timeouts). We may wish to revisit at some point and switch to using Swift concurrency, but I don’t think it’s urgent. In order to generate the list of which faults to skip, I ran all of the tests and checked which ones failed. Whilst doing so, I encountered a crash in ably-cocoa, which I believe is already reported as ably/ably-cocoa#1380. In order to get past this issue, I needed to use the `main` version of ably-cocoa, which contains a fix for it. I’ve created #641 to make sure we return to using a proper release of ably-cocoa before we make our next AAT release.
lawrence-forooghian
added a commit
that referenced
this issue
Mar 27, 2023
Resolves #539. These are based on the corresponding tests in the Android codebase at commit f6d163f (plus a fix added in eea952b). They stick as closely as possible to the structure and behaviour of those tests, to which I have not applied much of a critical eye. The majority of the log messages and comments are simply copied across. The only structural difference between these tests and the Android ones is that I have removed the caller’s responsibility to call `SubscriberMonitor#close`. There are places in the Android implementation where we forget to call it, and also it isn’t called if an error is thrown by `waitForStateTransition`. By making the subscriber monitor responsible for its own cleanup, we remove these issues. When I started writing these tests, we were still targeting iOS 12 and above, which meant it was not possible to use Swift concurrency. Hence, all of the code is written using either completion handlers or blocking functions. Since then, we’ve decided to set our deployment target to iOS 13 (see #597), which means we could now use Swift concurrency. However, by that point I was well into the writing of these tests and did not want to re-structure them, especially since they require functionality that doesn’t exist out of the box in Swift concurrency (for example timeouts). We may wish to revisit at some point and switch to using Swift concurrency, but I don’t think it’s urgent. In order to generate the list of which faults to skip, I ran all of the tests and checked which ones failed. Whilst doing so, I encountered a crash in ably-cocoa, which I believe is already reported as ably/ably-cocoa#1380. In order to get past this issue, I needed to use the `main` version of ably-cocoa, which contains a fix for it. I’ve created #641 to make sure we return to using a proper release of ably-cocoa before we make our next AAT release. (Note that the huge diff in the Package.resolved files is the version 2 change that I mentioned in 034502b.)
lawrence-forooghian
added
code-quality
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
In c8e1f88 I switched to using a fixed commit of ably-cocoa instead of a numbered release. We should switch back to using a numbered release once the next version of ably-cocoa is released, and definitely before we do our next release of AAT.
The text was updated successfully, but these errors were encountered: