RUN integration tests on API 35 in testing pyramid #2272

mariusc83 · 2024-09-19T15:02:00Z

What does this PR do?

Updates the latest API version for running our integration tests to API 35.

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)

0xnm

lgtm!

as a part of API 35 support, we also need to modify target SDK, build tools version, etc. in the Gradle config and Docker file as well.

codecov-commenter · 2024-09-19T15:36:41Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.98%. Comparing base (529e791) to head (85ea30a).
Report is 31 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #2272   +/-   ##
========================================
  Coverage    69.98%   69.98%           
========================================
  Files          731      731           
  Lines        27204    27212    +8     
  Branches      4574     4577    +3     
========================================
+ Hits         19037    19043    +6     
+ Misses        6896     6881   -15     
- Partials      1271     1288   +17

Files with missing lines	Coverage Δ
...onreplay/internal/recorder/resources/BitmapPool.kt	`87.50% <ø> (ø)`
...ay/internal/recorder/resources/BitmapPoolHelper.kt	`57.14% <ø> (ø)`
...android/sessionreplay/internal/utils/CacheUtils.kt	`11.11% <ø> (ø)`

... and 30 files with indirect coverage changes

0xnm · 2024-09-20T14:55:33Z

...ain/kotlin/com/datadog/android/sessionreplay/internal/recorder/resources/BitmapPoolHelper.kt


 internal class BitmapPoolHelper(
    private val invocationUtils: InvocationUtils = InvocationUtils()
 ) {
    internal fun generateKey(bitmap: Bitmap) =
-        generateKey(bitmap.width, bitmap.height, bitmap.config)
+        generateKey(bitmap.width, bitmap.height, bitmap.safeConfig())


So how it was working in the past? we now know, that annotation wasn't correct and Bitmap.getConfig may return null. It should crash runtime, because generateKey doesn't expect null value for the config parameter.

Also do we need safeConfig method? Maybe in some code paths we can handle null returned case without fallback value.

yes...in case it will return null it will crash. I've never seen this bug reported yet so I suspect this happens quite rarely. I am not sure that this will require a hotfix. @xgouchet @jonathanmos ?

the reason why I introduce the safeConfig is just to be re - used also in some tests where I had to replace it and maybe for later...looks cleaner this way.

The getConfig method is annotated NonNull for some reason. I haven't seen any cases of this returning null, but if it can then we do indeed need a safe method.

@jonathanmos It was annotated as NonNull by mistake, see https://cs.android.com/android/_/android/platform/frameworks/base/+/a4c2f9badb21180282596ac9730d4777f65e4b4b

The question was more if we can leave it like this until the next release where the safe method will be in place. You said you did not see any issue so far so I guess this is very rare when it returns null. We could leave it like this without a hotfix.

Warning
I'm not at all convinced we need a safeCongif here as we lose the information. Any local log or telemetry that could be sent from the generated key will hide the fact that the bitmap configuration was null.
In the current case, I'd prefer to have a generateKey allowing a nullable config instead

I didn't see any telemetry there, in any case the Config in this case is just a variable in the key generation so with our without it null will have more or less same changes of collisions for 2 elements with Bitmap.config null so I am ok with any approach. @jonathanmos , @0xnm what do you guys think ?

0xnm · 2024-09-20T14:56:13Z

...ession-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/utils/CacheUtils.kt

+
+    // some of this memory level are not being triggered after API 34. We still need to keep them for now
+    // for lower versions
+    @Suppress("DEPRECATION")


what is the replacement then? should we use it for newer APIs?

I think we can leave it like it is and prepare a task to remove those later. The only think that will happen for newer APIs is that those level will never be triggered. I don't see any harm now, maybe to create a task to clean these levels ?

yes, no harm. My point is just that if something is deprecated, then probably there is a replacement for the newer APIs?

Nope. no replacement suggested in their docs, I think they simply don't support these levels anymore and only kept few important ones. I think they did not need this granularity anymore.

xgouchet · 2024-09-23T07:43:15Z

...tlin/com/datadog/android/core/internal/persistence/file/datastore/DataStoreFileReaderTest.kt

@@ -142,7 +142,7 @@ internal class DataStoreFileReaderTest {
    @Test
    fun `M log error W read() { invalid number of blocks }`() {
        // Given
-        blocksReturned.removeLast()
+        blocksReturned.removeAt(blocksReturned.size - 1)


Suggested change

blocksReturned.removeAt(blocksReturned.size - 1)

blocksReturned.removeAt(blocksReturned.lastIndex)

xgouchet · 2024-09-23T07:43:22Z

...tlin/com/datadog/android/core/internal/persistence/file/datastore/DataStoreFileReaderTest.kt

@@ -213,7 +213,7 @@ internal class DataStoreFileReaderTest {
    @Test
    fun `M return onFailure W read() { invalid number of blocks }`() {
        // Given
-        blocksReturned.removeLast()
+        blocksReturned.removeAt(blocksReturned.size - 1)


Suggested change

blocksReturned.removeAt(blocksReturned.size - 1)

blocksReturned.removeAt(blocksReturned.lastIndex)

xgouchet · 2024-09-23T07:47:39Z

...ain/kotlin/com/datadog/android/sessionreplay/internal/recorder/resources/BitmapPoolHelper.kt


 internal class BitmapPoolHelper(
    private val invocationUtils: InvocationUtils = InvocationUtils()
 ) {
    internal fun generateKey(bitmap: Bitmap) =
-        generateKey(bitmap.width, bitmap.height, bitmap.config)
+        generateKey(bitmap.width, bitmap.height, bitmap.safeConfig())


Warning
I'm not at all convinced we need a safeCongif here as we lose the information. Any local log or telemetry that could be sent from the generated key will hide the fact that the bitmap configuration was null.
In the current case, I'd prefer to have a generateKey allowing a nullable config instead

xgouchet · 2024-09-24T06:41:23Z

...ain/kotlin/com/datadog/android/sessionreplay/internal/recorder/resources/BitmapPoolHelper.kt

+        return if (config != null) {
+            "$width-$height-$config"
+        } else {
+            "$width-$height-null"
+        }


Note
the String templating with a nullable variable will give the same result, you don't have to do this

Suggested change

return if (config != null) {

"$width-$height-$config"

} else {

"$width-$height-null"

}

return "$width-$height-$config"

yes just a weird autocomplete

sorry this comment was for the testing assertion message but for this case I was not aware that having null value in the string format will not actually break so I preferred the safer way. In the end I like it more as it is more explicit regarding what to expect.

xgouchet · 2024-09-24T06:44:13Z

.../test/kotlin/com/datadog/android/sessionreplay/internal/recorder/resources/BitmapPoolTest.kt

+        testedCache.getBitmapByProperties(mockBitmap.width, mockBitmap.height, mockConfig)

        // When
        testedCache.put(mockBitmap)

        // Then
-        val actualBitmap = testedCache.getBitmapByProperties(mockBitmap.width, mockBitmap.height, mockBitmap.config)
+        val actualBitmap =
+            testedCache.getBitmapByProperties(mockBitmap.width, mockBitmap.height, mockConfig)


Note
Why are those changes necessary ?

mockBitmap.config returns nullable now and that method expects nonnull so we are using directly the mockConfig with which this mockBitmap is mocked with.

Why can't we make the getBitmapByProperties() method accept a nullable Config? The only usage of the config is to call the generateKey which this PR modifies to allow a nullable config.

But why we should change this method if this code if this is not required ? All the methods calling this code are keeping the contract, if we would change this code we would change only for the sake of this test, this is not the same thing as the BitmapPoolHelper#generateKey method which accepts a bitmap and delegates to generateKey method. This method has a different scope and requires in the contract that the config should be NonNull when calling it. I really don't get why do you want to change this.

xgouchet · 2024-09-24T06:44:46Z

...lay/src/test/kotlin/com/datadog/android/sessionreplay/internal/utils/BitmapPoolHelperTest.kt

+        val key = testedHelper.generateKey(mockBitmap)
+
+        // Then
+        assertThat(key).isEqualTo("$fakeWidth-$fakeHeight-${null}")


Suggested change

assertThat(key).isEqualTo("$fakeWidth-$fakeHeight-${null}")

assertThat(key).isEqualTo("$fakeWidth-$fakeHeight-null")

RUN integration tests on API 35 in testing pyramid

cf9e60b

mariusc83 self-assigned this Sep 19, 2024

mariusc83 requested review from a team as code owners September 19, 2024 15:02

0xnm approved these changes Sep 19, 2024

View reviewed changes

xgouchet approved these changes Sep 19, 2024

View reviewed changes

RUM-6069 Update the build system and CI to target API 35

5a8be07

xgouchet approved these changes Sep 20, 2024

View reviewed changes

0xnm approved these changes Sep 20, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-6069/run-integration-tests-on-api-35 branch from d3892b5 to e0c771b Compare September 20, 2024 12:01

ambushwork approved these changes Sep 20, 2024

View reviewed changes

mariusc83 requested review from xgouchet, ambushwork and 0xnm September 20, 2024 14:46

ambushwork approved these changes Sep 20, 2024

View reviewed changes

0xnm approved these changes Sep 20, 2024

View reviewed changes

xgouchet requested changes Sep 23, 2024

View reviewed changes

jonathanmos approved these changes Sep 23, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-6069/run-integration-tests-on-api-35 branch from e0c771b to bedcb09 Compare September 23, 2024 12:31

mariusc83 requested review from xgouchet, ambushwork, 0xnm and jonathanmos September 23, 2024 12:32

mariusc83 force-pushed the mconstantin/rum-6069/run-integration-tests-on-api-35 branch 2 times, most recently from cec0387 to 29219d7 Compare September 23, 2024 14:36

xgouchet requested changes Sep 24, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-6069/run-integration-tests-on-api-35 branch from 29219d7 to 9a1cb71 Compare September 24, 2024 09:20

Fixes and updates related with API 35 source code.

478df2e

mariusc83 force-pushed the mconstantin/rum-6069/run-integration-tests-on-api-35 branch from 9a1cb71 to 478df2e Compare September 24, 2024 09:28

mariusc83 requested a review from xgouchet September 24, 2024 11:35

Accept nullable Config in BitmapPool just for testing purposes

85ea30a

xgouchet approved these changes Sep 25, 2024

View reviewed changes

mariusc83 merged commit 64ffa00 into develop Sep 25, 2024
23 checks passed

mariusc83 deleted the mconstantin/rum-6069/run-integration-tests-on-api-35 branch September 25, 2024 15:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUN integration tests on API 35 in testing pyramid #2272

RUN integration tests on API 35 in testing pyramid #2272

mariusc83 commented Sep 19, 2024

0xnm left a comment

codecov-commenter commented Sep 19, 2024 •

edited

Loading

0xnm Sep 20, 2024

mariusc83 Sep 20, 2024

mariusc83 Sep 20, 2024

jonathanmos Sep 23, 2024 •

edited

Loading

0xnm Sep 23, 2024

mariusc83 Sep 23, 2024

xgouchet Sep 23, 2024

mariusc83 Sep 23, 2024

0xnm Sep 20, 2024

mariusc83 Sep 20, 2024

0xnm Sep 23, 2024

mariusc83 Sep 23, 2024

xgouchet Sep 23, 2024

xgouchet Sep 23, 2024

xgouchet Sep 23, 2024

xgouchet Sep 24, 2024

mariusc83 Sep 24, 2024

mariusc83 Sep 25, 2024

xgouchet Sep 24, 2024

mariusc83 Sep 24, 2024

xgouchet Sep 25, 2024

mariusc83 Sep 25, 2024

xgouchet Sep 24, 2024

	blocksReturned.removeAt(blocksReturned.size - 1)
	blocksReturned.removeAt(blocksReturned.lastIndex)

	assertThat(key).isEqualTo("$fakeWidth-$fakeHeight-${null}")
	assertThat(key).isEqualTo("$fakeWidth-$fakeHeight-null")

RUN integration tests on API 35 in testing pyramid #2272

RUN integration tests on API 35 in testing pyramid #2272

Conversation

mariusc83 commented Sep 19, 2024

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

0xnm left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 19, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanmos Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 19, 2024 •

edited

Loading

jonathanmos Sep 23, 2024 •

edited

Loading