RUMM-2497 SnapshotProcessor - do not write anything if there was no m…

…utation resolved
DataDog · Sep 12, 2022 · 35c40fb · 35c40fb
1 parent 6178adb
commit 35c40fb
Show file tree

Hide file tree

Showing 5 changed files with 155 additions and 65 deletions.
diff --git a/detekt.yml b/detekt.yml
@@ -1039,6 +1039,7 @@ datadog:
       - "kotlin.collections.List.forEach(kotlin.Function1)"
       - "kotlin.collections.List.getOrNull(kotlin.Int)"
       - "kotlin.collections.List.isEmpty()"
+      - "kotlin.collections.List.isNotEmpty()"
       - "kotlin.collections.List.joinToString(kotlin.CharSequence, kotlin.CharSequence, kotlin.CharSequence, kotlin.Int, kotlin.CharSequence, kotlin.Function1?)"
       - "kotlin.collections.List.lastOrNull()"
       - "kotlin.collections.List.lastOrNull(kotlin.Function1)"

diff --git a/...on-replay/src/main/kotlin/com/datadog/android/sessionreplay/processor/MutationResolver.kt b/...on-replay/src/main/kotlin/com/datadog/android/sessionreplay/processor/MutationResolver.kt
@@ -14,7 +14,7 @@ internal class MutationResolver {
     fun resolveMutations(
         prevSnapshot: List<MobileSegment.Wireframe>,
         currentSnapshot: List<MobileSegment.Wireframe>
-    ): MobileSegment.MobileIncrementalData.MobileMutationData {
+    ): MobileSegment.MobileIncrementalData.MobileMutationData? {
         val prevSnapshotAsMap = prevSnapshot.associateBy { it.id() }.toMutableMap()
         val updates: MutableList<MobileSegment.WireframeUpdateMutation> = LinkedList()
         val adds: MutableList<MobileSegment.Add> = LinkedList()
@@ -31,11 +31,16 @@ internal class MutationResolver {
             prevWireframe = currentWireframe
         }
         val removes = prevSnapshotAsMap.map { MobileSegment.Remove(it.key) }
-        return MobileSegment.MobileIncrementalData.MobileMutationData(
-            adds = adds,
-            removes = removes,
-            updates = updates
-        )
+
+        return if (adds.isNotEmpty() || removes.isNotEmpty() || updates.isNotEmpty()) {
+            MobileSegment.MobileIncrementalData.MobileMutationData(
+                adds = adds,
+                removes = removes,
+                updates = updates
+            )
+        } else {
+            null
+        }
     }
 
     private fun resolveUpdateMutation(

diff --git a/...n-replay/src/main/kotlin/com/datadog/android/sessionreplay/processor/SnapshotProcessor.kt b/...n-replay/src/main/kotlin/com/datadog/android/sessionreplay/processor/SnapshotProcessor.kt
@@ -137,20 +137,27 @@ internal class SnapshotProcessor(
             records.add(focusRecord)
         }
 
-        val record = if (fullSnapshotRequired) {
-            MobileSegment.MobileRecord.MobileFullSnapshotRecord(
-                timestamp,
-                MobileSegment.Data(wireframes)
+        if (fullSnapshotRequired) {
+            records.add(
+                MobileSegment.MobileRecord.MobileFullSnapshotRecord(
+                    timestamp,
+                    MobileSegment.Data(wireframes)
+                )
             )
         } else {
-            MobileSegment.MobileRecord.MobileIncrementalSnapshotRecord(
-                timestamp,
-                mutationResolver.resolveMutations(prevSnapshot, wireframes)
-            )
+            mutationResolver.resolveMutations(prevSnapshot, wireframes)?.let {
+                records.add(
+                    MobileSegment.MobileRecord.MobileIncrementalSnapshotRecord(
+                        timestamp,
+                        it
+                    )
+                )
+            }
         }
-        records.add(record)
         prevSnapshot = wireframes
-        writer.write(bundleRecordInEnrichedRecord(newRumContext, records))
+        if (records.isNotEmpty()) {
+            writer.write(bundleRecordInEnrichedRecord(newRumContext, records))
+        }
     }
 
     private fun isLastFullSnapshotTime(): Boolean {

diff --git a/...eplay/src/test/kotlin/com/datadog/android/sessionreplay/processor/MutationResolverTest.kt b/...eplay/src/test/kotlin/com/datadog/android/sessionreplay/processor/MutationResolverTest.kt
@@ -11,6 +11,7 @@ import com.datadog.android.sessionreplay.utils.ForgeConfigurator
 import fr.xgouchet.elmyr.Forge
 import fr.xgouchet.elmyr.junit5.ForgeConfiguration
 import fr.xgouchet.elmyr.junit5.ForgeExtension
+import java.util.ArrayList
 import java.util.LinkedList
 import org.assertj.core.api.Assertions.assertThat
 import org.junit.jupiter.api.BeforeEach
@@ -60,9 +61,9 @@ internal class MutationResolverTest {
         )
 
         // Then
-        assertThat(mutations.adds).isEqualTo(expectedAdditions)
-        assertThat(mutations.removes).isNullOrEmpty()
-        assertThat(mutations.updates).isNullOrEmpty()
+        assertThat(mutations?.adds).isEqualTo(expectedAdditions)
+        assertThat(mutations?.removes).isNullOrEmpty()
+        assertThat(mutations?.updates).isNullOrEmpty()
     }
 
     @Test
@@ -87,9 +88,9 @@ internal class MutationResolverTest {
         )
 
         // Then
-        assertThat(mutations.adds).isEqualTo(expectedAdds)
-        assertThat(mutations.removes).isNullOrEmpty()
-        assertThat(mutations.updates).isNullOrEmpty()
+        assertThat(mutations?.adds).isEqualTo(expectedAdds)
+        assertThat(mutations?.removes).isNullOrEmpty()
+        assertThat(mutations?.updates).isNullOrEmpty()
     }
 
     // endregion
@@ -115,9 +116,9 @@ internal class MutationResolverTest {
         )
 
         // Then
-        assertThat(mutations.adds).isNullOrEmpty()
-        assertThat(mutations.removes).isEqualTo(expectedRemovals)
-        assertThat(mutations.updates).isNullOrEmpty()
+        assertThat(mutations?.adds).isNullOrEmpty()
+        assertThat(mutations?.removes).isEqualTo(expectedRemovals)
+        assertThat(mutations?.updates).isNullOrEmpty()
     }
 
     // endregion
@@ -150,9 +151,9 @@ internal class MutationResolverTest {
         )
 
         // Then
-        assertThat(mutations.adds).isNullOrEmpty()
-        assertThat(mutations.removes).isNullOrEmpty()
-        assertThat(mutations.updates).isEqualTo(expectedUpdates)
+        assertThat(mutations?.adds).isNullOrEmpty()
+        assertThat(mutations?.removes).isNullOrEmpty()
+        assertThat(mutations?.updates).isEqualTo(expectedUpdates)
     }
 
     @Test
@@ -181,9 +182,9 @@ internal class MutationResolverTest {
         )
 
         // Then
-        assertThat(mutations.adds).isNullOrEmpty()
-        assertThat(mutations.removes).isNullOrEmpty()
-        assertThat(mutations.updates).isEqualTo(expectedUpdates)
+        assertThat(mutations?.adds).isNullOrEmpty()
+        assertThat(mutations?.removes).isNullOrEmpty()
+        assertThat(mutations?.updates).isEqualTo(expectedUpdates)
     }
 
     @Test
@@ -218,9 +219,9 @@ internal class MutationResolverTest {
         )
 
         // Then
-        assertThat(mutations.adds).isNullOrEmpty()
-        assertThat(mutations.removes).isNullOrEmpty()
-        assertThat(mutations.updates).isEqualTo(expectedUpdates)
+        assertThat(mutations?.adds).isNullOrEmpty()
+        assertThat(mutations?.removes).isNullOrEmpty()
+        assertThat(mutations?.updates).isEqualTo(expectedUpdates)
     }
 
     @Test
@@ -256,9 +257,9 @@ internal class MutationResolverTest {
         )
 
         // Then
-        assertThat(mutations.adds).isNullOrEmpty()
-        assertThat(mutations.removes).isNullOrEmpty()
-        assertThat(mutations.updates).isEqualTo(expectedUpdates)
+        assertThat(mutations?.adds).isNullOrEmpty()
+        assertThat(mutations?.removes).isNullOrEmpty()
+        assertThat(mutations?.updates).isEqualTo(expectedUpdates)
     }
 
     @Test
@@ -284,9 +285,9 @@ internal class MutationResolverTest {
         )
 
         // Then
-        assertThat(mutations.adds).isNullOrEmpty()
-        assertThat(mutations.removes).isNullOrEmpty()
-        assertThat(mutations.updates).isNullOrEmpty()
+        assertThat(mutations?.adds).isNullOrEmpty()
+        assertThat(mutations?.removes).isNullOrEmpty()
+        assertThat(mutations?.updates).isNullOrEmpty()
     }
 
     // endregion
@@ -319,9 +320,9 @@ internal class MutationResolverTest {
         )
 
         // Then
-        assertThat(mutations.adds).isNullOrEmpty()
-        assertThat(mutations.removes).isNullOrEmpty()
-        assertThat(mutations.updates).isEqualTo(expectedUpdates)
+        assertThat(mutations?.adds).isNullOrEmpty()
+        assertThat(mutations?.removes).isNullOrEmpty()
+        assertThat(mutations?.updates).isEqualTo(expectedUpdates)
     }
 
     @Test
@@ -350,9 +351,9 @@ internal class MutationResolverTest {
         )
 
         // Then
-        assertThat(mutations.adds).isNullOrEmpty()
-        assertThat(mutations.removes).isNullOrEmpty()
-        assertThat(mutations.updates).isEqualTo(expectedUpdates)
+        assertThat(mutations?.adds).isNullOrEmpty()
+        assertThat(mutations?.removes).isNullOrEmpty()
+        assertThat(mutations?.updates).isEqualTo(expectedUpdates)
     }
 
     @Test
@@ -387,9 +388,9 @@ internal class MutationResolverTest {
         )
 
         // Then
-        assertThat(mutations.adds).isNullOrEmpty()
-        assertThat(mutations.removes).isNullOrEmpty()
-        assertThat(mutations.updates).isEqualTo(expectedUpdates)
+        assertThat(mutations?.adds).isNullOrEmpty()
+        assertThat(mutations?.removes).isNullOrEmpty()
+        assertThat(mutations?.updates).isEqualTo(expectedUpdates)
     }
 
     @Test
@@ -425,9 +426,9 @@ internal class MutationResolverTest {
         )
 
         // Then
-        assertThat(mutations.adds).isNullOrEmpty()
-        assertThat(mutations.removes).isNullOrEmpty()
-        assertThat(mutations.updates).isEqualTo(expectedUpdates)
+        assertThat(mutations?.adds).isNullOrEmpty()
+        assertThat(mutations?.removes).isNullOrEmpty()
+        assertThat(mutations?.updates).isEqualTo(expectedUpdates)
     }
 
     @Test
@@ -455,9 +456,9 @@ internal class MutationResolverTest {
         )
 
         // Then
-        assertThat(mutations.adds).isNullOrEmpty()
-        assertThat(mutations.removes).isNullOrEmpty()
-        assertThat(mutations.updates).isEqualTo(expectedUpdates)
+        assertThat(mutations?.adds).isNullOrEmpty()
+        assertThat(mutations?.removes).isNullOrEmpty()
+        assertThat(mutations?.updates).isEqualTo(expectedUpdates)
     }
 
     @Test
@@ -493,9 +494,9 @@ internal class MutationResolverTest {
         )
 
         // Then
-        assertThat(mutations.adds).isNullOrEmpty()
-        assertThat(mutations.removes).isNullOrEmpty()
-        assertThat(mutations.updates).isEqualTo(expectedUpdates)
+        assertThat(mutations?.adds).isNullOrEmpty()
+        assertThat(mutations?.removes).isNullOrEmpty()
+        assertThat(mutations?.updates).isEqualTo(expectedUpdates)
     }
 
     @Test
@@ -542,13 +543,13 @@ internal class MutationResolverTest {
         )
 
         // Then
-        assertThat(mutations.adds).isNullOrEmpty()
-        assertThat(mutations.removes).isNullOrEmpty()
-        assertThat(mutations.updates).isEqualTo(expectedUpdates)
+        assertThat(mutations?.adds).isNullOrEmpty()
+        assertThat(mutations?.removes).isNullOrEmpty()
+        assertThat(mutations?.updates).isEqualTo(expectedUpdates)
     }
 
     @Test
-    fun `M return empty mutations W resolveMutation {Text wireframes types are not matching }`(
+    fun `M return null W resolveMutation {Text wireframes types are not matching }`(
         forge: Forge
     ) {
         // Given
@@ -570,9 +571,53 @@ internal class MutationResolverTest {
         )
 
         // Then
-        assertThat(mutations.adds).isNullOrEmpty()
-        assertThat(mutations.removes).isNullOrEmpty()
-        assertThat(mutations.updates).isNullOrEmpty()
+        assertThat(mutations).isNull()
+    }
+
+    // endregion
+
+    // region no mutation
+
+    @Test
+    fun `M return null W resolveMutation { Text wireframes, no mutation }`(
+        forge: Forge
+    ) {
+        // Given
+        val fakePrevSnapshot = forge.aList(size = forge.anInt(min = 2, max = 10)) {
+            forge.getForgery(MobileSegment.Wireframe.TextWireframe::class.java)
+        }
+
+        val fakeCurrentSnapshot = ArrayList(fakePrevSnapshot)
+
+        // When
+        val mutations = testedMutationResolver.resolveMutations(
+            fakePrevSnapshot,
+            fakeCurrentSnapshot
+        )
+
+        // Then
+        assertThat(mutations).isNull()
+    }
+
+    @Test
+    fun `M return null W resolveMutation { Shape wireframes, no mutation }`(
+        forge: Forge
+    ) {
+        // Given
+        val fakePrevSnapshot = forge.aList(size = forge.anInt(min = 2, max = 10)) {
+            forge.getForgery(MobileSegment.Wireframe.ShapeWireframe::class.java)
+        }
+
+        val fakeCurrentSnapshot = ArrayList(fakePrevSnapshot)
+
+        // When
+        val mutations = testedMutationResolver.resolveMutations(
+            fakePrevSnapshot,
+            fakeCurrentSnapshot
+        )
+
+        // Then
+        assertThat(mutations).isNull()
     }
 
     // endregion

diff --git a/...play/src/test/kotlin/com/datadog/android/sessionreplay/processor/SnapshotProcessorTest.kt b/...play/src/test/kotlin/com/datadog/android/sessionreplay/processor/SnapshotProcessorTest.kt
@@ -483,6 +483,38 @@ internal class SnapshotProcessorTest {
         assertThat(incrementalSnapshotRecord.data).isEqualTo(fakeMutationData)
     }
 
+    @Test
+    fun `M do nothing W process { no mutation was detected }`(forge: Forge) {
+        // Given
+        val fakeSnapshot1 = forge.aSingleLevelSnapshot()
+        val fakeSnapshot2 = fakeSnapshot1.copy()
+        val fakeFlattenedSnapshot1 = forge.aList {
+            getForgery(MobileSegment.Wireframe::class.java)
+        }
+        val fakeFlattenedSnapshot2 = ArrayList(fakeFlattenedSnapshot1)
+        whenever(mockNodeFlattener.flattenNode(fakeSnapshot1)).thenReturn(fakeFlattenedSnapshot1)
+        whenever(mockNodeFlattener.flattenNode(fakeSnapshot2)).thenReturn(fakeFlattenedSnapshot2)
+        whenever(
+            mockMutationResolver.resolveMutations(
+                fakeFlattenedSnapshot1,
+                fakeFlattenedSnapshot2
+            )
+        ).thenReturn(null)
+        testedProcessor.process(fakeSnapshot1)
+
+        // When
+        testedProcessor.process(fakeSnapshot2)
+
+        // Then
+        // We should only send the FullSnapshotRecord. The IncrementalSnapshotRecord will not be
+        // send as there was no mutation data detected.
+        val captor = argumentCaptor<EnrichedRecord>()
+        verify(mockWriter, times(1)).write(captor.capture())
+        assertThat(captor.firstValue.records.size).isEqualTo(3)
+        assertThat(captor.firstValue.records[2])
+            .isInstanceOf(MobileSegment.MobileRecord.MobileFullSnapshotRecord::class.java)
+    }
+
     // region TouchData
 
     @Test