RUMM-2423 SnapshotProcessor - process wireframe mutations#1033
Conversation
74b77d9 to
b63c394
Compare
b63c394 to
beaf333
Compare
Codecov Report
@@ Coverage Diff @@
## feature/session-replay-vo #1033 +/- ##
=============================================================
+ Coverage 82.99% 83.22% +0.24%
=============================================================
Files 307 323 +16
Lines 9980 10424 +444
Branches 1628 1734 +106
=============================================================
+ Hits 8282 8675 +393
- Misses 1193 1210 +17
- Partials 505 539 +34
|
0xnm
left a comment
There was a problem hiding this comment.
Overall lgtm, I left some questions to clarify the code.
| adds = adds, | ||
| removes = removes, | ||
| updates = updates |
There was a problem hiding this comment.
Nitpick: I'd rename those fields: additions and removals. A field should be a noun, and verbes should be used for functions
There was a problem hiding this comment.
this is part of the JSON schemas we agreed upon. If you don't mind we can add this as an extra task later.
|
|
||
| // TODO: RUMM-2481 Use the `diff` method int the ShapeWireframe type when available | ||
| @Suppress("ComplexMethod") | ||
| private fun resolveUpdateMutation( |
There was a problem hiding this comment.
Nitpick: Just for clarity, I'd rename that one resolveShapeUpdateMutation and the other one resolveTextUpdateMutation.
| private fun MobileSegment.Wireframe.id(): Long { | ||
| return when (this) { | ||
| is MobileSegment.Wireframe.ShapeWireframe -> this.id | ||
| is MobileSegment.Wireframe.TextWireframe -> this.id | ||
| } | ||
| } |
There was a problem hiding this comment.
Wouldn't this one be better in the MobileSegmentExt.kt file?
| writer.write(bundleRecordInEnrichedRecord(newRumContext, records)) | ||
| } | ||
|
|
||
| private fun checkLastFullSnapshotTime(): Boolean { |
There was a problem hiding this comment.
Nitpick: I'd rename that isLastFullSnapshotTime
| val fakeAddedWireframes = forge.aList { forge.getForgery<MobileSegment.Wireframe>() } | ||
| val fakeCurrentSnapshot = fakePrevSnapshot.toMutableList() + fakeAddedWireframes | ||
| var lastPrevWireframe = fakePrevSnapshot.last() | ||
| val expectedAdds = LinkedList<MobileSegment.Add>() |
There was a problem hiding this comment.
Nitpick: expectedAdditions
| } | ||
| val fakeRemovedSize = forge.anInt(min = 1, max = fakePrevSnapshot.size) | ||
| val fakeCurrentSnapshot = fakePrevSnapshot.drop(fakeRemovedSize) | ||
| val expectedRemoves = fakePrevSnapshot |
| while (stack.isNotEmpty()) { | ||
| val node = stack.pop() | ||
| expectedList.addAll(node!!.wireframes) | ||
| for (i in node.children.count() - 1 downTo 0) { | ||
| stack.push(node.children[i]) | ||
| } | ||
| } |
There was a problem hiding this comment.
Here you're rewriting the prod algorithm, meaning that your test isn't really testing anything…
Another approach for this would be to start from the expected output: a list of nodes, and build a random tree that would give this output.
A pseudo algorithm would be:
fun generateTreeFromList(nodes : List<Node>) : Node {
val maxDepth = 9 // from your current test
val lastSiblings = Array<Node?>(size = maxDepth) { null }
val tree = nodes.popFirst()
while (nodes.isNotEmpty()) {
// get random index of non null node from lastSiblings > 0
val index = magic() // exercise left to reader
val parent = lastSiblings[index - 1]
val node = nodes.popFirst()
parent.addChild(node)
lastSiblings[index] = node
for (i in (index + 1) until maxDepth) {
lastSiblings[i] = null
}
}
return tree
}e6ffd2a to
e8bd782
Compare
What does this PR do?
A brief description of the change being made with this pull request.
Motivation
What inspired you to submit this pull request?
Additional Notes
Anything else we should know when reviewing?
Review checklist (to be filled by reviewers)