1910764 - Update Google Play Services Ads Identifier to version 18.1.0

Status: RESOLVED FIXED

(Firefox for Android :: Experimentation and Telemetry, task)

Description

[Ryan VanderMeulen [:RyanVM]]

We're currently on version 16.0.0, which is nearly 6 years old at this point. While it still works, the library has had a number of changes to it since and I'm worried that we'll eventually end up in a situation like bug 1903346 if we don't get updated sooner or later. The current release is version 18.1.0, and below are changelog links for the updates which have shipped since 16.0.0:
https://developers.google.com/android/guides/releases#june_17_2019
https://developers.google.com/android/guides/releases#may_26_2021
https://developers.google.com/android/guides/releases#september_22_2021_2
https://developers.google.com/android/guides/releases#december_09_2021
https://developers.google.com/android/guides/releases#january_05_2022
https://developers.google.com/android/guides/releases#may_29_2024

Of particular note, the com.google.android.gms.permission.AD_ID permission is declared by the library now, removing our need to explicitly do so for Fenix & Focus unless we see value in keeping ours around for clarity's sake.

Of bigger note, we now see a unit test failure:
mobile/android/fenix/app/src/test/java/org/mozilla/fenix/components/metrics/MetricsUtilsTest.kt:53:75: error: Null can not be a value of a non-null type AdvertisingIdClient.Info

Due to library nullability changes, AdvertisingIdClient.getAdvertisingIdInfo can't return null anymore. This will require changes in our code and tests. I haven't been able to find the right path forward on that, so I'm hoping someone with more domain knowledge will be able to take it from here.

Comment 1

[Jonathan Almeida [:jonalmeida]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #0)

We're currently on version 16.0.0, which is nearly 6 years old at this point. While it still works, the library has had a number of changes to it since and I'm worried that we'll eventually end up in a situation like bug 1903346 if we don't get updated sooner or later. The current release is version 18.1.0, and below are changelog links for the updates which have shipped since 16.0.0:
https://developers.google.com/android/guides/releases#june_17_2019
https://developers.google.com/android/guides/releases#may_26_2021
https://developers.google.com/android/guides/releases#september_22_2021_2
https://developers.google.com/android/guides/releases#december_09_2021
https://developers.google.com/android/guides/releases#january_05_2022
https://developers.google.com/android/guides/releases#may_29_2024

Of particular note, the com.google.android.gms.permission.AD_ID permission is declared by the library now, removing our need to explicitly do so for Fenix & Focus unless we see value in keeping ours around for clarity's sake.

Thanks for the links to the release notes! I agree that the only one of significance is the september_22_2021_2 which lets us remove the permission.

Of bigger note, we now see a unit test failure:
mobile/android/fenix/app/src/test/java/org/mozilla/fenix/components/metrics/MetricsUtilsTest.kt:53:75: error: Null can not be a value of a non-null type AdvertisingIdClient.Info

We can either:

1. Remove this test. It doesn't seem to have any value now that the result (if no exception is thrown) is non-null.
2. Update the test to return a mock that will then [return a null from the AdvertisingIdClient.getAdvertisingIdInfo(context).id call]. (sample patch below.)

I have no preferences here.

Due to library nullability changes, AdvertisingIdClient.getAdvertisingIdInfo can't return null anymore. This will require changes in our code and tests. I haven't been able to find the right path forward on that, so I'm hoping someone with more domain knowledge will be able to take it from here.

I don't think we should change our production code here because the library can still throw exceptions, so returning null in the wrapped method internal fun getAdvertisingID(context: Context): String? is fine if we go with either of the two test fixes above.

---

diff --git a/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/components/metrics/MetricsUtilsTest.kt b/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/components/metrics/MetricsUtilsTest.>
--- a/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/components/metrics/MetricsUtilsTest.kt
+++ b/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/components/metrics/MetricsUtilsTest.kt
@@ -45,29 +45,33 @@ class MetricsUtilsTest {
         }

         unmockkStatic("com.google.android.gms.ads.identifier.AdvertisingIdClient")
     }

     @Test
     fun `getAdvertisingID() returns null if the API returns null info`() {
         mockkStatic(AdvertisingIdClient::class)
-        every { AdvertisingIdClient.getAdvertisingIdInfo(any()) } returns null
+        val mockInfo: AdvertisingIdClient.Info = mockk()
+        every { mockInfo.id } returns null
+        every { AdvertisingIdClient.getAdvertisingIdInfo(any()) } returns mockInfo

         assertNull(MetricsUtils.getAdvertisingID(context))
     }

     @Test
     fun `getAdvertisingID() returns a valid string if the API returns a valid ID`() {
         val testId = "test-value-id"
+        val mockInfo: AdvertisingIdClient.Info = mockk()
+        every { mockInfo.id } returns testId

         mockkStatic(AdvertisingIdClient::class)
         every {
             AdvertisingIdClient.getAdvertisingIdInfo(any())
-        } returns AdvertisingIdClient.Info(testId, false)
+        } returns mockInfo

         assertEquals(testId, MetricsUtils.getAdvertisingID(context))
     }

     @Test
     fun `getHashedIdentifier() returns a hashed identifier`() = runTest {
         val testId = "test-value-id"
         val testPackageName = "org.mozilla-test.fenix"

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Jon's suggestion appears to work (as does removing the test outright), but apparently a new failure has appeared with this library bump in the intervening time since this was last tested.

https://treeherder.mozilla.org/logviewer?job_id=471960993&repo=try&lineNumber=7286
TEST-UNEXPECTED-FAIL | org.mozilla.fenix.components.metrics.ActivationPingTest.checkAndSend() triggers the ping if it wasn't marked as triggered | java.lang.AssertionError: Verification failed: call 1 of 1: ActivationPing(#1149).markAsTriggered$app_fenixDebug()) was not called.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Attachment: Bug 1910764 - Update Google Play Services Ads Identifier to version 18.1.0.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Newer log link:
https://treeherder.mozilla.org/logviewer?job_id=483823073&repo=try&lineNumber=7923

Comment 5

[Zac McKenney [:zmckenney]]

The issue you're facing now is that the mockk() context you're passing into ActivationPing is being used when trying to get the Advertising ID. It is trying to get the application context from a mocked context and since you're just verifying the calls and I don't think actually need to look up anything with the resource (by using the testContext extension), you could just modify the code as shown to make the test pass again.

Index: mobile/android/fenix/app/src/test/java/org/mozilla/fenix/components/metrics/ActivationPingTest.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/components/metrics/ActivationPingTest.kt b/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/components/metrics/ActivationPingTest.kt
--- a/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/components/metrics/ActivationPingTest.kt	(revision 4e859985c4b3fc0a842cef5d9c92e92eb539e851)
+++ b/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/components/metrics/ActivationPingTest.kt	(date 1732316994429)
@@ -4,6 +4,7 @@
 
 package org.mozilla.fenix.components.metrics
 
+import android.content.Context
 import io.mockk.Runs
 import io.mockk.every
 import io.mockk.just
@@ -15,7 +16,10 @@
 internal class ActivationPingTest {
     @Test
     fun `checkAndSend() triggers the ping if it wasn't marked as triggered`() {
-        val mockAp = spyk(ActivationPing(mockk()), recordPrivateCalls = true)
+        val context: Context = mockk()
+        every {context.applicationContext} returns mockk()
+
+        val mockAp = spyk(ActivationPing(context), recordPrivateCalls = true)
         every { mockAp.wasAlreadyTriggered() } returns false
         every { mockAp.markAsTriggered() } just Runs

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a3e900b15f35
Update Google Play Services Ads Identifier to version 18.1.0. r=android-reviewers,zmckenney

Comment 7

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/a3e900b15f35