diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt index 633378856b..ee4f54d217 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt +++ b/glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt @@ -63,7 +63,7 @@ open class GleanInternalAPI internal constructor () { // This is the wrapped http uploading mechanism: provides base functionalities // for logging and delegates the actual upload to the implementation in // the `Configuration`. - internal val httpClient by lazy { BaseUploader(configuration.httpClient) } + internal lateinit var httpClient: BaseUploader private lateinit var applicationContext: Context @@ -140,6 +140,7 @@ open class GleanInternalAPI internal constructor () { this.applicationContext = applicationContext this.configuration = configuration + this.httpClient = BaseUploader(configuration.httpClient) this.gleanDataDir = File(applicationContext.applicationInfo.dataDir, GLEAN_DATA_DIR) setUploadEnabled(uploadEnabled) diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/TestUtil.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/TestUtil.kt index bd693d47bc..af34f3c177 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/TestUtil.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/TestUtil.kt @@ -208,11 +208,12 @@ internal fun waitForEnqueuedWorker( * is fixed, the contents of this can be amended to trigger WorkManager directly. * * @param context the application [Context] to get the [WorkManager] instance for + * @param workerTag the tag for the expected worker (default: `PingUploadWorker.PING_WORKER_TAG`) */ -internal fun triggerWorkManager(context: Context) { +internal fun triggerWorkManager(context: Context, workerTag: String = PingUploadWorker.PING_WORKER_TAG) { // Check that the work is scheduled - val status = getWorkerStatus(context, PingUploadWorker.PING_WORKER_TAG) - Assert.assertTrue("A scheduled PingUploadWorker must exist", + val status = getWorkerStatus(context, workerTag) + Assert.assertTrue("A scheduled $workerTag must exist", status.isEnqueued) // Trigger WorkManager using TestDriver diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/scheduler/DeletionPingUploadWorkerTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/scheduler/DeletionPingUploadWorkerTest.kt index e797fbdc50..874cccc605 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/scheduler/DeletionPingUploadWorkerTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/scheduler/DeletionPingUploadWorkerTest.kt @@ -5,17 +5,27 @@ import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.work.testing.WorkManagerTestInitHelper import java.io.File +import mozilla.components.support.test.any import mozilla.telemetry.glean.Dispatchers import mozilla.telemetry.glean.Glean import mozilla.telemetry.glean.GleanInternalAPI import mozilla.telemetry.glean.config.Configuration import mozilla.telemetry.glean.getWorkerStatus +import mozilla.telemetry.glean.net.HeadersList +import mozilla.telemetry.glean.net.PingUploader +import mozilla.telemetry.glean.resetGlean +import mozilla.telemetry.glean.triggerWorkManager import org.junit.After +import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test import org.junit.runner.RunWith +import org.mockito.Mockito.anyString +import org.mockito.Mockito.spy +import org.mockito.Mockito.times +import org.mockito.Mockito.verify import org.robolectric.shadows.ShadowLog @RunWith(AndroidJUnit4::class) @@ -63,4 +73,44 @@ class DeletionPingUploadWorkerTest { assertTrue(getWorkerStatus(context, DeletionPingUploadWorker.PING_WORKER_TAG).isEnqueued) } + + /** + * A stub uploader class that does not upload anything, but we can spy on it. + */ + private class TestUploader : PingUploader { + override fun upload(url: String, data: String, headers: HeadersList): Boolean { + return true + } + } + + @Test + fun `deletion-request pings are only sent when toggled from on to off`() { + val httpClientSpy = spy(TestUploader()) + + // Use the test client in the Glean configuration + val config = Configuration(httpClient = httpClientSpy) + resetGlean(context, config) + + // Get directory for pending deletion-request pings + val pendingDeletionRequestDir = File(Glean.getDataDir(), DeletionPingUploadWorker.DELETION_PING_DIR) + + // Disabling upload generates a deletion ping + Glean.setUploadEnabled(false) + assertEquals(1, pendingDeletionRequestDir.listFiles()?.size) + + // Trigger the upload manager and check that the upload was started + triggerWorkManager(context, DeletionPingUploadWorker.PING_WORKER_TAG) + verify(httpClientSpy, times(1)).upload(anyString(), anyString(), any()) + + // Re-setting upload to `false` should not generate an additional ping + // and no worker should be scheduled. + Glean.setUploadEnabled(false) + + assertFalse(getWorkerStatus(context, DeletionPingUploadWorker.PING_WORKER_TAG).isEnqueued) + // Upload was definitely not triggered again + verify(httpClientSpy, times(1)).upload(anyString(), anyString(), any()) + + // No new file should have been written + assertEquals(0, pendingDeletionRequestDir.listFiles()?.size) + } } diff --git a/glean-core/src/lib.rs b/glean-core/src/lib.rs index ffafce17a3..67d7593975 100644 --- a/glean-core/src/lib.rs +++ b/glean-core/src/lib.rs @@ -276,14 +276,14 @@ impl Glean { pub fn set_upload_enabled(&mut self, flag: bool) -> bool { log::info!("Upload enabled: {:?}", flag); - // When upload is disabled, submit a deletion-request ping - if !flag { - if let Err(err) = self.internal_pings.deletion_request.submit(self, None) { - log::error!("Failed to send deletion-request ping on optout: {}", err); + if self.upload_enabled != flag { + // When upload is disabled, submit a deletion-request ping + if !flag { + if let Err(err) = self.internal_pings.deletion_request.submit(self, None) { + log::error!("Failed to submit deletion-request ping on optout: {}", err); + } } - } - if self.upload_enabled != flag { self.upload_enabled = flag; self.on_change_upload_enabled(flag); true diff --git a/glean-core/tests/ping.rs b/glean-core/tests/ping.rs index f95203639c..765297aea5 100644 --- a/glean-core/tests/ping.rs +++ b/glean-core/tests/ping.rs @@ -64,6 +64,24 @@ fn disabling_upload_clears_pending_pings() { assert_eq!(1, get_queued_pings(glean.get_data_path()).unwrap().len()); } +#[test] +fn deletion_request_only_when_toggled_from_on_to_off() { + let (mut glean, _) = new_glean(None); + + // Disabling upload generates a deletion ping + glean.set_upload_enabled(false); + assert_eq!(1, get_deletion_pings(glean.get_data_path()).unwrap().len()); + + // Re-setting it to `false` should not generate an additional ping. + // As we didn't clear the pending ping, that's the only one that sticks around. + glean.set_upload_enabled(false); + assert_eq!(1, get_deletion_pings(glean.get_data_path()).unwrap().len()); + + // Toggling back to true won't generate a ping either. + glean.set_upload_enabled(true); + assert_eq!(1, get_deletion_pings(glean.get_data_path()).unwrap().len()); +} + #[test] fn empty_pings_with_flag_are_sent() { let (mut glean, _) = new_glean(None);