From 7fb266063e9057e23210d0df0ed3df5891511921 Mon Sep 17 00:00:00 2001 From: Travis Long Date: Thu, 21 Feb 2019 08:11:35 -0600 Subject: [PATCH] Create a way for glean test API functions to await async IO operations Use the Job returned from calls to Dispatchers.API.launch to attempt to wait for the IO operation to complete using .join() before test functions return values. This also uses a withTimeout block in order to prevent waiting for too long. Update test API docstrings to reflect awaiting behavior. --- .../service/glean/BooleanMetricType.kt | 18 ++++++++++--- .../service/glean/CommonMetricData.kt | 25 +++++++++++++++++++ .../service/glean/CounterMetricType.kt | 17 ++++++++++--- .../service/glean/DatetimeMetricType.kt | 25 +++++++++++++++---- .../service/glean/EventMetricType.kt | 17 ++++++++++--- .../service/glean/StringListMetricType.kt | 19 +++++++++++--- .../service/glean/StringMetricType.kt | 17 ++++++++++--- .../service/glean/UuidMetricType.kt | 20 +++++++++++---- 8 files changed, 132 insertions(+), 26 deletions(-) diff --git a/components/service/glean/src/main/java/mozilla/components/service/glean/BooleanMetricType.kt b/components/service/glean/src/main/java/mozilla/components/service/glean/BooleanMetricType.kt index 823f6f6382b..5da13cca5ec 100644 --- a/components/service/glean/src/main/java/mozilla/components/service/glean/BooleanMetricType.kt +++ b/components/service/glean/src/main/java/mozilla/components/service/glean/BooleanMetricType.kt @@ -5,6 +5,7 @@ package mozilla.components.service.glean import android.support.annotation.VisibleForTesting +import kotlinx.coroutines.Job import kotlinx.coroutines.launch import mozilla.components.service.glean.storages.BooleansStorageEngine import mozilla.components.support.base.log.logger.Logger @@ -29,6 +30,10 @@ data class BooleanMetricType( private val logger = Logger("glean/BooleanMetricType") + // Holds the Job returned from launch{} for awaiting purposes + @VisibleForTesting(otherwise = VisibleForTesting.NONE) + var ioTask: Job? = null + /** * Set a boolean value. * @@ -41,7 +46,7 @@ data class BooleanMetricType( return } - Dispatchers.API.launch { + ioTask = Dispatchers.API.launch { // Delegate storing the boolean to the storage engine. BooleansStorageEngine.record( this@BooleanMetricType, @@ -51,7 +56,9 @@ data class BooleanMetricType( } /** - * Tests whether a value is stored for the metric for testing purposes only + * Tests whether a value is stored for the metric for testing purposes only. This function will + * attempt to await the last task (if any) writing to the the metric's storage engine before + * returning a value. * * @param pingName represents the name of the ping to retrieve the metric for. Defaults * to the either the first value in [defaultStorageDestinations] or the first @@ -60,11 +67,14 @@ data class BooleanMetricType( */ @VisibleForTesting(otherwise = VisibleForTesting.NONE) fun testHasValue(pingName: String = getStorageNames().first()): Boolean { + ioTask?.let { awaitJob(it) } + return BooleansStorageEngine.getSnapshot(pingName, false)?.get(identifier) != null } /** - * Returns the stored value for testing purposes only + * Returns the stored value for testing purposes only. This function will attempt to await the + * last task (if any) writing to the the metric's storage engine before returning a value. * * @param pingName represents the name of the ping to retrieve the metric for. Defaults * to the either the first value in [defaultStorageDestinations] or the first @@ -74,6 +84,8 @@ data class BooleanMetricType( */ @VisibleForTesting(otherwise = VisibleForTesting.NONE) fun testGetValue(pingName: String = getStorageNames().first()): Boolean { + ioTask?.let { awaitJob(it) } + return BooleansStorageEngine.getSnapshot(pingName, false)!![identifier]!! } } diff --git a/components/service/glean/src/main/java/mozilla/components/service/glean/CommonMetricData.kt b/components/service/glean/src/main/java/mozilla/components/service/glean/CommonMetricData.kt index 575cb0da054..cdfdc032a8c 100644 --- a/components/service/glean/src/main/java/mozilla/components/service/glean/CommonMetricData.kt +++ b/components/service/glean/src/main/java/mozilla/components/service/glean/CommonMetricData.kt @@ -4,6 +4,11 @@ package mozilla.components.service.glean +import android.support.annotation.VisibleForTesting +import kotlinx.coroutines.Job +import kotlinx.coroutines.TimeoutCancellationException +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withTimeout import mozilla.components.support.base.log.logger.Logger /** @@ -39,6 +44,7 @@ interface CommonMetricData { companion object { internal const val DEFAULT_STORAGE_NAME = "default" + private const val JOB_TIMEOUT_MS = 250L } fun shouldRecord(logger: Logger): Boolean { @@ -76,4 +82,23 @@ interface CommonMetricData { val filteredNames = sendInPings.filter { it != DEFAULT_STORAGE_NAME } return filteredNames + defaultStorageDestinations } + + /** + * Helper function to help await Jobs returned from coroutine launch executions used by metrics + * when recording data. This is to help ensure that data is updated before test functions + * check or access them. + * + * @param job Job that is to be awaited + * @param timeout Length of time in milliseconds that .join will be awaited. Defaults to + * [JOB_TIMEOUT_MS]. + * @throws TimeoutCancellationException if the function times out waiting for the join() + */ + @VisibleForTesting(otherwise = VisibleForTesting.NONE) + fun awaitJob(job: Job, timeout: Long = JOB_TIMEOUT_MS) { + runBlocking { + withTimeout(timeout) { + job.join() + } + } + } } diff --git a/components/service/glean/src/main/java/mozilla/components/service/glean/CounterMetricType.kt b/components/service/glean/src/main/java/mozilla/components/service/glean/CounterMetricType.kt index 0b2920c6a91..4147605795d 100644 --- a/components/service/glean/src/main/java/mozilla/components/service/glean/CounterMetricType.kt +++ b/components/service/glean/src/main/java/mozilla/components/service/glean/CounterMetricType.kt @@ -5,6 +5,7 @@ package mozilla.components.service.glean import android.support.annotation.VisibleForTesting +import kotlinx.coroutines.Job import kotlinx.coroutines.launch import mozilla.components.service.glean.storages.CountersStorageEngine import mozilla.components.support.base.log.logger.Logger @@ -30,6 +31,9 @@ data class CounterMetricType( private val logger = Logger("glean/CounterMetricType") + // Holds the Job returned from launch{} for awaiting purposes + private var ioTask: Job? = null + /** * Add to counter value. * @@ -48,7 +52,7 @@ data class CounterMetricType( return } - Dispatchers.API.launch { + ioTask = Dispatchers.API.launch { // Delegate storing the new counter value to the storage engine. CountersStorageEngine.record( this@CounterMetricType, @@ -58,7 +62,9 @@ data class CounterMetricType( } /** - * Tests whether a value is stored for the metric for testing purposes only + * Tests whether a value is stored for the metric for testing purposes only. This function will + * attempt to await the last task (if any) writing to the the metric's storage engine before + * returning a value. * * @param pingName represents the name of the ping to retrieve the metric for. Defaults * to the either the first value in [defaultStorageDestinations] or the first @@ -67,11 +73,14 @@ data class CounterMetricType( */ @VisibleForTesting(otherwise = VisibleForTesting.NONE) fun testHasValue(pingName: String = getStorageNames().first()): Boolean { + ioTask?.let { awaitJob(it) } + return CountersStorageEngine.getSnapshot(pingName, false)?.get(identifier) != null } /** - * Returns the stored value for testing purposes only + * Returns the stored value for testing purposes only. This function will attempt to await the + * last task (if any) writing to the the metric's storage engine before returning a value. * * @param pingName represents the name of the ping to retrieve the metric for. Defaults * to the either the first value in [defaultStorageDestinations] or the first @@ -81,6 +90,8 @@ data class CounterMetricType( */ @VisibleForTesting(otherwise = VisibleForTesting.NONE) fun testGetValue(pingName: String = getStorageNames().first()): Int { + ioTask?.let { awaitJob(it) } + return CountersStorageEngine.getSnapshot(pingName, false)!![identifier]!! } } diff --git a/components/service/glean/src/main/java/mozilla/components/service/glean/DatetimeMetricType.kt b/components/service/glean/src/main/java/mozilla/components/service/glean/DatetimeMetricType.kt index 49834e4fa05..3e277351def 100644 --- a/components/service/glean/src/main/java/mozilla/components/service/glean/DatetimeMetricType.kt +++ b/components/service/glean/src/main/java/mozilla/components/service/glean/DatetimeMetricType.kt @@ -5,6 +5,7 @@ package mozilla.components.service.glean import android.support.annotation.VisibleForTesting +import kotlinx.coroutines.Job import kotlinx.coroutines.launch import mozilla.components.service.glean.storages.DatetimesStorageEngine import mozilla.components.service.glean.utils.parseISOTimeString @@ -31,6 +32,9 @@ data class DatetimeMetricType( private val logger = Logger("glean/DatetimeMetricType") + // Holds the Job returned from launch{} for awaiting purposes + private var ioTask: Job? = null + /** * Set a datetime value, truncating it to the metric's resolution. * @@ -42,7 +46,7 @@ data class DatetimeMetricType( return } - Dispatchers.API.launch { + ioTask = Dispatchers.API.launch { // Delegate storing the datetime to the storage engine. DatetimesStorageEngine.set( this@DatetimeMetricType, @@ -67,7 +71,7 @@ data class DatetimeMetricType( return } - Dispatchers.API.launch { + ioTask = Dispatchers.API.launch { // Delegate storing the datetime to the storage engine. DatetimesStorageEngine.set( this@DatetimeMetricType, @@ -77,7 +81,9 @@ data class DatetimeMetricType( } /** - * Tests whether a value is stored for the metric for testing purposes only + * Tests whether a value is stored for the metric for testing purposes only. This function will + * attempt to await the last task (if any) writing to the the metric's storage engine before + * returning a value. * * @param pingName represents the name of the ping to retrieve the metric for. Defaults * to the either the first value in [defaultStorageDestinations] or the first @@ -86,11 +92,15 @@ data class DatetimeMetricType( */ @VisibleForTesting(otherwise = VisibleForTesting.NONE) fun testHasValue(pingName: String = getStorageNames().first()): Boolean { + ioTask?.let { awaitJob(it) } + return DatetimesStorageEngine.getSnapshot(pingName, false)?.get(identifier) != null } /** - * Returns the string representation of the stored value for testing purposes only. + * Returns the string representation of the stored value for testing purposes only. This + * function will attempt to await the last task (if any) writing to the the metric's storage + * engine before returning a value. * * @param pingName represents the name of the ping to retrieve the metric for. Defaults * to the either the first value in [defaultStorageDestinations] or the first @@ -100,11 +110,14 @@ data class DatetimeMetricType( */ @VisibleForTesting(otherwise = VisibleForTesting.NONE) fun testGetValueAsString(pingName: String = getStorageNames().first()): String { + ioTask?.let { awaitJob(it) } + return DatetimesStorageEngine.getSnapshot(pingName, false)!![identifier]!! } /** - * Returns the stored value for testing purposes only + * Returns the stored value for testing purposes only. This function will attempt to await the + * last task (if any) writing to the the metric's storage engine before returning a value. * * [Date] objects are always in the user's local timezone offset. If you * care about checking that the timezone offset was set and sent correctly, use @@ -118,6 +131,8 @@ data class DatetimeMetricType( */ @VisibleForTesting(otherwise = VisibleForTesting.NONE) fun testGetValue(pingName: String = getStorageNames().first()): Date { + ioTask?.let { awaitJob(it) } + return parseISOTimeString(DatetimesStorageEngine.getSnapshot(pingName, false)!![identifier]!!)!! } } diff --git a/components/service/glean/src/main/java/mozilla/components/service/glean/EventMetricType.kt b/components/service/glean/src/main/java/mozilla/components/service/glean/EventMetricType.kt index 1b876632011..772ba5c3b6c 100644 --- a/components/service/glean/src/main/java/mozilla/components/service/glean/EventMetricType.kt +++ b/components/service/glean/src/main/java/mozilla/components/service/glean/EventMetricType.kt @@ -5,6 +5,7 @@ package mozilla.components.service.glean import android.support.annotation.VisibleForTesting +import kotlinx.coroutines.Job import kotlinx.coroutines.launch import mozilla.components.service.glean.storages.EventsStorageEngine import mozilla.components.service.glean.storages.RecordedEventData @@ -33,6 +34,9 @@ data class EventMetricType( private val logger = Logger("glean/EventMetricType") + // Holds the Job returned from launch{} for awaiting purposes + private var ioTask: Job? = null + companion object { // Maximum length of any string value in the extra dictionary, in characters internal const val MAX_LENGTH_EXTRA_KEY_VALUE = 80 @@ -99,7 +103,7 @@ data class EventMetricType( eventKeys } - Dispatchers.API.launch { + ioTask = Dispatchers.API.launch { // Delegate storing the event to the storage engine. EventsStorageEngine.record( stores = getStorageNames(), @@ -113,7 +117,9 @@ data class EventMetricType( } /** - * Tests whether a value is stored for the metric for testing purposes only + * Tests whether a value is stored for the metric for testing purposes only. This function will + * attempt to await the last task (if any) writing to the the metric's storage engine before + * returning a value. * * @param pingName represents the name of the ping to retrieve the metric for. Defaults * to the either the first value in [defaultStorageDestinations] or the first @@ -122,6 +128,8 @@ data class EventMetricType( */ @VisibleForTesting(otherwise = VisibleForTesting.NONE) fun testHasValue(pingName: String = getStorageNames().first()): Boolean { + ioTask?.let { awaitJob(it) } + val snapshot = EventsStorageEngine.getSnapshot(pingName, false) ?: return false return snapshot.any { event -> event.identifier == identifier @@ -129,7 +137,8 @@ data class EventMetricType( } /** - * Returns the stored value for testing purposes only + * Returns the stored value for testing purposes only. This function will attempt to await the + * last task (if any) writing to the the metric's storage engine before returning a value. * * @param pingName represents the name of the ping to retrieve the metric for. Defaults * to the either the first value in [defaultStorageDestinations] or the first @@ -139,6 +148,8 @@ data class EventMetricType( */ @VisibleForTesting(otherwise = VisibleForTesting.NONE) fun testGetValue(pingName: String = getStorageNames().first()): List { + ioTask?.let { awaitJob(it) } + return EventsStorageEngine.getSnapshot(pingName, false)!!.filter { event -> event.identifier == identifier } diff --git a/components/service/glean/src/main/java/mozilla/components/service/glean/StringListMetricType.kt b/components/service/glean/src/main/java/mozilla/components/service/glean/StringListMetricType.kt index 855823e8e5d..86c94952b43 100644 --- a/components/service/glean/src/main/java/mozilla/components/service/glean/StringListMetricType.kt +++ b/components/service/glean/src/main/java/mozilla/components/service/glean/StringListMetricType.kt @@ -5,6 +5,7 @@ package mozilla.components.service.glean import android.support.annotation.VisibleForTesting +import kotlinx.coroutines.Job import kotlinx.coroutines.launch import mozilla.components.service.glean.storages.StringListsStorageEngine import mozilla.components.support.base.log.logger.Logger @@ -30,6 +31,9 @@ data class StringListMetricType( private val logger = Logger("glean/StringListMetricType") + // Holds the Job returned from launch{} for awaiting purposes + private var ioTask: Job? = null + companion object { // Maximum length of any passed value string, in characters. const val MAX_STRING_LENGTH = 50 @@ -61,7 +65,7 @@ data class StringListMetricType( it } - Dispatchers.API.launch { + ioTask = Dispatchers.API.launch { // Delegate storing the string to the storage engine. StringListsStorageEngine.add( metricData = this@StringListMetricType, @@ -93,7 +97,7 @@ data class StringListMetricType( it.take(MAX_STRING_LENGTH) } - Dispatchers.API.launch { + ioTask = Dispatchers.API.launch { // Delegate storing the string list to the storage engine. StringListsStorageEngine.set( metricData = this@StringListMetricType, @@ -103,7 +107,9 @@ data class StringListMetricType( } /** - * Tests whether a value is stored for the metric for testing purposes only + * Tests whether a value is stored for the metric for testing purposes only. This function will + * attempt to await the last task (if any) writing to the the metric's storage engine before + * returning a value. * * @param pingName represents the name of the ping to retrieve the metric for. Defaults * to the either the first value in [defaultStorageDestinations] or the first @@ -112,11 +118,14 @@ data class StringListMetricType( */ @VisibleForTesting(otherwise = VisibleForTesting.NONE) fun testHasValue(pingName: String = getStorageNames().first()): Boolean { + ioTask?.let { awaitJob(it) } + return StringListsStorageEngine.getSnapshot(pingName, false)?.get(identifier) != null } /** - * Returns the stored value for testing purposes only + * Returns the stored value for testing purposes only. This function will attempt to await the + * last task (if any) writing to the the metric's storage engine before returning a value. * * @param pingName represents the name of the ping to retrieve the metric for. Defaults * to the either the first value in [defaultStorageDestinations] or the first @@ -126,6 +135,8 @@ data class StringListMetricType( */ @VisibleForTesting(otherwise = VisibleForTesting.NONE) fun testGetValue(pingName: String = getStorageNames().first()): List { + ioTask?.let { awaitJob(it) } + return StringListsStorageEngine.getSnapshot(pingName, false)!![identifier]!! } } diff --git a/components/service/glean/src/main/java/mozilla/components/service/glean/StringMetricType.kt b/components/service/glean/src/main/java/mozilla/components/service/glean/StringMetricType.kt index b78a5b1c6ad..9acf16d11e2 100644 --- a/components/service/glean/src/main/java/mozilla/components/service/glean/StringMetricType.kt +++ b/components/service/glean/src/main/java/mozilla/components/service/glean/StringMetricType.kt @@ -5,6 +5,7 @@ package mozilla.components.service.glean import android.support.annotation.VisibleForTesting +import kotlinx.coroutines.Job import kotlinx.coroutines.launch import mozilla.components.service.glean.storages.StringsStorageEngine import mozilla.components.support.base.log.logger.Logger @@ -30,6 +31,9 @@ data class StringMetricType( private val logger = Logger("glean/StringMetricType") + // Holds the Job returned from launch{} for awaiting purposes + private var ioTask: Job? = null + companion object { // Maximum length of any passed value string, in characters. private const val MAX_LENGTH_VALUE = 50 @@ -56,7 +60,7 @@ data class StringMetricType( it } - Dispatchers.API.launch { + ioTask = Dispatchers.API.launch { // Delegate storing the string to the storage engine. StringsStorageEngine.record( this@StringMetricType, @@ -66,7 +70,9 @@ data class StringMetricType( } /** - * Tests whether a value is stored for the metric for testing purposes only + * Tests whether a value is stored for the metric for testing purposes only. This function will + * attempt to await the last task (if any) writing to the the metric's storage engine before + * returning a value. * * @param pingName represents the name of the ping to retrieve the metric for. Defaults * to the either the first value in [defaultStorageDestinations] or the first @@ -75,11 +81,14 @@ data class StringMetricType( */ @VisibleForTesting(otherwise = VisibleForTesting.NONE) fun testHasValue(pingName: String = getStorageNames().first()): Boolean { + ioTask?.let { awaitJob(it) } + return StringsStorageEngine.getSnapshot(pingName, false)?.get(identifier) != null } /** - * Returns the stored value for testing purposes only + * Returns the stored value for testing purposes only. This function will attempt to await the + * last task (if any) writing to the the metric's storage engine before returning a value. * * @param pingName represents the name of the ping to retrieve the metric for. Defaults * to the either the first value in [defaultStorageDestinations] or the first @@ -89,6 +98,8 @@ data class StringMetricType( */ @VisibleForTesting(otherwise = VisibleForTesting.NONE) fun testGetValue(pingName: String = getStorageNames().first()): String { + ioTask?.let { awaitJob(it) } + return StringsStorageEngine.getSnapshot(pingName, false)!![identifier]!! } } diff --git a/components/service/glean/src/main/java/mozilla/components/service/glean/UuidMetricType.kt b/components/service/glean/src/main/java/mozilla/components/service/glean/UuidMetricType.kt index f007f17f98c..f9fe4ff83b8 100644 --- a/components/service/glean/src/main/java/mozilla/components/service/glean/UuidMetricType.kt +++ b/components/service/glean/src/main/java/mozilla/components/service/glean/UuidMetricType.kt @@ -5,6 +5,7 @@ package mozilla.components.service.glean import android.support.annotation.VisibleForTesting +import kotlinx.coroutines.Job import kotlinx.coroutines.launch import java.util.UUID @@ -31,6 +32,9 @@ data class UuidMetricType( private val logger = Logger("glean/UuidMetricType") + // Holds the Job returned from launch{} for awaiting purposes + private var ioTask: Job? = null + /** * Generate a new UUID value and set it in the metric store. * @@ -61,7 +65,7 @@ data class UuidMetricType( return } - Dispatchers.API.launch { + ioTask = Dispatchers.API.launch { // Delegate storing the event to the storage engine. UuidsStorageEngine.record( this@UuidMetricType, @@ -71,7 +75,9 @@ data class UuidMetricType( } /** - * Tests whether a value is stored for the metric for testing purposes only + * Tests whether a value is stored for the metric for testing purposes only. This function will + * attempt to await the last task (if any) writing to the the metric's storage engine before + * returning a value. * * @param pingName represents the name of the ping to retrieve the metric for. Defaults * to the either the first value in [defaultStorageDestinations] or the first @@ -80,12 +86,14 @@ data class UuidMetricType( */ @VisibleForTesting(otherwise = VisibleForTesting.NONE) fun testHasValue(pingName: String = getStorageNames().first()): Boolean { - return UuidsStorageEngine - .getSnapshot(pingName, false)?.get(identifier) != null + ioTask?.let { awaitJob(it) } + + return UuidsStorageEngine.getSnapshot(pingName, false)?.get(identifier) != null } /** - * Returns the stored value for testing purposes only + * Returns the stored value for testing purposes only. This function will attempt to await the + * last task (if any) writing to the the metric's storage engine before returning a value. * * @param pingName represents the name of the ping to retrieve the metric for. Defaults * to the either the first value in [defaultStorageDestinations] or the first @@ -95,6 +103,8 @@ data class UuidMetricType( */ @VisibleForTesting(otherwise = VisibleForTesting.NONE) fun testGetValue(pingName: String = getStorageNames().first()): UUID { + ioTask?.let { awaitJob(it) } + return UuidsStorageEngine.getSnapshot(pingName, false)!![identifier]!! } }