Skip to content

RUMM-3041 The RumEventMapper checks ViewEvents by reference#1279

Merged
mariusc83 merged 1 commit into
developfrom
mconstantin/rumm-3041/rumeventmapper-should-check-mapped-object-by-reference
Feb 14, 2023
Merged

RUMM-3041 The RumEventMapper checks ViewEvents by reference#1279
mariusc83 merged 1 commit into
developfrom
mconstantin/rumm-3041/rumeventmapper-should-check-mapped-object-by-reference

Conversation

@mariusc83

@mariusc83 mariusc83 commented Feb 13, 2023

Copy link
Copy Markdown
Member

What does this PR do?

There was a bug in the RumEventMapper because for the ViewEvent we were comparing the original object with the mapped one by value and not by reference as required.

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)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

@mariusc83
mariusc83 requested a review from a team as a code owner February 13, 2023 09:55
@mariusc83 mariusc83 self-assigned this Feb 13, 2023
@mariusc83
mariusc83 force-pushed the mconstantin/rumm-3041/rumeventmapper-should-check-mapped-object-by-reference branch from 34b3014 to 2fecd9c Compare February 13, 2023 09:56
@codecov-commenter

codecov-commenter commented Feb 13, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1279 (2fecd9c) into develop (a9351e9) will increase coverage by 0.07%.
The diff coverage is 100.00%.

❗ Current head 2fecd9c differs from pull request most recent head 9a30cdd. Consider uploading reports for the commit 9a30cdd to get more accurate results

@@             Coverage Diff             @@
##           develop    #1279      +/-   ##
===========================================
+ Coverage    81.85%   81.92%   +0.07%     
===========================================
  Files          361      361              
  Lines        12835    12835              
  Branches      2148     2148              
===========================================
+ Hits         10505    10514       +9     
  Misses        1664     1664              
+ Partials       666      657       -9
Impacted Files Coverage Δ
...ndroid/rum/internal/domain/event/RumEventMapper.kt 95.24% <100.00%> (ø)
...g/internal/domain/event/DdSpanToSpanEventMapper.kt 95.83% <0.00%> (-4.17%) ⬇️
...n/com/datadog/android/v2/core/SdkInternalLogger.kt 88.89% <0.00%> (-2.78%) ⬇️
...d/v2/core/internal/storage/FileEventBatchWriter.kt 94.74% <0.00%> (-2.63%) ⬇️
.../kotlin/com/datadog/android/v2/core/DatadogCore.kt 85.17% <0.00%> (-0.42%) ⬇️
...android/v2/core/internal/DatadogContextProvider.kt 97.56% <0.00%> (ø)
.../android/rum/internal/monitor/DatadogRumMonitor.kt 89.42% <0.00%> (+0.53%) ⬆️
...in/com/datadog/android/log/internal/LogsFeature.kt 84.62% <0.00%> (+0.77%) ⬆️
...ain/java/com/datadog/opentracing/PendingTrace.java 60.34% <0.00%> (+0.86%) ⬆️
...lin/com/datadog/android/rum/internal/RumFeature.kt 93.53% <0.00%> (+1.44%) ⬆️
... and 2 more

0xnm
0xnm requested changes Feb 13, 2023
View reviewed changes
Comment thread ...-android/src/test/kotlin/com/datadog/android/rum/internal/domain/event/RumEventMapperTest.kt Outdated
val mappedRumEvent = testedRumEventMapper.map(fakeRumEvent)

// THEN
assertThat(mappedRumEvent).isEqualTo(fakeRumEvent)

@0xnm 0xnm Feb 13, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to use isSameAs here, because this line will be green even if another instance is returned (isEqualTo will call equals).

Suggested change
assertThat(mappedRumEvent).isEqualTo(fakeRumEvent)
assertThat(mappedRumEvent).isEqualTo(fakeRumEvent)

The log verification below helps though to avoid this test to pass-by.

@mariusc83
mariusc83 force-pushed the mconstantin/rumm-3041/rumeventmapper-should-check-mapped-object-by-reference branch from 2fecd9c to 9a30cdd Compare February 14, 2023 09:21
@mariusc83
mariusc83 merged commit dde54aa into develop Feb 14, 2023
@mariusc83
mariusc83 deleted the mconstantin/rumm-3041/rumeventmapper-should-check-mapped-object-by-reference branch February 14, 2023 10:05
@xgouchet xgouchet added this to the 1.18.0 milestone Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@0xnm 0xnm 0xnm approved these changes

Assignees

@mariusc83 mariusc83

Labels

None yet

Projects

None yet

Milestone

1.18.0

Development

Successfully merging this pull request may close these issues.

4 participants

@mariusc83 @codecov-commenter @0xnm @xgouchet