From 44ce8882eb6ba4df168fa70422e0b6291fd853ff Mon Sep 17 00:00:00 2001
From: Philipp Hofmann <philipp.hofmann@sentry.io>
Date: Tue, 14 Nov 2023 13:59:28 +0100
Subject: [PATCH] test: Improve SentryReachabilityTests (#3399)

The SentryReachabilityTests are still sometimes failing in CI. We now
skip registering and unregistering the actual callbacks to
SCNetworkReachability for all tests except one to minimize side effects.
---
 Sources/Sentry/SentryReachability.m           | 35 ++++++++++++++++++-
 Sources/Sentry/include/SentryReachability.h   | 13 +++++++
 .../Networking/SentryReachabilityTests.m      | 18 +++++++++-
 3 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/Sources/Sentry/SentryReachability.m b/Sources/Sentry/SentryReachability.m
index 930b5fea345..fd94a7d8559 100644
--- a/Sources/Sentry/SentryReachability.m
+++ b/Sources/Sentry/SentryReachability.m
@@ -35,17 +35,21 @@
 static SCNetworkReachabilityFlags sentry_current_reachability_state
     = kSCNetworkReachabilityFlagsUninitialized;
 static dispatch_queue_t sentry_reachability_queue;
-static BOOL sentry_reachability_ignore_actual_callback = NO;
 
 NSString *const SentryConnectivityCellular = @"cellular";
 NSString *const SentryConnectivityWiFi = @"wifi";
 NSString *const SentryConnectivityNone = @"none";
 
+#    if TEST || TESTCI
+static BOOL sentry_reachability_ignore_actual_callback = NO;
+
 void
 SentrySetReachabilityIgnoreActualCallback(BOOL value)
 {
+    SENTRY_LOG_DEBUG(@"Setting ignore actual callback to %@", value ? @"YES" : @"NO");
     sentry_reachability_ignore_actual_callback = value;
 }
+#    endif // TEST || TESTCI
 
 /**
  * Check whether the connectivity change should be noted or ignored.
@@ -132,10 +136,13 @@
 {
     SENTRY_LOG_DEBUG(
         @"SentryConnectivityCallback called with target: %@; flags: %u", target, flags);
+#    if TEST || TESTCI
     if (sentry_reachability_ignore_actual_callback) {
         SENTRY_LOG_DEBUG(@"Ignoring actual callback.");
         return;
     }
+#    endif // TEST || TESTCI
+
     SentryConnectivityCallback(flags);
 }
 
@@ -155,6 +162,19 @@ + (void)initialize
     }
 }
 
+#    if TEST || TESTCI
+
+- (instancetype)init
+{
+    if (self = [super init]) {
+        self.skipRegisteringActualCallbacks = NO;
+    }
+
+    return self;
+}
+
+#    endif // TEST || TESTCI
+
 - (void)addObserver:(id<SentryReachabilityObserver>)observer;
 {
     SENTRY_LOG_DEBUG(@"Adding observer: %@", observer);
@@ -171,6 +191,13 @@ - (void)addObserver:(id<SentryReachabilityObserver>)observer;
             return;
         }
 
+#    if TEST || TESTCI
+        if (self.skipRegisteringActualCallbacks) {
+            SENTRY_LOG_DEBUG(@"Skip registering actual callbacks");
+            return;
+        }
+#    endif // TEST || TESTCI
+
         sentry_reachability_queue
             = dispatch_queue_create("io.sentry.cocoa.connectivity", DISPATCH_QUEUE_SERIAL);
         // Ensure to call CFRelease for the return value of SCNetworkReachabilityCreateWithName, see
@@ -214,6 +241,12 @@ - (void)removeAllObservers
 
 - (void)unsetReachabilityCallback
 {
+#    if TEST || TESTCI
+    if (self.skipRegisteringActualCallbacks) {
+        SENTRY_LOG_DEBUG(@"Skip unsetting actual callbacks");
+    }
+#    endif // TEST || TESTCI
+
     sentry_current_reachability_state = kSCNetworkReachabilityFlagsUninitialized;
 
     if (_sentry_reachability_ref != nil) {
diff --git a/Sources/Sentry/include/SentryReachability.h b/Sources/Sentry/include/SentryReachability.h
index 4a645df7491..a40566d3cfe 100644
--- a/Sources/Sentry/include/SentryReachability.h
+++ b/Sources/Sentry/include/SentryReachability.h
@@ -34,11 +34,14 @@ NS_ASSUME_NONNULL_BEGIN
 
 void SentryConnectivityCallback(SCNetworkReachabilityFlags flags);
 
+#    if TEST || TESTCI
 /**
  * Needed for testing.
  */
 void SentrySetReachabilityIgnoreActualCallback(BOOL value);
 
+#    endif // TEST || TESTCI
+
 NSString *SentryConnectivityFlagRepresentation(SCNetworkReachabilityFlags flags);
 
 BOOL SentryConnectivityShouldReportChange(SCNetworkReachabilityFlags flags);
@@ -65,6 +68,16 @@ SENTRY_EXTERN NSString *const SentryConnectivityNone;
  */
 @interface SentryReachability : NSObject
 
+#    if TEST || TESTCI
+
+/**
+ * Only needed for testing. Use this flag to skip registering and unregistering the actual callbacks
+ * to SCNetworkReachability to minimize side effects.
+ */
+@property (nonatomic, assign) BOOL skipRegisteringActualCallbacks;
+
+#    endif // TEST || TESTCI
+
 /**
  * Add an observer which is called each time network connectivity changes.
  */
diff --git a/Tests/SentryTests/Networking/SentryReachabilityTests.m b/Tests/SentryTests/Networking/SentryReachabilityTests.m
index ca0b28dc094..147951c05e5 100644
--- a/Tests/SentryTests/Networking/SentryReachabilityTests.m
+++ b/Tests/SentryTests/Networking/SentryReachabilityTests.m
@@ -35,11 +35,13 @@ @implementation SentryReachabilityTest
 
 - (void)setUp
 {
-    self.reachability = [[SentryReachability alloc] init];
     // Ignore the actual reachability callbacks, cause we call the callbacks manually.
     // Otherwise, the actual reachability callbacks are called during later unrelated tests causing
     // flakes.
     SentrySetReachabilityIgnoreActualCallback(YES);
+
+    self.reachability = [[SentryReachability alloc] init];
+    self.reachability.skipRegisteringActualCallbacks = YES;
 }
 
 - (void)tearDown
@@ -145,5 +147,19 @@ - (void)testReportSameReachabilityState_OnlyCalledOnce
     [self.reachability removeObserver:observer];
 }
 
+/**
+ * We only want to make sure running the actual registering and unregistering callbacks doesn't
+ * crash.
+ */
+- (void)testRegisteringActualCallbacks
+{
+    self.reachability.skipRegisteringActualCallbacks = NO;
+
+    TestSentryReachabilityObserver *observer = [[TestSentryReachabilityObserver alloc] init];
+
+    [self.reachability addObserver:observer];
+    [self.reachability removeObserver:observer];
+}
+
 @end
 #endif // !TARGET_OS_WATCH