Merge pull request #65 from mozilla/1551892-string-metric-test-api

Bug 1551892: Implement the testing API for the StringMetricType and enable the related tests

glean-core/android/src/main/java/mozilla/telemetry/glean/private/StringMetricType.kt

@@ -9,6 +9,7 @@ import com.sun.jna.StringArray
 import mozilla.telemetry.glean.Glean
 import mozilla.telemetry.glean.rust.LibGleanFFI
 import mozilla.telemetry.glean.rust.toByte
+import mozilla.telemetry.glean.rust.getAndConsumeRustString
 
 import mozilla.telemetry.glean.Dispatchers
 // import mozilla.components.service.glean.storages.StringsStorageEngine
@@ -84,11 +85,10 @@ class StringMetricType(
     @VisibleForTesting(otherwise = VisibleForTesting.NONE)
     fun testHasValue(pingName: String = sendInPings.first()): Boolean {
         /*@Suppress("EXPERIMENTAL_API_USAGE")
-        Dispatchers.API.assertInTestingMode()
+        Dispatchers.API.assertInTestingMode()*/
 
-        return StringsStorageEngine.getSnapshot(pingName, false)?.get(identifier) != null*/
-        assert(false, { "Testing API not implementated for StringMetricType" })
-        return false
+        val res = LibGleanFFI.INSTANCE.glean_string_test_has_value(Glean.handle, this.handle, pingName)
+        return res != 0.toByte()
     }
 
     /**
@@ -104,10 +104,12 @@ class StringMetricType(
     @VisibleForTesting(otherwise = VisibleForTesting.NONE)
     fun testGetValue(pingName: String = sendInPings.first()): String {
         /*@Suppress("EXPERIMENTAL_API_USAGE")
-        Dispatchers.API.assertInTestingMode()
+        Dispatchers.API.assertInTestingMode()*/
 
-        return StringsStorageEngine.getSnapshot(pingName, false)!![identifier]!!*/
-        assert(false, { "Testing API not implementated for StringMetricType" })
-        return "asd"
+        if (!testHasValue(pingName)) {
+            throw NullPointerException()
+        }
+        val ptr = LibGleanFFI.INSTANCE.glean_string_test_get_value(Glean.handle, this.handle, pingName)!!
+        return ptr.getAndConsumeRustString()
     }
 }

glean-core/android/src/main/java/mozilla/telemetry/glean/rust/LibGleanFFI.kt

@@ -15,6 +15,28 @@ import java.lang.reflect.Proxy
 // Turn a boolean into its Byte (u8) representation
 internal fun Boolean.toByte(): Byte = if (this) 1 else 0
 
+/**
+ * Helper to read a null terminated String out of the Pointer and free it.
+ *
+ * Important: Do not use this pointer after this! For anything!
+ */
+internal fun Pointer.getAndConsumeRustString(): String {
+    try {
+        return this.getRustString()
+    } finally {
+        LibGleanFFI.INSTANCE.glean_str_free(this)
+    }
+}
+
+/**
+ * Helper to read a null terminated string out of the pointer.
+ *
+ * Important: doesn't free the pointer, use [getAndConsumeRustString] for that!
+ */
+internal fun Pointer.getRustString(): String {
+    return this.getString(0, "utf8")
+}
+
 @Suppress("TooManyFunctions")
 internal interface LibGleanFFI : Library {
     companion object {
@@ -77,6 +99,10 @@ internal interface LibGleanFFI : Library {
         disabled: Byte
     ): Long
 
+    fun glean_string_test_get_value(glean_handle: Long, metric_id: Long, storage_name: String): Pointer?
+
+    fun glean_string_test_has_value(glean_handle: Long, metric_id: Long, storage_name: String): Byte
+
     fun glean_ping_collect(glean_handle: Long, ping_name: String): Pointer?
 
     fun glean_send_ping(glean_handle: Long, ping_name: String)

glean-core/android/src/main/java/mozilla/telemetry/glean/rust/RustError.kt

@@ -67,25 +67,3 @@ internal open class RustError : Structure() {
         return Arrays.asList("code", "message")
     }
 }
-
-/**
- * Helper to read a null terminated String out of the Pointer and free it.
- *
- * Important: Do not use this pointer after this! For anything!
- */
-internal fun Pointer.getAndConsumeRustString(): String {
-    try {
-        return this.getRustString()
-    } finally {
-        LibGleanFFI.INSTANCE.glean_str_free(this)
-    }
-}
-
-/**
- * Helper to read a null terminated string out of the pointer.
- *
- * Important: doesn't free the pointer, use [getAndConsumeRustString] for that!
- */
-internal fun Pointer.getRustString(): String {
-    return this.getString(0, "utf8")
-}

glean-core/android/src/test/java/mozilla/telemetry/glean/private/StringMetricTypeTest.kt

@@ -17,7 +17,6 @@ import org.junit.Assert.assertEquals
 import org.junit.Assert.assertFalse
 import org.junit.Assert.assertTrue
 import org.junit.Before
-import org.junit.Ignore
 import org.junit.Test
 import org.junit.runner.RunWith
 import org.robolectric.RobolectricTestRunner
@@ -33,7 +32,6 @@ class StringMetricTypeTest {
         resetGlean()
     }
 
-    @Ignore("Ignoring the test as the testing API for strings is not implemented")
     @Test
     fun `The API saves to its storage engine`() {
         // Define a 'stringMetric' string metric, which will be stored in "store1"
@@ -58,27 +56,6 @@ class StringMetricTypeTest {
         assertEquals("overriddenValue", stringMetric.testGetValue())
     }
 
-    @Ignore("Ignoring the test as the testing API for strings is not implemented")
-    @Test
-    fun `strings with no lifetime must not record data`() {
-        // Define a 'stringMetric' string metric, which will be stored in
-        // "store1". It's disabled so it should not record anything.
-        val stringMetric = StringMetricType(
-            disabled = true,
-            category = "telemetry",
-            lifetime = Lifetime.Ping,
-            name = "stringMetric",
-            sendInPings = listOf("store1")
-        )
-
-        // Attempt to store the string.
-        stringMetric.set("value")
-        // Check that nothing was recorded.
-        assertFalse("Strings must not be recorded if they have no lifetime",
-            stringMetric.testHasValue())
-    }
-
-    @Ignore("Ignoring the test as the testing API for strings is not implemented")
     @Test
     fun `disabled strings must not record data`() {
         // Define a 'stringMetric' string metric, which will be stored in "store1". It's disabled
@@ -98,7 +75,6 @@ class StringMetricTypeTest {
             stringMetric.testHasValue())
     }
 
-    @Ignore("Ignoring the test as the testing API for strings is not implemented")
     @Test(expected = NullPointerException::class)
     fun `testGetValue() throws NullPointerException if nothing is stored`() {
         val stringMetric = StringMetricType(
@@ -111,7 +87,6 @@ class StringMetricTypeTest {
         stringMetric.testGetValue()
     }
 
-    @Ignore("Ignoring the test as the testing API for strings is not implemented")
     @Test
     fun `The API saves to secondary pings`() {
         // Define a 'stringMetric' string metric, which will be stored in "store1" and "store2"

glean-core/ffi/examples/glean.h

@@ -103,6 +103,10 @@ void glean_set_upload_enabled(uint64_t glean_handle, uint8_t flag);
 
 void glean_string_set(uint64_t glean_handle, uint64_t metric_id, FfiStr value);
 
+char *glean_string_test_get_value(uint64_t glean_handle, uint64_t metric_id, FfiStr storage_name);
+
+uint8_t glean_string_test_has_value(uint64_t glean_handle, uint64_t metric_id, FfiStr storage_name);
+
 void glean_destroy_glean(uint64_t handle, ExternError *error);
 void glean_destroy_boolean_metric(uint64_t handle, ExternError *error);
 void glean_destroy_string_metric(uint64_t handle, ExternError *error);

glean-core/ffi/src/lib.rs

@@ -228,6 +228,35 @@ pub extern "C" fn glean_string_set(glean_handle: u64, metric_id: u64, value: Ffi
     })
 }
 
+#[no_mangle]
+pub extern "C" fn glean_string_test_has_value(
+    glean_handle: u64,
+    metric_id: u64,
+    storage_name: FfiStr,
+) -> u8 {
+    GLEAN.call_infallible(glean_handle, |glean| {
+        STRING_METRICS.call_infallible(metric_id, |metric| {
+            metric
+                .test_get_value(glean, storage_name.as_str())
+                .is_some()
+        })
+    })
+}
+
+#[no_mangle]
+pub extern "C" fn glean_string_test_get_value(
+    glean_handle: u64,
+    metric_id: u64,
+    storage_name: FfiStr,
+) -> *mut c_char {
+    GLEAN.call_infallible(glean_handle, |glean| {
+        let res: glean_core::Result<String> = STRING_METRICS.get_u64(metric_id, |metric| {
+            Ok(metric.test_get_value(glean, storage_name.as_str()).unwrap())
+        });
+        res.unwrap()
+    })
+}
+
 #[no_mangle]
 pub extern "C" fn glean_ping_collect(glean_handle: u64, ping_name: FfiStr) -> *mut c_char {
     GLEAN.call_infallible(glean_handle, |glean| {

glean-core/src/error.rs

@@ -4,7 +4,7 @@ use std::result;
 
 use failure::{self, Backtrace, Context, Fail};
 
-use ffi_support::ExternError;
+use ffi_support::{handle_map::HandleError, ExternError};
 
 use rkv::error::StoreError;
 
@@ -27,6 +27,10 @@ pub type Result<T> = result::Result<T, Error>;
 /// [`Error`]: std.struct.Error.html
 #[derive(Debug, Fail)]
 pub enum ErrorKind {
+    /// FFI-Support error
+    #[fail(display = "Invalid handle")]
+    Handle(HandleError),
+
     /// IO error
     #[fail(display = "An I/O error occured.")]
     IoError(io::Error),
@@ -82,6 +86,14 @@ impl From<Context<ErrorKind>> for Error {
     }
 }
 
+impl From<HandleError> for Error {
+    fn from(error: HandleError) -> Error {
+        Error {
+            inner: Context::new(ErrorKind::Handle(error)),
+        }
+    }
+}
+
 impl From<io::Error> for Error {
     fn from(error: io::Error) -> Error {
         Error {