RUM-6035 Add the integration tests related with RumMonitor#addViewLoadingTime API#2268
Conversation
| // region AddViewLoading time | ||
|
|
||
| @OptIn(ExperimentalRumApi::class) | ||
| @RepeatedTest(16) |
There was a problem hiding this comment.
Why do we want to repeat it 16 times given that there is only 2 execution branches for the given input - with overwrite and without. For the test above 16 (or repeat the test in general) makes sense, because there is more code paths there for the given input, I guess.
There was a problem hiding this comment.
I guess 1 time is faster and doesn't change anything, just want to check with @xgouchet what was his intention with the previous tests and in case we're good I will change it to one single test.
There was a problem hiding this comment.
RepeatedTest makes sense if there is a big variety in the input parameters which can lead to many different code paths taken. But here the only parameter which can lead (I'm not sure, didn't dig into the code) to the different code paths is the overwrite flag, which is simply binary.
There was a problem hiding this comment.
yes...it make sense, let me change that to a single test.
| } | ||
|
|
||
| @OptIn(ExperimentalRumApi::class) | ||
| @RepeatedTest(16) |
There was a problem hiding this comment.
same question as above (and same for the tests below)
There was a problem hiding this comment.
not sure, I just followed the pattern, @xgouchet do you have more input ?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2268 +/- ##
===========================================
+ Coverage 69.91% 70.13% +0.22%
===========================================
Files 727 727
Lines 27112 27167 +55
Branches 4572 4576 +4
===========================================
+ Hits 18954 19053 +99
+ Misses 6872 6836 -36
+ Partials 1286 1278 -8
|
2d4c7a4 to
52f0ab3
Compare
| hasType("view") | ||
| hasViewUrl(key) | ||
| hasViewName(name) | ||
| hasActionCount(0) |
There was a problem hiding this comment.
should we add assertion here that there is no loading time on this view?
There was a problem hiding this comment.
yup we could do that, good call
| rumMonitor.addViewLoadingTime(overwrite) | ||
|
|
||
| // When | ||
| Thread.sleep(100) |
There was a problem hiding this comment.
to simulate some time between addViewLoadingTime calls
There was a problem hiding this comment.
but there is no throttling mechanism here or something like that, how does it help?
52f0ab3 to
29b64be
Compare
e70c3ea to
23e11c2
Compare
5 participants
…dingTime API
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)