Skip to content
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.

Sign up for GitHub

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

Jump to bottom

RUM-6035 Add the integration tests related with RumMonitor#addViewLoadingTime API #2268

Merged
merged 2 commits into from
Sep 19, 2024
Merged

RUM-6035 Add the integration tests related with RumMonitor#addViewLoadingTime API #2268

merged 2 commits into from
Sep 19, 2024

Conversation

mariusc83
Copy link
Member

…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)

  • 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 self-assigned this Sep 18, 2024
@mariusc83 mariusc83 marked this pull request as ready for review September 18, 2024 13:46
@mariusc83 mariusc83 requested review from a team as code owners September 18, 2024 13:46
0xnm
0xnm reviewed Sep 18, 2024
View reviewed changes
.../single-fit/rum/src/test/kotlin/com/datadog/android/rum/integration/ManualTrackingRumTest.kt Outdated
// region AddViewLoading time

@OptIn(ExperimentalRumApi::class)
@RepeatedTest(16)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes...it make sense, let me change that to a single test.

.../single-fit/rum/src/test/kotlin/com/datadog/android/rum/integration/ManualTrackingRumTest.kt Outdated
}

@OptIn(ExperimentalRumApi::class)
@RepeatedTest(16)
Copy link
Member

Choose a reason for hiding this comment

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

same question as above (and same for the tests below)

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure, I just followed the pattern, @xgouchet do you have more input ?

@codecov-commenter
Copy link

codecov-commenter commented Sep 18, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 96.55172% with 2 lines in your changes missing coverage. Please review.

Project coverage is 70.13%. Comparing base (531effd) to head (29b64be).
Report is 13 commits behind head on develop.

Files with missing lines Patch % Lines
...n/kotlin/com/datadog/android/api/InternalLogger.kt 0.00% 1 Missing ⚠️
.../android/rum/internal/domain/scope/RumViewScope.kt 97.67% 0 Missing and 1 partial ⚠️
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
Files with missing lines Coverage Δ
...lin/com/datadog/android/rum/internal/RumFeature.kt 93.86% <ø> (+2.05%) ⬆️
...id/rum/internal/domain/event/RumEventSerializer.kt 100.00% <100.00%> (ø)
...d/rum/internal/domain/scope/RumViewManagerScope.kt 91.30% <100.00%> (+1.30%) ⬆️
...ndroid/telemetry/internal/TelemetryEventHandler.kt 86.87% <ø> (+12.45%) ⬆️
...n/kotlin/com/datadog/android/api/InternalLogger.kt 93.75% <0.00%> (-6.25%) ⬇️
.../android/rum/internal/domain/scope/RumViewScope.kt 94.64% <97.67%> (+0.29%) ⬆️

... and 34 files with indirect coverage changes

@mariusc83 mariusc83 force-pushed the mconstantin/rum-6035/add-integrationtest-for-addviewloadingtime-api branch from 2d4c7a4 to 52f0ab3 Compare September 18, 2024 15:36
@mariusc83 mariusc83 requested a review from 0xnm September 18, 2024 15:42
0xnm
0xnm approved these changes Sep 19, 2024
View reviewed changes
.../single-fit/rum/src/test/kotlin/com/datadog/android/rum/integration/ManualTrackingRumTest.kt
hasType("view")
hasViewUrl(key)
hasViewName(name)
hasActionCount(0)
Copy link
Member

Choose a reason for hiding this comment

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

should we add assertion here that there is no loading time on this view?

Copy link
Member Author

Choose a reason for hiding this comment

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

yup we could do that, good call

.../single-fit/rum/src/test/kotlin/com/datadog/android/rum/integration/ManualTrackingRumTest.kt
rumMonitor.addViewLoadingTime(overwrite)

// When
Thread.sleep(100)
Copy link
Member

Choose a reason for hiding this comment

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

why we need that?

Copy link
Member Author

Choose a reason for hiding this comment

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

to simulate some time between addViewLoadingTime calls

Copy link
Member

Choose a reason for hiding this comment

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

but there is no throttling mechanism here or something like that, how does it help?

@mariusc83 mariusc83 force-pushed the mconstantin/rum-6035/add-integrationtest-for-addviewloadingtime-api branch from 52f0ab3 to 29b64be Compare September 19, 2024 07:43
@mariusc83 mariusc83 force-pushed the mconstantin/rum-6033/add-viewloadingtime-logs branch from e70c3ea to 23e11c2 Compare September 19, 2024 12:57
Base automatically changed from mconstantin/rum-6033/add-viewloadingtime-logs to develop September 19, 2024 14:44
@mariusc83 mariusc83 merged commit 529e791 into develop Sep 19, 2024
22 checks passed
@mariusc83 mariusc83 deleted the mconstantin/rum-6035/add-integrationtest-for-addviewloadingtime-api branch September 19, 2024 14:47
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

@jonathanmos jonathanmos jonathanmos approved these changes

@xgouchet xgouchet xgouchet approved these changes

Assignees

@mariusc83 mariusc83

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mariusc83 @codecov-commenter @xgouchet @0xnm @jonathanmos