Skip to content

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

Merged
mariusc83 merged 2 commits into
developfrom
mconstantin/rum-807/add-integration-tests-for-text-fields-page
Sep 1, 2023
Merged

RUM-807 Add SR integration tests for TextView and EditText view type#1593
mariusc83 merged 2 commits into
developfrom
mconstantin/rum-807/add-integration-tests-for-text-fields-page

Conversation

@mariusc83

@mariusc83 mariusc83 commented Aug 31, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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

codecov-commenter commented Aug 31, 2023 •
edited
Loading

Copy link
Copy Markdown

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

@0xnm 0xnm left a comment

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.

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

Comment thread 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

@0xnm 0xnm Aug 31, 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.

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.

@mariusc83 mariusc83 Aug 31, 2023

Copy link
Copy Markdown
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

@mariusc83 mariusc83 Aug 31, 2023

Copy link
Copy Markdown
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 ?

@0xnm 0xnm Aug 31, 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.

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

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

@0xnm 0xnm Aug 31, 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.

maybe we should document it.

Comment thread ...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.

@0xnm 0xnm Aug 31, 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.

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

@mariusc83 mariusc83 Aug 31, 2023

Copy link
Copy Markdown
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