Merge pull request #1540 from bugsnag/PLAT-7614/avoid-duplicate-conne…

…ctivity-breadcrumb

Avoid unnecessary network connectivity change breadcrumb

CHANGELOG.md

@@ -12,6 +12,9 @@
 * Flush in-memory sessions first
   [#1538](https://github.com/bugsnag/bugsnag-android/pull/1538)
 
+* Avoid unnecessary network connectivity change breadcrumb
+  [#1540](https://github.com/bugsnag/bugsnag-android/pull/1540)
+
 ## 5.16.0 (2021-11-29)
 
 ### Bug fixes

bugsnag-android-core/src/main/java/com/bugsnag/android/ConnectivityCompat.kt

@@ -9,6 +9,9 @@ import android.net.Network
 import android.net.NetworkCapabilities
 import android.os.Build
 import androidx.annotation.RequiresApi
+import androidx.annotation.VisibleForTesting
+import com.bugsnag.android.UnknownConnectivity.retrieveNetworkAccessState
+import java.util.concurrent.atomic.AtomicBoolean
 
 internal typealias NetworkChangeCallback = (hasConnection: Boolean, networkState: String) -> Unit
 
@@ -89,8 +92,10 @@ internal class ConnectivityLegacy(
         }
     }
 
-    private inner class ConnectivityChangeReceiver(private val cb: NetworkChangeCallback?) :
-        BroadcastReceiver() {
+    private inner class ConnectivityChangeReceiver(
+        private val cb: NetworkChangeCallback?
+    ) : BroadcastReceiver() {
+
         override fun onReceive(context: Context, intent: Intent) {
             cb?.invoke(hasNetworkConnection(), retrieveNetworkAccessState())
         }
@@ -122,22 +127,38 @@ internal class ConnectivityApi24(
         }
     }
 
-    private inner class ConnectivityTrackerCallback(private val cb: NetworkChangeCallback?) :
-        ConnectivityManager.NetworkCallback() {
+    @VisibleForTesting
+    internal class ConnectivityTrackerCallback(
+        private val cb: NetworkChangeCallback?
+    ) : ConnectivityManager.NetworkCallback() {
+
+        private val receivedFirstCallback = AtomicBoolean(false)
+
         override fun onUnavailable() {
             super.onUnavailable()
-            cb?.invoke(false, retrieveNetworkAccessState())
+            invokeNetworkCallback(false)
         }
 
         override fun onAvailable(network: Network) {
             super.onAvailable(network)
-            cb?.invoke(true, retrieveNetworkAccessState())
+            invokeNetworkCallback(true)
+        }
+
+        /**
+         * Invokes the network callback, as long as the ConnectivityManager callback has been
+         * triggered at least once before (when setting a NetworkCallback Android always
+         * invokes the callback with the current network state).
+         */
+        private fun invokeNetworkCallback(hasConnection: Boolean) {
+            if (receivedFirstCallback.getAndSet(true)) {
+                cb?.invoke(hasConnection, retrieveNetworkAccessState())
+            }
         }
     }
 }
 
 /**
- * Connectivity used in cases where we cannot access the system  ConnectivityManager.
+ * Connectivity used in cases where we cannot access the system ConnectivityManager.
  * We assume that there is some sort of network and do not attempt to report any network changes.
  */
 internal object UnknownConnectivity : Connectivity {

bugsnag-android-core/src/test/java/com/bugsnag/android/ConnectivityApi24Test.kt

@@ -1,5 +1,3 @@
-@file:Suppress("DEPRECATION")
-
 package com.bugsnag.android
 
 import android.content.Context
@@ -31,15 +29,15 @@ class ConnectivityApi24Test {
     lateinit var capabilities: NetworkCapabilities
 
     @Test
-    fun connectivityLegacyHasConnection() {
+    fun connectivityApi24HasConnection() {
         val conn = ConnectivityApi24(cm, null)
         assertFalse(conn.hasNetworkConnection())
         Mockito.`when`(cm.activeNetwork).thenReturn(info)
         assertTrue(conn.hasNetworkConnection())
     }
 
     @Test
-    fun connectivityLegacyNetworkState() {
+    fun connectivityApi24NetworkState() {
         val conn = ConnectivityApi24(cm, null)
         Mockito.`when`(cm.activeNetwork).thenReturn(info)
 
@@ -50,4 +48,40 @@ class ConnectivityApi24Test {
         Mockito.`when`(capabilities.hasTransport(0)).thenReturn(true)
         assertEquals("cellular", conn.retrieveNetworkAccessState())
     }
+
+    @Test
+    fun connectivityApi24OnAvailable() {
+        var count = 0
+        val tracker = ConnectivityApi24.ConnectivityTrackerCallback { _, _ ->
+            count++
+        }
+        assertEquals(0, count)
+
+        tracker.onAvailable(info)
+        assertEquals(0, count)
+
+        tracker.onAvailable(info)
+        assertEquals(1, count)
+
+        tracker.onAvailable(info)
+        assertEquals(2, count)
+    }
+
+    @Test
+    fun connectivityApi24OnUnvailable() {
+        var count = 0
+        val tracker = ConnectivityApi24.ConnectivityTrackerCallback { _, _ ->
+            count++
+        }
+        assertEquals(0, count)
+
+        tracker.onUnavailable()
+        assertEquals(0, count)
+
+        tracker.onUnavailable()
+        assertEquals(1, count)
+
+        tracker.onUnavailable()
+        assertEquals(2, count)
+    }
 }

features/steps/android_steps.rb

@@ -276,11 +276,9 @@ def click_if_present(element)
 # which results in multiple similar log messages
 Then("Bugsnag confirms it has no errors to send") do
   steps %Q{
-    And I wait to receive 3 logs
+    And I wait to receive 2 logs
     Then the "debug" level log message equals "No startupcrash events to flush to Bugsnag."
     And I discard the oldest log
     Then the "debug" level log message equals "No regular events to flush to Bugsnag."
-    And I discard the oldest log
-    Then the "debug" level log message equals "No regular events to flush to Bugsnag."
   }
 end