RUM-988 Implement the SR payloads update local_ci task by mariusc83 · Pull Request #1598 · DataDog/dd-sdk-android

mariusc83 · 2023-09-04T16:01:21Z

What does this PR do?

Session Replay has a suite of functional tests which can be found in the instrumentation:integration module.
Those tests are assessing the recorded payload for a specific scenario against a given payload from assets/session_replay_payloads in the androidTest source set.
In case you need to update these payloads given a change in the SDK we created this new gradle task to ease this process.

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)

Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
Make sure you discussed the feature or bugfix with the maintaining team in an Issue
Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

0xnm

nice appoach!

0xnm · 2023-09-05T07:38:57Z

+
+Session Replay has a suite of functional tests which can be found in the `instrumentation:integration` module.
+Those tests are assessing the recorded payload for a specific scenario against a given payload from `assets/session_replay_payloads` in the `androidTest` source set.
+In case you need to update these payloads given a change in the SDK, you can run the following command:


Suggested change

In case you need to update these payloads given a change in the SDK, you can run the following command:

In case you need to update these payloads after a change in the SDK, you can run the following command:

0xnm · 2023-09-05T07:40:06Z

+        register("srPayloadUpdate") {
+            id = "srPayloadUpdate" // the alias
+            implementationClass = "com.datadog.gradle.plugin.srpayloadupdate.SrPayloadUpdatePlugin"
+        }


minor: maybe for the clarity we shouldn't use sr and use sessionReplay instead in the names (for all classes and tasks). WDYT?

yes, makes sense, was afraid they will be too long :)

0xnm · 2023-09-05T07:41:21Z

+    }
+
+    override fun exec() {
+        println("Executing command: ${commandLine.joinToString(" ")}")


should use Gradle logger here instead of println, this will allow to control the verbosity.

oh yeah...good point

0xnm · 2023-09-05T07:41:52Z

+
+import org.gradle.api.tasks.Exec
+
+open class SrPayloadCreateTask : Exec() {


why this class should be open?

that's the request for a gradle task class

0xnm · 2023-09-05T07:43:48Z

+
+    init {
+        group = "datadog"
+        description = "Update the Session Replay integration tests payloads"


Suggested change

description = "Update the Session Replay integration tests payloads"

description = "Create the Session Replay integration tests payloads"

0xnm · 2023-09-05T07:49:34Z

@@ -0,0 +1 @@
+[{"type":4,"data":{"width":411,"height":914}}, {"type":6,"data":{"has_focus":true}}, {"type":10,"data":{"wireframes":[{"width":411,"height":914,"shapeStyle":{"backgroundColor":"#303030ff","opacity":1.0},"type":"shape"},{"width":395,"height":19,"type":"text","text":"Default Text View","textStyle":{"family":"roboto, sans-serif","size":14,"color":"#a538afff"},"textPosition":{"padding":{"top":0,"bottom":0,"left":0,"right":0},"alignment":{"horizontal":"left","vertical":"top"}}},{"width":395,"height":19,"type":"text","text":"Material Text View","textStyle":{"family":"roboto, sans-serif","size":14,"color":"#cc0000ff"},"textPosition":{"padding":{"top":0,"bottom":0,"left":0,"right":0},"alignment":{"horizontal":"left","vertical":"top"}}},{"width":395,"height":19,"type":"text","text":"App Compat Text View","textStyle":{"family":"roboto, sans-serif","size":14,"color":"#ffbb33ff"},"textPosition":{"padding":{"top":0,"bottom":0,"left":0,"right":0},"alignment":{"horizontal":"left","vertical":"top"}}},{"width":395,"height":44,"type":"text","text":"***","textStyle":{"family":"roboto, sans-serif","size":17,"color":"#ffffffff"},"textPosition":{"padding":{"top":9,"bottom":11,"left":3,"right":3},"alignment":{"horizontal":"left","vertical":"center"}}},{"width":395,"height":1,"shapeStyle":{"backgroundColor":"#a538afff","opacity":1.0},"type":"shape"},{"width":395,"height":44,"type":"text","text":"***","textStyle":{"family":"roboto, sans-serif","size":17,"color":"#ffffffff"},"textPosition":{"padding":{"top":9,"bottom":11,"left":3,"right":3},"alignment":{"horizontal":"left","vertical":"center"}}},{"width":395,"height":1,"shapeStyle":{"backgroundColor":"#ffbb33ff","opacity":1.0},"type":"shape"},{"width":395,"height":44,"type":"text","text":"***","textStyle":{"family":"roboto, sans-serif","size":17,"color":"#ffffffff"},"textPosition":{"padding":{"top":9,"bottom":11,"left":3,"right":3},"alignment":{"horizontal":"left","vertical":"center"}}},{"width":395,"height":1,"shapeStyle":{"backgroundColor":"#ffbb33ff","opacity":1.0},"type":"shape"},{"width":395,"height":44,"type":"text","text":"***","textStyle":{"family":"roboto, sans-serif","size":17,"color":"#ffffffff"},"textPosition":{"padding":{"top":9,"bottom":11,"left":3,"right":3},"alignment":{"horizontal":"left","vertical":"center"}}},{"width":395,"height":1,"shapeStyle":{"backgroundColor":"#a538afff","opacity":1.0},"type":"shape"},{"width":395,"height":44,"type":"text","text":"***","textStyle":{"family":"roboto, sans-serif","size":17,"color":"#ffffffff"},"textPosition":{"padding":{"top":9,"bottom":11,"left":3,"right":3},"alignment":{"horizontal":"left","vertical":"center"}}},{"width":395,"height":1,"shapeStyle":{"backgroundColor":"#a538afff","opacity":1.0},"type":"shape"},{"width":395,"height":32,"type":"text","text":"Checked Text View","textStyle":{"family":"roboto, sans-serif","size":14,"color":"#ffffffff"},"textPosition":{"padding":{"top":0,"bottom":12,"left":0,"right":32},"alignment":{"horizontal":"left","vertical":"center"}}},{"width":19,"height":19,"border":{"color":"#ffffffff","width":1},"type":"shape"},{"width":395,"height":32,"type":"text","text":"App Compat Checked Text View","textStyle":{"family":"roboto, sans-serif","size":14,"color":"#ffffffff"},"textPosition":{"padding":{"top":0,"bottom":12,"left":0,"right":32},"alignment":{"horizontal":"left","vertical":"center"}}},{"width":19,"height":19,"border":{"color":"#ffffffff","width":1},"type":"shape"},{"width":411,"height":56,"type":"placeholder","label":"Toolbar"}]}}]


when comparison fails, do we have a only the difference highlighted in the output or we have a full context as expected vs actual?

it is using Json comparator from assertj , It will show you where it fails.

0xnm · 2023-09-05T07:50:52Z

+        payloadUpdateFile.writeText(payloadUpdateData)
+    }
+
+    private fun wasPayloadUpdateRequest(): Boolean {


Suggested change

private fun wasPayloadUpdateRequest(): Boolean {

private fun isPayloadUpdateRequest(): Boolean {

0xnm · 2023-09-05T07:51:55Z

+        val records = handledRequests
+            .mapNotNull { it.extractSrSegmentAsJson()?.asJsonObject }
+            .flatMap { it.getAsJsonArray("records") }
+            .filter { it.asJsonObject.get("type").asString != "11" }


would be nice to add a comment explaining this line

0xnm · 2023-09-05T07:52:12Z

+            .flatMap { it.getAsJsonArray("records") }
+            .filter { it.asJsonObject.get("type").asString != "11" }
+            .map { it.dropInconsistentProperties() }
+        assertThat(records.size).isGreaterThan(2)


also here, why 2?

because I am taking 3 bellow so want to make sure I have where to take from...I could actually just take 3 and have the exception in case it doesn't have data yet but I rather like it better this way, more readable

0xnm · 2023-09-05T07:52:23Z

+        if (!payloadUpdateFile.exists()) {
+            payloadUpdateFile.createNewFile()
+        }
+        val payloadUpdateData = records.take(3).toString()


and here explaining why 3.

codecov-commenter · 2023-09-05T07:53:38Z

Codecov Report

Merging #1598 (c70ce82) into develop (aa1c2e1) will decrease coverage by 0.08%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #1598      +/-   ##
===========================================
- Coverage    83.67%   83.59%   -0.08%     
===========================================
  Files          451      451              
  Lines        15563    15563              
  Branches      2318     2318              
===========================================
- Hits         13021    13009      -12     
- Misses        1928     1932       +4     
- Partials       614      622       +8

see 18 files with indirect coverage changes

jonathanmos

Nice pr! I've added only one minor suggestion

jonathanmos · 2023-09-05T08:01:10Z

+    protected fun runInstrumentationScenario() {
+        val instrumentation = InstrumentationRegistry.getInstrumentation()
+        instrumentation.waitForIdleSync()
+        // we need this to avoid the Bitrise flakiness and to force and to wait for


Suggested change

// we need this to avoid the Bitrise flakiness and to force and to wait for

// we need this to avoid Bitrise flakiness by requiring waiting for

xgouchet · 2023-09-05T08:00:15Z

+        register("srPayloadUpdate") {
+            id = "srPayloadUpdate" // the alias
+            implementationClass = "com.datadog.gradle.plugin.srpayloadupdate.SrPayloadUpdatePlugin"
+        }


xgouchet · 2023-09-05T08:03:09Z

+    init {
+        group = "datadog"
+        description = "Update the Session Replay integration tests payloads"
+        setCommandLine(


Is the plugin really just a proxy to a command line call? If so, maybe we can add an option in the local_ci.sh file instead and call the adb tool directly.

hmmm not quite, I still need the dependency on the assemble, install gradle tasks and is much more readable this way. Maybe later I will also leverage the up - to - date property of the gradle task, for now I wanted to force it to always execute to see how it behaves but probably I will get back to it later and remove that force flag.

xgouchet · 2023-09-05T08:04:50Z

+import org.gradle.api.tasks.Input
+import org.gradle.api.tasks.OutputDirectory
+
+open class SrPayloadOverrideTask : Exec() {


Same here, this is just a command line proxy, maybe not necessary to create a plugin

same answer as above, also I like that I can add the input path relative to the project. What would be the drawback for using the plugin ?

Basically it's the overhead of using the command line to start gradle to launch a command line.
Right now everything you do in those task is using constants, which can still be done in the local_ci.sh file. You can still ensure that the assemble / install tasks are ran before calling adb.

I like that I can add the input path relative to the project
The input path is hardcoded, did you mean that you want to set it to use the module's package name instead? Is this comming in this PR or another one?

for now I wanted to force it to always execute
If the adb command is ran from local_ci, it would always execute as there would not be any cache for it.

let me think about it and give it a try in shell

0xnm · 2023-09-05T11:26:06Z

    TEST=1
    shift
    ;;
+  -u | --updateSessionReplayPayload)


I wouldn't use -u for that and keep only long version of argument. -u is not quite self-descriptive and this task anyway is specific for the single module.

yeah makes more sense

0xnm · 2023-09-05T15:03:28Z

    companion object {
        internal val INITIAL_WAIT_MS = TimeUnit.SECONDS.toMillis(60)
+        private const val UI_THREAD_DELAY_IN_MS = 1000L
+        private const val PAYLOAD_UPDATE_REQUEST = "updateSrPayloads"


can also name it updateSessionReplayPayloads. IMO having SessionReplay not shortened will simplify text search, because searching only for sr may give too much noise.

mariusc83 self-assigned this Sep 4, 2023

mariusc83 force-pushed the mconstantin/rum-988/provide-gradle-task-for-sr-payloads-update branch 2 times, most recently from 55213a6 to 7fe45a8 Compare September 5, 2023 07:07

mariusc83 marked this pull request as ready for review September 5, 2023 07:08

mariusc83 requested a review from a team as a code owner September 5, 2023 07:08

0xnm reviewed Sep 5, 2023

View reviewed changes

jonathanmos previously approved these changes Sep 5, 2023

View reviewed changes

xgouchet reviewed Sep 5, 2023

View reviewed changes

mariusc83 dismissed jonathanmos’s stale review via 9dbdf79 September 5, 2023 09:05

mariusc83 force-pushed the mconstantin/rum-988/provide-gradle-task-for-sr-payloads-update branch 2 times, most recently from 9dbdf79 to 372e8c0 Compare September 5, 2023 09:13

mariusc83 requested review from 0xnm, jonathanmos and xgouchet September 5, 2023 09:13

mariusc83 force-pushed the mconstantin/rum-988/provide-gradle-task-for-sr-payloads-update branch from 372e8c0 to dfb4d49 Compare September 5, 2023 09:40

0xnm reviewed Sep 5, 2023

View reviewed changes

mariusc83 requested a review from 0xnm September 5, 2023 11:43

mariusc83 force-pushed the mconstantin/rum-988/provide-gradle-task-for-sr-payloads-update branch 3 times, most recently from 1d28667 to ffe2e3d Compare September 5, 2023 11:58

RUM-988 Implement the SR payloads update gradle task

c70ce82

mariusc83 force-pushed the mconstantin/rum-988/provide-gradle-task-for-sr-payloads-update branch from ffe2e3d to c70ce82 Compare September 5, 2023 13:50

xgouchet approved these changes Sep 5, 2023

View reviewed changes

0xnm approved these changes Sep 5, 2023

View reviewed changes

mariusc83 changed the title ~~RUM-988 Implement the SR payloads update gradle task~~ RUM-988 Implement the SR payloads update local_ci task Sep 5, 2023

mariusc83 merged commit 8a78bee into develop Sep 5, 2023

mariusc83 deleted the mconstantin/rum-988/provide-gradle-task-for-sr-payloads-update branch September 5, 2023 15:14

xgouchet added this to the 2.2.0 milestone Dec 13, 2023

	In case you need to update these payloads given a change in the SDK, you can run the following command:
	In case you need to update these payloads after a change in the SDK, you can run the following command:


		import org.gradle.api.tasks.Exec

		open class SrPayloadCreateTask : Exec() {

	description = "Update the Session Replay integration tests payloads"
	description = "Create the Session Replay integration tests payloads"

		@@ -0,0 +1 @@
		[{"type":4,"data":{"width":411,"height":914}}, {"type":6,"data":{"has_focus":true}}, {"type":10,"data":{"wireframes":[{"width":411,"height":914,"shapeStyle":{"backgroundColor":"#303030ff","opacity":1.0},"type":"shape"},{"width":395,"height":19,"type":"text","text":"Default Text View","textStyle":{"family":"roboto, sans-serif","size":14,"color":"#a538afff"},"textPosition":{"padding":{"top":0,"bottom":0,"left":0,"right":0},"alignment":{"horizontal":"left","vertical":"top"}}},{"width":395,"height":19,"type":"text","text":"Material Text View","textStyle":{"family":"roboto, sans-serif","size":14,"color":"#cc0000ff"},"textPosition":{"padding":{"top":0,"bottom":0,"left":0,"right":0},"alignment":{"horizontal":"left","vertical":"top"}}},{"width":395,"height":19,"type":"text","text":"App Compat Text View","textStyle":{"family":"roboto, sans-serif","size":14,"color":"#ffbb33ff"},"textPosition":{"padding":{"top":0,"bottom":0,"left":0,"right":0},"alignment":{"horizontal":"left","vertical":"top"}}},{"width":395,"height":44,"type":"text","text":"*","textStyle":{"family":"roboto, sans-serif","size":17,"color":"#ffffffff"},"textPosition":{"padding":{"top":9,"bottom":11,"left":3,"right":3},"alignment":{"horizontal":"left","vertical":"center"}}},{"width":395,"height":1,"shapeStyle":{"backgroundColor":"#a538afff","opacity":1.0},"type":"shape"},{"width":395,"height":44,"type":"text","text":"","textStyle":{"family":"roboto, sans-serif","size":17,"color":"#ffffffff"},"textPosition":{"padding":{"top":9,"bottom":11,"left":3,"right":3},"alignment":{"horizontal":"left","vertical":"center"}}},{"width":395,"height":1,"shapeStyle":{"backgroundColor":"#ffbb33ff","opacity":1.0},"type":"shape"},{"width":395,"height":44,"type":"text","text":"","textStyle":{"family":"roboto, sans-serif","size":17,"color":"#ffffffff"},"textPosition":{"padding":{"top":9,"bottom":11,"left":3,"right":3},"alignment":{"horizontal":"left","vertical":"center"}}},{"width":395,"height":1,"shapeStyle":{"backgroundColor":"#ffbb33ff","opacity":1.0},"type":"shape"},{"width":395,"height":44,"type":"text","text":"","textStyle":{"family":"roboto, sans-serif","size":17,"color":"#ffffffff"},"textPosition":{"padding":{"top":9,"bottom":11,"left":3,"right":3},"alignment":{"horizontal":"left","vertical":"center"}}},{"width":395,"height":1,"shapeStyle":{"backgroundColor":"#a538afff","opacity":1.0},"type":"shape"},{"width":395,"height":44,"type":"text","text":"**","textStyle":{"family":"roboto, sans-serif","size":17,"color":"#ffffffff"},"textPosition":{"padding":{"top":9,"bottom":11,"left":3,"right":3},"alignment":{"horizontal":"left","vertical":"center"}}},{"width":395,"height":1,"shapeStyle":{"backgroundColor":"#a538afff","opacity":1.0},"type":"shape"},{"width":395,"height":32,"type":"text","text":"Checked Text View","textStyle":{"family":"roboto, sans-serif","size":14,"color":"#ffffffff"},"textPosition":{"padding":{"top":0,"bottom":12,"left":0,"right":32},"alignment":{"horizontal":"left","vertical":"center"}}},{"width":19,"height":19,"border":{"color":"#ffffffff","width":1},"type":"shape"},{"width":395,"height":32,"type":"text","text":"App Compat Checked Text View","textStyle":{"family":"roboto, sans-serif","size":14,"color":"#ffffffff"},"textPosition":{"padding":{"top":0,"bottom":12,"left":0,"right":32},"alignment":{"horizontal":"left","vertical":"center"}}},{"width":19,"height":19,"border":{"color":"#ffffffff","width":1},"type":"shape"},{"width":411,"height":56,"type":"placeholder","label":"Toolbar"}]}}] No newline at end of file

	private fun wasPayloadUpdateRequest(): Boolean {
	private fun isPayloadUpdateRequest(): Boolean {

	// we need this to avoid the Bitrise flakiness and to force and to wait for
	// we need this to avoid Bitrise flakiness by requiring waiting for

Uh oh!

Conversation

mariusc83 commented Sep 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

Uh oh!

0xnm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jonathanmos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

mariusc83 commented Sep 4, 2023 •

edited

Loading

codecov-commenter commented Sep 5, 2023 •

edited

Loading