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.

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-2183 Nightly tests for ConfigurationBuilder#setVitalsUpdateFrequency #929

Conversation

mariusc83
Copy link
Member

What does this PR do?

Adding the nightly tests for newly introduce API : ConfigurationBuilder#setVitalsUpdateFrequency
Increasing the nightly tests coverage to 0.94

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 May 10, 2022
@mariusc83 mariusc83 requested a review from a team as a code owner May 10, 2022 08:59
@mariusc83 mariusc83 force-pushed the mconstantin/rumm-2076/mobile-vitals-uptate-frequency-option branch from d0feae5 to b63545e Compare May 10, 2022 09:00
@xgouchet xgouchet added the size-small This PR is small sized label May 10, 2022
// Give some time to vitals monitors to be updated
Thread.sleep(5000)
scenario.onActivity {
InstrumentationRegistry.getInstrumentation().callActivityOnPause(it)
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 need to call pause and then resume?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just because I want to make sure I get some choreographer updates.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking that Choreographer always does postFrameCallback, even if nothing happens on the screen (so it is sufficient only to launch an activity), but maybe my knowledge is wrong.

initializeSdk(
InstrumentationRegistry.getInstrumentation().targetContext,
TrackingConsent.GRANTED,
Configuration
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest either to extract configuration into a dedicated variable to reduce the indent level, or extract custom viewStrategy into a dedicated class for the same purpose.

@codecov-commenter
Copy link

codecov-commenter commented May 11, 2022

Codecov Report

Merging #929 (679fc51) into mconstantin/rumm-2076/mobile-vitals-uptate-frequency-option (b63545e) will increase coverage by 0.14%.
The diff coverage is n/a.

@@                                       Coverage Diff                                       @@
##           mconstantin/rumm-2076/mobile-vitals-uptate-frequency-option     #929      +/-   ##
===============================================================================================
+ Coverage                                                        82.64%   82.79%   +0.14%     
===============================================================================================
  Files                                                              265      265              
  Lines                                                             9006     9006              
  Branches                                                          1449     1449              
===============================================================================================
+ Hits                                                              7443     7456      +13     
+ Misses                                                            1167     1156      -11     
+ Partials                                                           396      394       -2     
Impacted Files Coverage Δ
...android/rum/internal/ndk/DatadogNdkCrashHandler.kt 87.93% <0.00%> (+4.60%) ⬆️
...ndroid/core/internal/persistence/file/EventMeta.kt 90.00% <0.00%> (+20.00%) ⬆️
...android/log/internal/logger/TelemetryLogHandler.kt 100.00% <0.00%> (+25.00%) ⬆️

@mariusc83 mariusc83 force-pushed the mconstantin/rumm-2183/vitals-update-frequency-api-nightly-tests branch 2 times, most recently from 00f3f1c to 6c6a438 Compare May 11, 2022 07:46
// Give some time to vitals monitors to be updated
Thread.sleep(5000)
scenario.onActivity {
InstrumentationRegistry.getInstrumentation().callActivityOnPause(it)
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking that Choreographer always does postFrameCallback, even if nothing happens on the screen (so it is sufficient only to launch an activity), but maybe my knowledge is wrong.

exclude = listOf(VitalsUpdateFrequency.NEVER)
)
val testMethodName = "rum_config_set_vitals_update_frequency"
// we need to initialize the SDK on the Main Thread (Looper thread) otherwise the
Copy link
Member

Choose a reason for hiding this comment

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

very minor: this comment belongs to L750.

Copy link
Member Author

Choose a reason for hiding this comment

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

@0xnm regarding the choreographer...you are not wrong, for some reason during my tests sometimes it was not triggered if I didn't play a bit with the activity.

Copy link
Member Author

Choose a reason for hiding this comment

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

very minor: this comment belongs to L750.

yeah, I think it was messed up after I did the refactor by extracting the variable.

@mariusc83 mariusc83 force-pushed the mconstantin/rumm-2183/vitals-update-frequency-api-nightly-tests branch from 6c6a438 to 679fc51 Compare May 11, 2022 12:01
Base automatically changed from mconstantin/rumm-2076/mobile-vitals-uptate-frequency-option to develop May 12, 2022 13:13
@mariusc83 mariusc83 merged commit c05f878 into develop May 13, 2022
@mariusc83 mariusc83 deleted the mconstantin/rumm-2183/vitals-update-frequency-api-nightly-tests branch May 13, 2022 06:58
@xgouchet xgouchet added this to the 1.14.0 milestone Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size-small This PR is small sized
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants