RUM-5525 Add the integration tests for the SdkCore APIs #2145

mariusc83 · 2024-07-26T12:19:08Z

What does this PR do?

In this PR we are adding the integrations tests to cover the SdkCore public API. In addition to that we are introducing the StubStorageBackedFeature which will be leveraged in order to assert expected behaviours of the core module during the integration tests.

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 · 2024-07-26T12:50:40Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.03%. Comparing base (405b9b4) to head (ed62d5f).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2145      +/-   ##
===========================================
+ Coverage    69.98%   70.03%   +0.05%     
===========================================
  Files          726      726              
  Lines        27001    26996       -5     
  Branches      4525     4524       -1     
===========================================
+ Hits         18895    18904       +9     
+ Misses        6836     6829       -7     
+ Partials      1270     1263       -7

see 25 files with indirect coverage changes

xgouchet · 2024-07-29T08:36:00Z

reliability/core-it/build.gradle.kts

+    buildTypes {
+        getByName("release") {
+            isMinifyEnabled = true
+            proguardFiles(
+                getDefaultProguardFile("proguard-android-optimize.txt"),
+                "proguard-rules.pro"
+            )
+            testProguardFile("test-proguard-rules.pro")
+        }
+        getByName("debug") {
+            isMinifyEnabled = true
+            proguardFiles(
+                getDefaultProguardFile("proguard-android-optimize.txt"),
+                "proguard-rules.pro"
+            )
+            testProguardFile("test-proguard-rules.pro")
+        }


Note
Why do you need a release build, and why minify both release and debug ?

I am trying to test these in the same conditions as a normal app. We did the same for integration tests. Not sure is the best approach but I think it is better.

xgouchet · 2024-07-29T08:40:19Z

reliability/core-it/src/androidTest/kotlin/com/datadog/android/core/integration/SdkCoreTest.kt

+    private var fakeUserId: String? = null
+    private var fakeUserName: String? = null
+    private var fakeUserEmail: String? = null
+    private var fakeUserAdditionalProperties: Map<String, Any?> = emptyMap()
+    private lateinit var stubFeature: StorageBackedFeature
+    private lateinit var fakeFeatureName: String


Note
The @[…]Forgery annotations work in JUnit4 too, so you could use that instead of manual forgeries in the setUp() method.

xgouchet · 2024-07-29T08:41:11Z

reliability/core-it/src/androidTest/kotlin/com/datadog/android/core/integration/SdkCoreTest.kt

+class SdkCoreTest : MockServerTest() {
+
+    @get:Rule
+    var forge = ForgeRule()


Suggested change

var forge = ForgeRule()

var forge = ForgeRule()

.useJvmFactories()

.useToolsFactories()

.withFactory(ConfigurationCoreForgeryFactory()

xgouchet · 2024-07-29T08:53:47Z

reliability/core-it/src/androidTest/kotlin/com/datadog/android/core/integration/SdkCoreTest.kt

+        ConditionWatcher {
+            var readUserInfo: UserInfo? = null
+            featureSdkCore?.getFeature(stubFeature.name)?.withWriteContext { datadogContext, _ ->
+                readUserInfo = datadogContext.userInfo
+            }
+            assertThat(readUserInfo?.id).isEqualTo(fakeUserId)
+            assertThat(readUserInfo?.name).isEqualTo(fakeUserName)
+            assertThat(readUserInfo?.email).isEqualTo(fakeUserEmail)
+            assertThat(readUserInfo?.additionalProperties)
+                .containsExactlyInAnyOrderEntriesOf(fakeUserAdditionalProperties)
+            true
+        }.doWait(timeoutMs = FINAL_WAIT_MS)


Important
Is a 30 seconds wait really necessary here ? We're not writing anything to disk, and this could hide a problem if setting the user info actually gets delayed more than acceptable amount of time.

it is max 30 seconds, same approach we have in the integration tests now. I am not actually sure we are not writing on disk as the UserInfoProvider writes on the disk first. I am going to check if we use also the RAM in addition to that for the current process.

xgouchet · 2024-07-29T08:54:02Z

reliability/core-it/src/androidTest/kotlin/com/datadog/android/core/integration/SdkCoreTest.kt

+            assertThat(readUserInfo?.additionalProperties)
+                .containsExactlyInAnyOrderEntriesOf(fakeUserAdditionalProperties + expectedUserExtraProperties)
+            true
+        }.doWait(timeoutMs = FINAL_WAIT_MS)


xgouchet · 2024-07-29T08:54:12Z

reliability/core-it/src/androidTest/kotlin/com/datadog/android/core/integration/SdkCoreTest.kt

+                assertThat(datadogContext.trackingConsent).isEqualTo(expectedTrackingConsent)
+            }
+            true
+        }.doWait(timeoutMs = FINAL_WAIT_MS)


jonathanmos · 2024-08-01T08:13:45Z

...bility/core-it/src/androidTest/kotlin/com/datadog/android/core/integration/tests/BaseTest.kt

+abstract class BaseTest {
+
+    companion object {
+        internal val LONG_WAIT_MS = TimeUnit.SECONDS.toMillis(30)


I don't see this being used anywhere anymore. Can it be removed?

I might use it later as some use - cases will require it (waiting for messages to be send through mock server). I would rather keep fit for now and decide later.

xgouchet · 2024-08-01T09:30:33Z

reliability/core-it/src/androidTest/kotlin/com/datadog/android/core/integration/SdkCoreTest.kt

+        val configuration: Configuration = forge.getForgery()
+        val sdkCore = Datadog.initialize(
+            ApplicationProvider.getApplicationContext(),
+            configuration,
+            forge.aValueFrom(TrackingConsent::class.java)
+        )
+        val featureSdkCore = sdkCore as? FeatureSdkCore
+        featureSdkCore?.registerFeature(stubFeature)


Note
Shouldn't this be part of the setup?

yup we could add it there

xgouchet · 2024-08-01T09:31:15Z

reliability/core-it/src/androidTest/kotlin/com/datadog/android/core/integration/SdkCoreTest.kt

+        val countDownLatch = CountDownLatch(1)
+        var readUserInfo: UserInfo? = null
+        featureSdkCore?.getFeature(stubFeature.name)?.withWriteContext { datadogContext, _ ->
+            readUserInfo = datadogContext.userInfo
+            countDownLatch.countDown()
+        }
+        countDownLatch.await(SHORT_WAIT_MS, TimeUnit.MILLISECONDS)
+        assertThat(readUserInfo?.id).isEqualTo(fakeUserId)
+        assertThat(readUserInfo?.name).isEqualTo(fakeUserName)
+        assertThat(readUserInfo?.email).isEqualTo(fakeUserEmail)
+        assertThat(readUserInfo?.additionalProperties)
+            .containsExactlyInAnyOrderEntriesOf(fakeUserAdditionalProperties)


👍 Much Better

xgouchet · 2024-08-01T09:32:42Z

reliability/core-it/src/androidTest/kotlin/com/datadog/android/core/integration/SdkCoreTest.kt

+    // region add User Properties
+
+    @Test
+    fun must_addUserExtraProperties_when_addUserProperties() {


Note
You need another test when you don't call setUserInfo first, and probably another ones where you overwrite values too (both overriding a value set from setUserInfo and a value set from addUserProperties.

xgouchet · 2024-08-01T09:33:25Z

reliability/core-it/src/androidTest/kotlin/com/datadog/android/core/integration/SdkCoreTest.kt

+    // region set Tracking Consent
+
+    @Test
+    fun must_updateTrackingConsent_when_setTrackingConsent() {


Note
You need another test to verify that the value you get is the one from initialize when you don't update it.

xgouchet · 2024-08-01T09:36:51Z

reliability/core-it/src/androidTest/kotlin/com/datadog/android/core/integration/SdkCoreTest.kt

+import java.util.concurrent.TimeUnit
+
+@RunWith(AndroidJUnit4::class)
+class SdkCoreTest : MockServerTest() {


Note
To make tests easier, we can split the core test by scopes, and name the test classes accordingly

SdkCoreInformation (not really sure about the name here) for the current one, to test data that's kept in memory (user info, global attributes, tracking consent, …) used by features

SdkCoreIO (maybe with variants based on Tracking consent: …Granted, …Denied, …Pending, …PendingToGranted, …PendingToDenied, …) to test that data sent to the core is dealt with accordingly

… ?

xgouchet · 2024-08-02T14:40:13Z

...ty/core-it/src/androidTest/kotlin/com/datadog/android/core/integration/SdkCoreContextTest.kt

+    private lateinit var fakeTrackingConsent: TrackingConsent
+
+    private var featureSdkCore: FeatureSdkCore? = null
+    private var sdkCore: SdkCore? = null


Note
Nit pick, as per our guidelines it should be named testedSdkCore

will change it in my next PR

0xnm · 2024-08-12T15:39:11Z

...ty/core-it/src/androidTest/kotlin/com/datadog/android/core/integration/SdkCoreContextTest.kt

+
+    private lateinit var fakeTrackingConsent: TrackingConsent
+
+    private var featureSdkCore: FeatureSdkCore? = null


better to use lateinit var with non-nullable type. Will remove a lot of .? in the code.

0xnm · 2024-08-12T15:41:39Z

reliability/core-it/build.gradle.kts

-    keepStartingWith("org.json")
+    androidTestImplementation(project(":reliability:stub-feature"))
+    androidTestImplementation(libs.assertJ)
+    androidTestImplementation(libs.mockitoAndroid)


I cannot see any usages of that. Is it really needed?

mariusc83 self-assigned this Jul 26, 2024

mariusc83 force-pushed the mconstantin/rum-5525/add-integration-tests-for-sdkcore branch from b1cc80f to 89364ed Compare July 26, 2024 12:26

mariusc83 marked this pull request as ready for review July 26, 2024 12:29

mariusc83 requested review from a team as code owners July 26, 2024 12:29

xgouchet requested changes Jul 29, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-5525/add-integration-tests-for-sdkcore branch from 89364ed to f9a4c8e Compare July 29, 2024 12:07

mariusc83 requested a review from xgouchet July 29, 2024 12:07

mariusc83 force-pushed the mconstantin/rum-5525/add-integration-tests-for-sdkcore branch 3 times, most recently from 5eba221 to 07b92b7 Compare July 31, 2024 09:44

jonathanmos approved these changes Aug 1, 2024

View reviewed changes

xgouchet reviewed Aug 1, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-5525/add-integration-tests-for-sdkcore branch 2 times, most recently from def6157 to 85fb24b Compare August 2, 2024 11:56

RUM-5525 Add the integration tests for the SdkCore APIs

ed62d5f

mariusc83 force-pushed the mconstantin/rum-5525/add-integration-tests-for-sdkcore branch from 85fb24b to ed62d5f Compare August 2, 2024 11:57

mariusc83 requested a review from xgouchet August 2, 2024 12:04

xgouchet approved these changes Aug 2, 2024

View reviewed changes

mariusc83 merged commit 7257d4d into develop Aug 2, 2024
22 checks passed

mariusc83 deleted the mconstantin/rum-5525/add-integration-tests-for-sdkcore branch August 2, 2024 14:57

0xnm reviewed Aug 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUM-5525 Add the integration tests for the SdkCore APIs #2145

RUM-5525 Add the integration tests for the SdkCore APIs #2145

mariusc83 commented Jul 26, 2024 •

edited

Loading

codecov-commenter commented Jul 26, 2024 •

edited

Loading

xgouchet Jul 29, 2024

mariusc83 Jul 29, 2024

xgouchet Jul 29, 2024

xgouchet Jul 29, 2024

xgouchet Jul 29, 2024

mariusc83 Jul 29, 2024

xgouchet Jul 29, 2024

xgouchet Jul 29, 2024

jonathanmos Aug 1, 2024

mariusc83 Aug 1, 2024

xgouchet Aug 1, 2024

mariusc83 Aug 1, 2024

xgouchet Aug 1, 2024

xgouchet Aug 1, 2024

xgouchet Aug 1, 2024

xgouchet Aug 1, 2024

xgouchet Aug 2, 2024

mariusc83 Aug 2, 2024

0xnm Aug 12, 2024

0xnm Aug 12, 2024

-    var forge = ForgeRule()
+    var forge = ForgeRule()
+            .useJvmFactories()
+            .useToolsFactories()
+            .withFactory(ConfigurationCoreForgeryFactory()


		private lateinit var fakeTrackingConsent: TrackingConsent

		private var featureSdkCore: FeatureSdkCore? = null

RUM-5525 Add the integration tests for the SdkCore APIs #2145

RUM-5525 Add the integration tests for the SdkCore APIs #2145

Conversation

mariusc83 commented Jul 26, 2024 • edited Loading

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

codecov-commenter commented Jul 26, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariusc83 commented Jul 26, 2024 •

edited

Loading

codecov-commenter commented Jul 26, 2024 •

edited

Loading