Skip to content

Commit

Permalink
Create a way for glean test API functions to await async IO operations
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@travis79
travis79 committed Feb 21, 2019
1 parent 786e8a3 commit 7fb2660
Show file tree
Hide file tree
Showing 8 changed files with 132 additions and 26 deletions.
18 changes: 15 additions & 3 deletions components/service/glean/src/main/java/mozilla/components/service/glean/BooleanMetricType.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
*
Expand All @@ -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,
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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]!!
}
}
25 changes: 25 additions & 0 deletions components/service/glean/src/main/java/mozilla/components/service/glean/CommonMetricData.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

/**
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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()
}
}
}
}
17 changes: 14 additions & 3 deletions components/service/glean/src/main/java/mozilla/components/service/glean/CounterMetricType.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
*
Expand All @@ -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,
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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]!!
}
}
25 changes: 20 additions & 5 deletions ...onents/service/glean/src/main/java/mozilla/components/service/glean/DatetimeMetricType.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
*
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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]!!)!!
}
}
17 changes: 14 additions & 3 deletions components/service/glean/src/main/java/mozilla/components/service/glean/EventMetricType.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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(),
Expand All @@ -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
Expand All @@ -122,14 +128,17 @@ 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
}
}

/**
* 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
Expand All @@ -139,6 +148,8 @@ data class EventMetricType(
*/
@VisibleForTesting(otherwise = VisibleForTesting.NONE)
fun testGetValue(pingName: String = getStorageNames().first()): List<RecordedEventData> {
ioTask?.let { awaitJob(it) }

return EventsStorageEngine.getSnapshot(pingName, false)!!.filter { event ->
event.identifier == identifier
}
Expand Down
19 changes: 15 additions & 4 deletions ...ents/service/glean/src/main/java/mozilla/components/service/glean/StringListMetricType.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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
Expand All @@ -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
Expand All @@ -126,6 +135,8 @@ data class StringListMetricType(
*/
@VisibleForTesting(otherwise = VisibleForTesting.NONE)
fun testGetValue(pingName: String = getStorageNames().first()): List<String> {
ioTask?.let { awaitJob(it) }

return StringListsStorageEngine.getSnapshot(pingName, false)!![identifier]!!
}
}
Loading

0 comments on commit 7fb2660

Please sign in to comment.