RUM-5527 Add integration tests for internal sdk core#2177
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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 |
1e4e3b1 to
48ed480
Compare
| val removedEntries = mutableMapOf<Any, Any?>() | ||
| val keys = this.keys.toList() | ||
| val numberOfElementsToRemove = forge.anInt(1, keys.size) | ||
| synchronized(this) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| import fr.xgouchet.elmyr.Forge | ||
|
|
||
| internal fun <T : Any, R : Any?> MutableMap<T, R>.removeRandomEntries(forge: Forge): Map<Any, Any?> { |
There was a problem hiding this comment.
there is type erasure in the return type: Shouldn't it be Map<T, R> instead of Map<Any, Any?>?
| @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 |
There was a problem hiding this comment.
in theory it can be just Feature, because there is note above that this test is not related to writing data
| private var featureSdkCore: FeatureSdkCore? = null | ||
| private var testedInternalSdkCore: InternalSdkCore? = null |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
we also need the feature to assert it below where we check the number of returned features.
There was a problem hiding this comment.
I'm not sure I got it. InternalSdkCore extends FeatureSdkCore, so having just testedInternalSdkCore: InternalSdkCore should be sufficient to do the necessary assertions.
There was a problem hiding this comment.
oh yes, sorry you're totally right, not sure why I added that.
| } | ||
| } | ||
| }.start() | ||
| testedInternalSdkCore?.setContextUpdateReceiver(fakeFeatureName) { _, _ -> countDownLatch.countDown() } |
There was a problem hiding this comment.
same: should be set before threads are started
| // 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() } |
| // Then | ||
| assertThat(networkInfo?.connectivity).isEqualTo(expectedConnectivity) |
There was a problem hiding this comment.
| // Then | |
| assertThat(networkInfo?.connectivity).isEqualTo(expectedConnectivity) | |
| // Then | |
| checkNotNull(networkInfo) | |
| assertThat(networkInfo.connectivity).isEqualTo(expectedConnectivity) |
| ApplicationProvider.getApplicationContext(), | ||
| fakeConfiguration, | ||
| fakeTrackingConsent | ||
| ) as? InternalSdkCore |
There was a problem hiding this comment.
| ) as? InternalSdkCore | |
| ) as InternalSdkCore |
We want test to fail here if this cannot be casted. It will be closer to the failure site.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
48ed480 to
549f314
Compare
549f314 to
2a61ca6
Compare
| @StringForgery(type = StringForgeryType.ALPHABETICAL) | ||
| lateinit var fakeFeatureName: String | ||
|
|
||
| private lateinit var fakeTrackingConsent: TrackingConsent |
There was a problem hiding this comment.
minor: you can simply add Forgery annotation here and remove lateinit and fakeTrackingConsent = forge.aValueFrom(TrackingConsent::class.java) line.
|
|
||
| // Then | ||
| assertThat(features).containsAll(expectedRegisteredFeatures) | ||
| assertThat(features.size).isEqualTo(expectedRegisteredFeaturesSize) |
There was a problem hiding this comment.
| assertThat(features.size).isEqualTo(expectedRegisteredFeaturesSize) | |
| assertThat(features).hasSize(expectedRegisteredFeaturesSize) |
3 participants
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)