RUM-1005 Add the SR functional tests for sensitive input fields by mariusc83 · Pull Request #1601 · DataDog/dd-sdk-android

mariusc83 · 2023-09-06T11:12:27Z

What does this PR do?

Adding the functional tests for SR sensitive input fields use cases.
Apart from the functional tests we also applied several fixes:

we made sure the SDK is always stopped before asserting a new test case to not alter the payload.
we are removing the base64 content from wireframes assertion as it is not consistent.

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)

codecov-commenter · 2023-09-06T11:12:35Z

Codecov Report

Merging #1601 (6d14b04) into develop (8a78bee) will decrease coverage by 0.00%.
Report is 8 commits behind head on develop.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #1601      +/-   ##
===========================================
- Coverage    83.67%   83.66%   -0.00%     
===========================================
  Files          451      451              
  Lines        15563    15567       +4     
  Branches      2318     2318              
===========================================
+ Hits         13021    13024       +3     
- Misses        1928     1931       +3     
+ Partials       614      612       -2

Files Changed	Coverage Δ
.../com/datadog/android/sessionreplay/NoOpRecorder.kt	`100.00% <ø> (ø)`
...dog/android/sessionreplay/SessionReplayRecorder.kt	`94.74% <100.00%> (+0.07%)`	⬆️
...oid/sessionreplay/internal/SessionReplayFeature.kt	`100.00% <100.00%> (ø)`
...nreplay/internal/async/RecordedDataQueueHandler.kt	`97.59% <100.00%> (+0.06%)`	⬆️

... and 17 files with indirect coverage changes

📢 Have feedback on the report? Share it here.

mariusc83 · 2023-09-06T11:43:07Z

        STORAGE_CONFIGURATION

    override fun onStop() {
+        Log.v("SessionReplayFeature", "====================onStop")


damn, sorry for that

0xnm · 2023-09-06T12:45:24Z

            .getInstrumentation()
            .targetContext
-            .cacheDir.deleteRecursively {
-                Log.i("MockServerActivityTestRule", "After activity finished, deleting file $it")


This log entry is not useful?

well not so much....it was before to make sure it is deleting the files but right now we are not checking it anymore

0xnm · 2023-09-06T12:58:53Z

+        GlobalRumMonitor.get().stopSession()
+        Datadog.stopInstance()
+        GlobalRumMonitor::class.java.getDeclaredMethod("reset").apply {
+            isAccessible = true
+            invoke(null)
+        }


we already doing this in the MockServerActivityTestRule, so aren't we doing this twice then?

0xnm · 2023-09-06T12:59:42Z

        val sessionReplaySampleRate = intent.getSrSampleRate()
        Datadog.setVerbosity(Log.VERBOSE)
+        // make sure the previous instance is stopped
+        Datadog.stopInstance()


is there any bug without this line?

not quite but there was some flakiness on my end and wanted to actually make sure that everything is stopped before my test starts to avoid any issues

0xnm · 2023-09-06T13:01:14Z

+    <string name="default_password_label">Default password</string>
+    <string name="a_password">apassword</string>
+    <string name="a_number_password">1213213</string>
+    <string name="an_email">[email protected]</string>


it can be a real user email, so to avoid any issues maybe we should use some email which for sure doesn't exist?

how would you know it doesn't exist ? In any case the value doesn't really matter here as I am using the inputType of the field in my code.

We should use either email from Datadog domain (some no-reply address for example), or something from https://example.com/ domain.

0xnm · 2023-09-06T14:04:48Z

+    fun tearDown() {
+        GlobalRumMonitor.get().stopSession()
+        Datadog.stopInstance()
+        GlobalRumMonitor::class.java.getDeclaredMethod("reset").apply {


why it is needed? If instance is stopped, then nothing will be written anyway.

I can see the functional gap here however, because probably if we register instance with the same name as before, monitor will be still the old one, since we don't clean global RUM monitor when we stop SDK instance, I will create a ticket for it.

yeah...for now I want to keep this here to make sure everything is cleaned but yes I will open a ticket for later.

0xnm · 2023-09-08T07:20:01Z


 @NoOpImplementation
 internal interface Recorder {
-    @MainThread


why do we remove them?

because the way I implemented them in the recorder allow them to be called on any thread. In the recorder they are being passed through an uiThread handler

0xnm · 2023-09-08T07:22:50Z

+    @Synchronized
+    internal fun clearAndStopProcessingQueue() {
+        recordedDataQueue.clear()
+        executorService.shutdown()


should it be shutdownNow? Not sure if will make the difference though in this particular case, but shutdown will wait until already started tasks are completed.

yes....that's what I want, I want to avoid stopping anything in progress as it can create more issues.

mariusc83 self-assigned this Sep 6, 2023

mariusc83 requested a review from a team as a code owner September 6, 2023 11:12

mariusc83 commented Sep 6, 2023

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-1005/sr-functional-tests-for-sensible-fields branch 2 times, most recently from 7e10cc3 to 3c567e1 Compare September 6, 2023 12:29

0xnm reviewed Sep 6, 2023

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-1005/sr-functional-tests-for-sensible-fields branch from 3c567e1 to d628bf1 Compare September 6, 2023 16:16

mariusc83 commented Sep 7, 2023

View reviewed changes

Comment thread ...src/main/kotlin/com/datadog/android/sessionreplay/internal/async/RecordedDataQueueHandler.kt

mariusc83 force-pushed the mconstantin/rum-1005/sr-functional-tests-for-sensible-fields branch 5 times, most recently from 4e3b8e6 to 82dd7a4 Compare September 7, 2023 13:10

mariusc83 requested a review from 0xnm September 7, 2023 13:29

mariusc83 force-pushed the mconstantin/rum-1005/sr-functional-tests-for-sensible-fields branch from 82dd7a4 to 38139e8 Compare September 7, 2023 13:31

mariusc83 added 2 commits September 7, 2023 17:37

RUM-1005 Add the SR functional tests for sensitive input fields

50b6c4e

Make sure we clean the SR pending queue when the feature is stopped

6d14b04

mariusc83 force-pushed the mconstantin/rum-1005/sr-functional-tests-for-sensible-fields branch from 38139e8 to 6d14b04 Compare September 7, 2023 15:37

0xnm approved these changes Sep 8, 2023

View reviewed changes

mariusc83 merged commit ea817f1 into develop Sep 8, 2023

mariusc83 deleted the mconstantin/rum-1005/sr-functional-tests-for-sensible-fields branch September 8, 2023 08:17

xgouchet added this to the 2.2.0 milestone Dec 13, 2023

Uh oh!

Conversation

mariusc83 commented Sep 6, 2023

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

Uh oh!

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

Uh oh!

Codecov Report

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!

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!

4 participants

codecov-commenter commented Sep 6, 2023 •

edited

Loading