RUMM-1061: Add a warning when RUM action is dropped while another action is still active for the same view #503
Conversation
…ion is still active for the same view
| @@ -158,7 +159,15 @@ internal class RumViewScope( | |||
| ) { | |||
| delegateEventToChildren(event, writer) | |||
|
|
|||
| if (stopped || activeActionScope != null) return | |||
| if (stopped) return | |||
There was a problem hiding this comment.
no logging (even verbose) in this case to be consistent with other onXXX methods in this class (probably it can happen quite often?)
There was a problem hiding this comment.
Yes it can happen too often that the previous view is still present but stopped.
| @@ -158,7 +159,15 @@ internal class RumViewScope( | |||
| ) { | |||
| delegateEventToChildren(event, writer) | |||
|
|
|||
| if (stopped || activeActionScope != null) return | |||
| if (stopped) return | |||
There was a problem hiding this comment.
Yes it can happen too often that the previous view is still present but stopped.
thanks @xgouchet . this is the part I was missing, I was looking for the examples of validating |
0xnm
force-pushed
the
nogorodnikov/rumm-1061/add-warning-when-rum-action-is-dropped-while-another-is-active
branch
from
d2741f5
to
4257658
Compare
| verify(mockDevLogHandler).handleLog( | ||
| eq(Log.WARN), | ||
| argThat { | ||
| contains("was dropped") && |
There was a problem hiding this comment.
I didn't go for the exact match on purpose
There was a problem hiding this comment.
why not asserting the whole string here ?
There was a problem hiding this comment.
because then I have to copy-paste it here (it is parametrized) and keep both in sync, but not a big deal if needed. Just imo we care about the context of the message only.
There was a problem hiding this comment.
if you use an internal constant for that message format in the other class then you can share it with the test. This is what we usually do in this case.
There was a problem hiding this comment.
yes, but the thing is that it is not constant, it is parametrized. I saw that in some places there is dedicated format method to format message with parameters. do you propose to extract it to the format method?
There was a problem hiding this comment.
Exactly ;) this is what I usually do, but it is not dedicated method is the String default method. Basically you can write:
companion object {
internal const val WARNING_ERROR_MESSAGE_FORMAT="RUM Action (%s on %s) was dropped, because another action is still active for the same view"
}
and then use it like:
WARNING_ERROR_MESSAGE.format(eventType,eventName)
| verify(mockDevLogHandler).handleLog( | ||
| eq(Log.WARN), | ||
| argThat { | ||
| contains("was dropped") && |
There was a problem hiding this comment.
why not asserting the whole string here ?
Codecov Report
@@ Coverage Diff @@
## master #503 +/- ##
============================================
+ Coverage 89.12% 89.27% +0.15%
- Complexity 1409 1413 +4
============================================
Files 168 168
Lines 4928 4931 +3
Branches 569 570 +1
============================================
+ Hits 4392 4402 +10
+ Misses 345 339 -6
+ Partials 191 190 -1 |
0xnm
deleted the
nogorodnikov/rumm-1061/add-warning-when-rum-action-is-dropped-while-another-is-active
branch
What does this PR do?
Additional help to investigate #490
Motivation
Issue #490
Additional Notes
N/A
Review checklist (to be filled by reviewers)