From 350486623cb4a9677e3792f3d6c343b16cbc563d Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Fri, 21 Feb 2020 13:36:45 +0100 Subject: [PATCH 1/3] Ensure we only generate a deletion-request ping when toggling from on to off. --- .../java/mozilla/telemetry/glean/TestUtil.kt | 7 +-- .../scheduler/DeletionPingUploadWorkerTest.kt | 50 +++++++++++++++++++ glean-core/tests/ping.rs | 18 +++++++ 3 files changed, 72 insertions(+), 3 deletions(-) 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/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); From e8882712c7e4628207827c1f12591078500b9b6f Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Fri, 21 Feb 2020 13:38:11 +0100 Subject: [PATCH 2/3] Only trigger a deletion-request ping when actually switching from on to off. The old code already did what we wanted, because when `upload_enabled` is false the ping submission will exit early. This just makes it more explicit, easier to read and understand without that context and we avoid a useless log line. --- glean-core/src/lib.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) 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 From b2157fc8844306adacdb9a0717dbaeb1fe6c5f83 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Fri, 21 Feb 2020 15:44:43 +0100 Subject: [PATCH 3/3] Properly set the http client on every init Under normal use Glean is initialized exactly once. No state is ever reset or reinitialized. Tests however work differently. We need to reset all data. Through configuration we can swap out the used HTTP uploader. Previously however the uploader used by upload workers was created once, lazily. Even if we (test-)reset and change the uploader it wasn't overriden. We can instead make it `lateinit`, thus allowing us to reset it to the value we get from the configuration during `initialize` --- .../android/src/main/java/mozilla/telemetry/glean/Glean.kt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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)