RUM-5878 Add the integration tests for FeatureScope public API#2209
Conversation
4149d49 to
7f426c2
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2209 +/- ##
===========================================
- Coverage 70.10% 70.04% -0.05%
===========================================
Files 726 726
Lines 27009 27001 -8
Branches 4528 4527 -1
===========================================
- Hits 18932 18912 -20
+ Misses 6823 6822 -1
- Partials 1254 1267 +13 |
d12d24b to
fecd166
Compare
| APPROXIMATE_APP_START_TIME_NS, | ||
| Offset.offset(APP_START_TIME_OFFSET_NS) | ||
| ) | ||
| assertThat(appStartTimeNs).isPositive() |
There was a problem hiding this comment.
I think it is still better to keep this assertion stricter. Like for sure, it should be positive, but also not exceed some reasonable value. Maybe like 5-10 seconds? WDYT?
There was a problem hiding this comment.
it was very flaky but yes let's try something else.
| val eventMetadata = forge.anAlphabeticalString() | ||
| RawBatchEvent( | ||
| fakeEvent.toString().toByteArray(), | ||
| eventMetadata.toByteArray() | ||
| ) | ||
| } | ||
| fakeBatchMetadata = forge.anAlphabeticalString().toByteArray() |
There was a problem hiding this comment.
should we generate nullable type for event/batch metadata to cover more?
There was a problem hiding this comment.
yeah let's do this to cover also this case.
There was a problem hiding this comment.
yes but I specifically added a test below where I test for null value...just to make sure we are testing for both possibilities.
|
|
||
| @After | ||
| fun tearDown() { | ||
| cleanupStorage() |
There was a problem hiding this comment.
| cleanupStorage() | |
| cleanStorage() |
for the naming consistency with cleanMockWebServer
| import fr.xgouchet.elmyr.Forge | ||
| import fr.xgouchet.elmyr.ForgeryFactory | ||
|
|
||
| class RawBatchEventForgeryFactory : ForgeryFactory<RawBatchEvent> { |
There was a problem hiding this comment.
this should be removed
fecd166 to
809d3f0
Compare
| val eventMetadata = forge.anAlphabeticalString() | ||
| RawBatchEvent( | ||
| fakeEvent.toString().toByteArray(), | ||
| eventMetadata.toByteArray() | ||
| ) | ||
| } | ||
| fakeBatchMetadata = forge.anAlphabeticalString().toByteArray() |
| APPROXIMATE_APP_START_TIME_NS, | ||
| Offset.offset(APP_START_TIME_OFFSET_NS) | ||
| ) | ||
| assertThat(appStartTimeNs).isBetween(0, APPLICATION_START_TIME_UPPER_BOUND_NS) |
There was a problem hiding this comment.
shouldn't we limit lower bound? like by using (APPLICATION_START_TIME_UPPER_BOUND_NS - <few seconds>).coerceAtLeast(0)?
There was a problem hiding this comment.
I am afraid I will do it too flaky (we already have 2 different ways of computing this in the logic based on the API version) ...the intention of this test is not to assert the logic of computing the AppStartTime which mainly should happen in the unit test. I think here we should make sure that this value is relevant enough. What do you think ?
4 participants
What does this PR do?
In this PR we are adding the integration tests for the
FeatureScopepublic API. Basically we are assessing the capability of the SDK core to write and propagate an event through the pipeline up to the NetworkLayer by using a MockWebServer to intercept the payloads.Please note as this was specified also in the
TestClassthesendEventAPI was not tested as this functionality was already covered in theFeatureSdkCoreTestMotivation
What inspired you to submit this pull request?
Additional Notes
Anything else we should know when reviewing?
Review checklist (to be filled by reviewers)