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-807 Add SR integration tests for TextView and EditText view type #1593

Merged
merged 2 commits into from
Sep 1, 2023
Merged

RUM-807 Add SR integration tests for TextView and EditText view type #1593

merged 2 commits into from
Sep 1, 2023

Conversation

mariusc83
Copy link
Member

@mariusc83 mariusc83 commented Aug 31, 2023 •
edited
Loading

What does this PR do?

Testing Strategy

This PR lays the groundwork for a testing approach that focuses on verifying the Transformer/Persister component. The core idea is to compare the persisted JSON payload with a predefined expected payload derived from screen elements.

By systematically assessing the JSON payload against the expected output, we can effectively identify and prevent potential regressions during development.

This initial implementation covers a simple View containing a TextView and an EditText. Future enhancements will expand the test coverage to encompass more complex UI elements and interactions. This will come up in the following PRs.

Expected Benefits

This new testing strategy is expected to significantly improve the overall quality and stability of the Session Replay feature by proactively detecting and addressing regression issues.

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 Aug 31, 2023
@codecov-commenter
Copy link

codecov-commenter commented Aug 31, 2023 •
edited
Loading

Codecov Report

Merging #1593 (9763fc3) into develop (bf9b6c3) will increase coverage by 0.04%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #1593      +/-   ##
===========================================
+ Coverage    83.58%   83.62%   +0.04%     
===========================================
  Files          443      443              
  Lines        15122    15122              
  Branches      2264     2264              
===========================================
+ Hits         12639    12645       +6     
+ Misses        1885     1873      -12     
- Partials       598      604       +6

see 16 files with indirect coverage changes

@mariusc83 mariusc83 marked this pull request as ready for review August 31, 2023 08:33
@mariusc83 mariusc83 requested a review from a team as a code owner August 31, 2023 08:33
xgouchet
xgouchet previously approved these changes Aug 31, 2023
View reviewed changes
0xnm
0xnm previously approved these changes Aug 31, 2023
View reviewed changes
Copy link
Member

@0xnm 0xnm left a comment

Choose a reason for hiding this comment

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

lgtm. I left few comments, but they are not blocking.

instrumented/integration/src/main/kotlin/com/datadog/android/sdk/utils/IntentExtensions.kt Outdated
Comment on lines 19 to 21
internal const val ALLOW = 1
internal const val MASK_USER_INPUT = 2
internal const val MASK = 3
Copy link
Member

Choose a reason for hiding this comment

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

are they needed? we can add SessionReplayPrivacy enum values to the intent extra directly and read them back using getSerializableExtra, because any enum is serializable.

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, check the tests as I have one for each of these values and pass those values in the IntentExtras :
https://github.com/DataDog/dd-sdk-android/pull/1593/files#diff-a691b85cf560e7461b22d88e7bd08c8d5ec192584d8da98cb159fb568c5e6ec4R28

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 am not sure we can pass an Enum in a Bundle, are we ?

Copy link
Member

Choose a reason for hiding this comment

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

yes, we can, so these values are not needed.

...tegration/src/androidTest/kotlin/com/datadog/android/sdk/integration/sessionreplay/SrTest.kt
Comment on lines +164 to +170
EXACT,
CONTAINS
Copy link
Member

Choose a reason for hiding this comment

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

maybe we should document it.

...c/main/kotlin/com/datadog/android/sdk/integration/sessionreplay/BaseSessionReplayActivity.kt Outdated
Comment on lines 41 to 42
// we will use a large long task threshold to make sure we will not have LongTask events
// noise in our integration tests.
Copy link
Member

Choose a reason for hiding this comment

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

since we don't check RUM data, it is probably not needed.

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 was a leftover

@mariusc83 mariusc83 dismissed stale reviews from 0xnm and xgouchet via 8e6d3a3 August 31, 2023 11:51
@mariusc83 mariusc83 force-pushed the mconstantin/rum-807/add-integration-tests-for-text-fields-page branch 3 times, most recently from 19ee787 to b861ba4 Compare August 31, 2023 14:24
@mariusc83 mariusc83 requested review from 0xnm and xgouchet August 31, 2023 14:34
0xnm
0xnm previously approved these changes Aug 31, 2023
View reviewed changes
@mariusc83 mariusc83 force-pushed the mconstantin/rum-807/add-integration-tests-for-text-fields-page branch from b861ba4 to cfc5d47 Compare August 31, 2023 14:42
@mariusc83 mariusc83 force-pushed the mconstantin/rum-807/add-integration-tests-for-text-fields-page branch 5 times, most recently from ed6f602 to 8c452f0 Compare September 1, 2023 07:32
@mariusc83 mariusc83 force-pushed the mconstantin/rum-807/add-integration-tests-for-text-fields-page branch from 8c452f0 to 7a2f184 Compare September 1, 2023 08:22
@mariusc83 mariusc83 merged commit dee218f into develop Sep 1, 2023
@mariusc83 mariusc83 deleted the mconstantin/rum-807/add-integration-tests-for-text-fields-page branch September 1, 2023 09:04
@xgouchet xgouchet added this to the 2.1.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

@xgouchet xgouchet Awaiting requested review from xgouchet

Assignees

@mariusc83 mariusc83

Labels
None yet
Projects
None yet
Milestone
2.1.0
Development

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