-
Notifications
You must be signed in to change notification settings - Fork 63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RUMM-2885 Add addFeatureFlagEvaluation
function for RUM
#1265
Conversation
`addFeatureFlagEvalutaion` adds the result of evaluating a feature flag to a view, which is key / value pair. These feature flags are sent with any subsequent View updates, as well as on Errors until the view is stopped.
Codecov Report
@@ Coverage Diff @@
## develop #1265 +/- ##
===========================================
- Coverage 82.01% 81.90% -0.11%
===========================================
Files 361 361
Lines 12832 12847 +15
Branches 2149 2151 +2
===========================================
- Hits 10524 10522 -2
- Misses 1655 1667 +12
- Partials 653 658 +5
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! I've added some minor comments to the tests
@StringForgery flagName: String, | ||
@StringForgery flagValue: String | ||
) { | ||
// GIVEN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty GIVEN
. Should it be like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the givens here are set up by the fixture setup. I'm not sure if I should keep the comment and have a blank body or remove the comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just remove it I think. They are all added manually, we don't have a template.
argumentCaptor<ViewEvent> { | ||
verify(mockWriter).write(eq(mockEventBatchWriter), capture()) | ||
assertThat(lastValue) | ||
.apply { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apply
is not needed here if I'm not wrong, direct call is possible. Same for all tests below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Yeah, I guess I only need apply if I were checking multiple things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is not needed even for multiple assertions, because each assertion returns Assert
instance, it can be called in the chain.
} | ||
|
||
// endregion | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps missing a test on not sending pre-existing Feature Flag after a new view is started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'll probably need to see if I can add that to DatadogRumMonitorTest
since that's what would handle changing the scope.
88eb385
to
25fa963
Compare
@0xnm Can I get a review of the new test before I merge in? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good!
What does this PR do?
addFeatureFlagEvalutaion
adds the result of evaluating a feature flag to a view, which is key / value pair. These feature flags are sent with any subsequent View updates, as well as on Errors until the view is stopped.Review checklist (to be filled by reviewers)