RUM-2778: Print error when no-op core is invoked#3290
Conversation
8037fa8 to
a713287
Compare
This comment has been minimized.
This comment has been minimized.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3290 +/- ##
===========================================
+ Coverage 71.45% 71.48% +0.03%
===========================================
Files 943 943
Lines 34816 34829 +13
Branches 5899 5899
===========================================
+ Hits 24877 24897 +20
+ Misses 8276 8268 -8
- Partials 1663 1664 +1
🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| override fun getFeature(featureName: String): FeatureScope? = null | ||
| override fun getFeature(featureName: String): FeatureScope? { |
There was a problem hiding this comment.
This may get very spammy, given the number of calls to this method. Maybe we should print here only once?
There was a problem hiding this comment.
I think it depends, it's probably enough to print only once per feature, however if the client enables multiple features with no-op core and we print only the error for the first one, then it will be harder for them to discover the issue.
There was a problem hiding this comment.
I mean we can leverage onlyOnce argument of the logger, it will be printed once per each feature.
There was a problem hiding this comment.
oh yes, I was thinking the same argument, but I didn't realize that the message is unique per feature, so it will print once per feature indeed.
Note: another change is made for the convenience of test for onlyOnce, and I don't see why we need to get() a new SdkInternalLogger every time.
// before
override val internalLogger: InternalLogger
get() = SdkInternalLogger(this)
// now
override val internalLogger: InternalLogger = SdkInternalLogger(this)
a713287 to
5a70499
Compare
| whenever(mockFeature.name) doReturn fakeFeatureName | ||
|
|
||
| // When + Then | ||
| Mockito.mockStatic(Log::class.java).use { |
There was a problem hiding this comment.
Do we need to mock if we don't verify log calls anyway? Normally it should be just no-op without mocking.
Probably it is redundant here and in the 2 tests below.
There was a problem hiding this comment.
remove mockStatic since it's not necessary, but keep the tests since they can still protect some corner cases.
5a70499 to
aa96409
Compare
4 participants
What does this PR do?
Print error when no-op core is invoked
Motivation
RUM-2778
Additional Notes
Anything else we should know when reviewing?
Review checklist (to be filled by reviewers)