Create an issue for subscriber `NetworkConnectivityTests` failures #575

lawrence-forooghian · 2023-02-20T19:48:34Z

As #574, but after implementing subscriber tests in #539.

sync-by-unito · 2023-02-20T19:50:29Z

➤ Automation for Jira commented:

The link to the corresponding Jira issue is https://ably.atlassian.net/browse/SDK-3389

Based on Android at f6d163f TODO explain why it's not using async await (also I have no idea how to do timeouts) differences: - always close susbcriber monitor’s Ably so that it's closed if error and so we don't forget to do it (like Android does someitmes) TODO there is a crash seemingly due to the bad access in ably-cocoa reachability TODO explain re ably-cocoa: I was using "branch" : "fix/1380-dispatch-ARTOSReachability_Callback", "revision" : "036be28852b3ef1b3a112aea7052c463c5b8792a" because of reachability crashes related to ably/ably-cocoa#1380 internal thread: https://ably-real-time.slack.com/archives/CSQEKCE81/p1678802535684519 but it appears (although perhaps it was more sporadic than I imagined) all of the affected tests fail anyway, so now that I’ve marked those tests as skipped, I’ve reverted that, and we can deal with it when we try re-enabling those tests in #575, by which point hopefully the fix for ably-cocoa#1380 will have been released. (I don't want AAT to use an ably-cocoa branch.)

Based on Android at f6d163f TODO explain why it's not using async await (also I have no idea how to do timeouts) differences: - always close susbcriber monitor’s Ably so that it's closed if error and so we don't forget to do it (like Android does someitmes) TODO there is a crash seemingly due to the bad access in ably-cocoa reachability TODO explain re ably-cocoa: I was using "branch" : "fix/1380-dispatch-ARTOSReachability_Callback", "revision" : "036be28852b3ef1b3a112aea7052c463c5b8792a" because of reachability crashes related to ably/ably-cocoa#1380 internal thread: https://ably-real-time.slack.com/archives/CSQEKCE81/p1678802535684519 but it appears (although perhaps it was more sporadic than I imagined) all of the affected tests fail anyway, so now that I’ve marked those tests as skipped, I’ve reverted that, and we can deal with it when we try re-enabling those tests in #575, by which point hopefully the fix for ably-cocoa#1380 will have been released. (I don't want AAT to use an ably-cocoa branch.) TODO check docstrings

These are based on the corresponding tests in the Android codebase at commit f6d163f. 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 tried using Marat’s fix from ably-cocoa branch `fix/1380-dispatch-ARTOSReachability_Callback` (commit 036be28). This removed the crashes. The list of skipped tests is the list of tests that failed when using this branch. It then actually turned out that the non-skipped tests do not exhibit this crash anyway, so I didn’t include any version change to ably-cocoa in this PR. We can deal with this crash when we try re-enabling the tests in #575, by which point hopefully the fix for ably-cocoa#1380 will have been released. (Internal thread re this crash and its appearance in these tests is [1].) TODO check copied docstrings

Resolves #539. These are based on the corresponding tests in the Android codebase at commit f6d163f. 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 tried using Marat’s fix from ably-cocoa branch `fix/1380-dispatch-ARTOSReachability_Callback` (commit 036be28). This removed the crashes. The list of skipped tests is the list of tests that failed when using this branch. It then actually turned out that the non-skipped tests do not exhibit this crash anyway, so I didn’t include any version change to ably-cocoa in this PR. We can deal with this crash when we try re-enabling the tests in #575, by which point hopefully the fix for ably-cocoa#1380 will have been released. (Internal thread re this crash and its appearance in these tests is [1].) TODO check copied docstrings

Resolves #539. These are based on the corresponding tests in the Android codebase at commit f6d163f. 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 tried using Marat’s fix from ably-cocoa branch `fix/1380-dispatch-ARTOSReachability_Callback` (commit 036be28). This removed the crashes. The list of skipped tests is the list of tests that failed when using this branch. It then actually turned out that the non-skipped tests do not exhibit this crash anyway, so I didn’t include any version change to ably-cocoa in this PR. We can deal with this crash when we try re-enabling the tests in #575, by which point hopefully the fix for ably-cocoa#1380 will have been released. (Internal thread re this crash and its appearance in these tests is [1].)

Resolves #539. These are based on the corresponding tests in the Android codebase at commit f6d163f. 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 tried using Marat’s fix from ably-cocoa branch `fix/1380-dispatch-ARTOSReachability_Callback` (commit 036be28). This removed the crashes. The list of skipped tests is the list of tests that failed when using this branch. It then actually turned out that the non-skipped tests do not exhibit this crash anyway, so I didn’t include any version change to ably-cocoa in this PR. We can deal with this crash when we try re-enabling the tests in #575, by which point hopefully the fix for ably-cocoa#1380 will have been released. (Internal thread re this crash and its appearance in these tests is [1].) [1] https://ably-real-time.slack.com/archives/CSQEKCE81/p1678802535684519

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 tried using Marat’s fix from ably-cocoa branch `fix/1380-dispatch-ARTOSReachability_Callback` (commit 036be28). This removed the crashes. The list of skipped tests is the list of tests that failed when using this branch. It then actually turned out that the non-skipped tests do not exhibit this crash anyway, so I didn’t include any version change to ably-cocoa in this PR. We can deal with this crash when we try re-enabling the tests in #575, by which point hopefully the fix for ably-cocoa#1380 will have been released. (Internal thread re this crash and its appearance in these tests is [1].) [1] https://ably-real-time.slack.com/archives/CSQEKCE81/p1678802535684519

lawrence-forooghian mentioned this issue Feb 20, 2023

Add network connectivity integration tests #535

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create an issue for subscriber `NetworkConnectivityTests` failures #575

Create an issue for subscriber `NetworkConnectivityTests` failures #575

lawrence-forooghian commented Feb 20, 2023 •

edited by sync-by-unito bot

Loading

sync-by-unito bot commented Feb 20, 2023

Create an issue for subscriber NetworkConnectivityTests failures #575

Create an issue for subscriber NetworkConnectivityTests failures #575

Comments

lawrence-forooghian commented Feb 20, 2023 • edited by sync-by-unito bot Loading

sync-by-unito bot commented Feb 20, 2023

Create an issue for subscriber `NetworkConnectivityTests` failures #575

Create an issue for subscriber `NetworkConnectivityTests` failures #575

lawrence-forooghian commented Feb 20, 2023 •

edited by sync-by-unito bot

Loading