RUM-5527 Add integration tests for internal sdk core #2177

mariusc83 · 2024-08-12T09:34:26Z

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)

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-08-12T10:20:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.05%. Comparing base (27f8ab9) to head (2a61ca6).
Report is 4 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2177      +/-   ##
===========================================
+ Coverage    69.96%   70.05%   +0.09%     
===========================================
  Files          726      726              
  Lines        26978    26979       +1     
  Branches      4524     4522       -2     
===========================================
+ Hits         18875    18900      +25     
+ Misses        6831     6818      -13     
+ Partials      1272     1261      -11

see 37 files with indirect coverage changes

0xnm · 2024-08-12T13:54:12Z

...-it/src/androidTest/kotlin/com/datadog/android/core/integration/tests/utils/MutableMapExt.kt

+    val removedEntries = mutableMapOf<Any, Any?>()
+    val keys = this.keys.toList()
+    val numberOfElementsToRemove = forge.anInt(1, keys.size)
+    synchronized(this) {


why synchronization is needed here? most of the tests are using one thread, won't it be better then to use synchronization at the call site where needed? this will align with default Java collections design, where all the implementations are not thread-safe by default.

was thinking just in case someone is using without making sure it is thread safe. I guess it is too much, will remove that lock.

0xnm · 2024-08-12T13:55:27Z

...-it/src/androidTest/kotlin/com/datadog/android/core/integration/tests/utils/MutableMapExt.kt

+
+import fr.xgouchet.elmyr.Forge
+
+internal fun <T : Any, R : Any?> MutableMap<T, R>.removeRandomEntries(forge: Forge): Map<Any, Any?> {


there is type erasure in the return type: Shouldn't it be Map<T, R> instead of Map<Any, Any?>?

...it/src/androidTest/kotlin/com/datadog/android/core/integration/InternalSdkCoreContextTest.kt

0xnm · 2024-08-12T14:00:17Z

...it/src/androidTest/kotlin/com/datadog/android/core/integration/InternalSdkCoreContextTest.kt

+    @StringForgery(regex = "[a-z]+\\.[a-z]+@[a-z]+\\.[a-z]{3}")
+    lateinit var fakeUserEmail: String
+    private var fakeUserAdditionalProperties: Map<String, Any?> = emptyMap()
+    private lateinit var stubFeature: StorageBackedFeature


in theory it can be just Feature, because there is note above that this test is not related to writing data

0xnm · 2024-08-12T14:04:40Z

...it/src/androidTest/kotlin/com/datadog/android/core/integration/InternalSdkCoreContextTest.kt

+    private var featureSdkCore: FeatureSdkCore? = null
+    private var testedInternalSdkCore: InternalSdkCore? = null


why do we need both? by looking on the featureSdkCore = testedInternalSdkCore below, it seems we can just have testedInternalSdkCore: InternalSdkCore? and that is it, isn't it?

also it is better to use lateinit with non-null value type InternalSdkCore, this will remove the need of using ? in many places.

we also need the feature to assert it below where we check the number of returned features.

I'm not sure I got it. InternalSdkCore extends FeatureSdkCore, so having just testedInternalSdkCore: InternalSdkCore should be sufficient to do the necessary assertions.

oh yes, sorry you're totally right, not sure why I added that.

0xnm · 2024-08-12T14:12:21Z

...it/src/androidTest/kotlin/com/datadog/android/core/integration/InternalSdkCoreContextTest.kt

+                }
+            }
+        }.start()
+        testedInternalSdkCore?.setContextUpdateReceiver(fakeFeatureName) { _, _ -> countDownLatch.countDown() }


same: should be set before threads are started

0xnm · 2024-08-12T14:12:35Z

...it/src/androidTest/kotlin/com/datadog/android/core/integration/InternalSdkCoreContextTest.kt

+        // give some time to the firs 2 threads to start
+        Thread.sleep(100)
+        Thread {
+            featureSdkCore?.updateFeatureContext(fakeFeatureName) {
+                droppedKeyValues.forEach { (key, _) ->
+                    it.remove(key)
+                }
+            }
+        }.start()
+        testedInternalSdkCore?.setContextUpdateReceiver(fakeFeatureName) { _, _ -> countDownLatch.countDown() }


same as above

0xnm · 2024-08-12T14:12:58Z

...it/src/androidTest/kotlin/com/datadog/android/core/integration/InternalSdkCoreContextTest.kt

+        // Then
+        assertThat(networkInfo?.connectivity).isEqualTo(expectedConnectivity)


Suggested change

// Then

assertThat(networkInfo?.connectivity).isEqualTo(expectedConnectivity)

// Then

checkNotNull(networkInfo)

assertThat(networkInfo.connectivity).isEqualTo(expectedConnectivity)

0xnm · 2024-08-12T14:14:06Z

...it/src/androidTest/kotlin/com/datadog/android/core/integration/InternalSdkCoreContextTest.kt

+            ApplicationProvider.getApplicationContext(),
+            fakeConfiguration,
+            fakeTrackingConsent
+        ) as? InternalSdkCore


Suggested change

) as? InternalSdkCore

) as InternalSdkCore

We want test to fail here if this cannot be casted. It will be closer to the failure site.

0xnm · 2024-08-12T14:16:12Z

...it/src/androidTest/kotlin/com/datadog/android/core/integration/InternalSdkCoreContextTest.kt

are these test actually different from what we have for the CoreFeature in the unit-tests? I have the feeling that we are repeating the stuff with a little added value here.

The intentions is to test all the public APIs, we do not care if the functionality was already tested somewhere else. Otherwise we will miss things and will be hard to keep track.

0xnm · 2024-08-13T11:36:23Z

...it/src/androidTest/kotlin/com/datadog/android/core/integration/InternalSdkCoreContextTest.kt

+    @StringForgery(type = StringForgeryType.ALPHABETICAL)
+    lateinit var fakeFeatureName: String
+
+    private lateinit var fakeTrackingConsent: TrackingConsent


minor: you can simply add Forgery annotation here and remove lateinit and fakeTrackingConsent = forge.aValueFrom(TrackingConsent::class.java) line.

0xnm · 2024-08-13T11:38:48Z

...it/src/androidTest/kotlin/com/datadog/android/core/integration/InternalSdkCoreContextTest.kt

+
+        // Then
+        assertThat(features).containsAll(expectedRegisteredFeatures)
+        assertThat(features.size).isEqualTo(expectedRegisteredFeaturesSize)


Suggested change

assertThat(features.size).isEqualTo(expectedRegisteredFeaturesSize)

assertThat(features).hasSize(expectedRegisteredFeaturesSize)

mariusc83 self-assigned this Aug 12, 2024

mariusc83 marked this pull request as ready for review August 12, 2024 09:47

mariusc83 requested review from a team as code owners August 12, 2024 09:47

mariusc83 force-pushed the mconstantin/rum-5527/add-integration-tests-for-internal-sdk-core-api branch from 1e4e3b1 to 48ed480 Compare August 12, 2024 13:28

0xnm reviewed Aug 12, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-5527/add-integration-tests-for-internal-sdk-core-api branch from 48ed480 to 549f314 Compare August 13, 2024 08:39

RUM-5527 Add integration tests for internal sdk core

2a61ca6

mariusc83 force-pushed the mconstantin/rum-5527/add-integration-tests-for-internal-sdk-core-api branch from 549f314 to 2a61ca6 Compare August 13, 2024 08:58

mariusc83 requested a review from 0xnm August 13, 2024 11:23

0xnm approved these changes Aug 13, 2024

View reviewed changes

mariusc83 merged commit 6d2cf93 into develop Aug 13, 2024
22 checks passed

mariusc83 deleted the mconstantin/rum-5527/add-integration-tests-for-internal-sdk-core-api branch August 13, 2024 12:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUM-5527 Add integration tests for internal sdk core #2177

RUM-5527 Add integration tests for internal sdk core #2177

mariusc83 commented Aug 12, 2024

codecov-commenter commented Aug 12, 2024 •

edited

Loading

0xnm Aug 12, 2024 •

edited

Loading

mariusc83 Aug 13, 2024

0xnm Aug 12, 2024

0xnm Aug 12, 2024

0xnm Aug 12, 2024

mariusc83 Aug 13, 2024

0xnm Aug 13, 2024

mariusc83 Aug 13, 2024

0xnm Aug 12, 2024

0xnm Aug 12, 2024

0xnm Aug 12, 2024

0xnm Aug 12, 2024

0xnm Aug 12, 2024

mariusc83 Aug 13, 2024

0xnm Aug 13, 2024

0xnm Aug 13, 2024


		import fr.xgouchet.elmyr.Forge

		internal fun <T : Any, R : Any?> MutableMap<T, R>.removeRandomEntries(forge: Forge): Map<Any, Any?> {

		private var featureSdkCore: FeatureSdkCore? = null
		private var testedInternalSdkCore: InternalSdkCore? = null

		// Then
		assertThat(networkInfo?.connectivity).isEqualTo(expectedConnectivity)

-        // Then
-        assertThat(networkInfo?.connectivity).isEqualTo(expectedConnectivity)
+        // Then
+        checkNotNull(networkInfo)
+        assertThat(networkInfo.connectivity).isEqualTo(expectedConnectivity)

	assertThat(features.size).isEqualTo(expectedRegisteredFeaturesSize)
	assertThat(features).hasSize(expectedRegisteredFeaturesSize)

RUM-5527 Add integration tests for internal sdk core #2177

RUM-5527 Add integration tests for internal sdk core #2177

Conversation

mariusc83 commented Aug 12, 2024

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

codecov-commenter commented Aug 12, 2024 • edited Loading

Codecov Report

0xnm Aug 12, 2024 • edited Loading

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

codecov-commenter commented Aug 12, 2024 •

edited

Loading

0xnm Aug 12, 2024 •

edited

Loading