Bug 1261494 - Reduce telemetry init delay to 1 second for integration testing. r=gbrown, a=test-only
authorMichael Comella <michael.l.comella@gmail.com>
Sat, 30 Jul 2016 09:44:30 -0400
changeset 340157 f0c525d1274c65dde11dd9fd916754f98d95f6b8
parent 340156 fb90485f11a590834aaa80481d61f8a4c11368e2
child 340158 f3a3809acae60a936d6b09297bbac7c02dac00b0
push id6249
push userjlund@mozilla.com
push dateMon, 01 Aug 2016 13:59:36 +0000
treeherdermozilla-beta@bad9d4f5bf7e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgbrown, test-only
bugs1261494, 24641374
milestone49.0a2
Bug 1261494 - Reduce telemetry init delay to 1 second for integration testing. r=gbrown, a=test-only My one concern is that this change could increase the amount of processing time spent on telemetry initialization, causing the runtime of the robocop test suite to increase. Checking my try push [1] against other try pushes, it doesn't seem to have made a significant difference, but the change in runtime between pushes can be large (e.g. > 5min) so it's hard to tell. [1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2017843315fe&selectedJob=24641374 MozReview-Commit-ID: LeeGgNEp74h
mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testUnifiedTelemetryClientId.java
--- a/mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testUnifiedTelemetryClientId.java
+++ b/mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testUnifiedTelemetryClientId.java
@@ -1,16 +1,17 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 package org.mozilla.gecko.tests;
 
 import static org.mozilla.gecko.tests.helpers.AssertionHelper.*;
 
+import android.util.Log;
 import org.json.JSONException;
 import org.json.JSONObject;
 import org.mozilla.gecko.GeckoProfile;
 
 import java.io.File;
 import java.io.IOException;
 import java.util.UUID;
 
@@ -18,73 +19,96 @@ public class testUnifiedTelemetryClientI
     private static final String TEST_JS = "testUnifiedTelemetryClientId.js";
 
     private static final String CLIENT_ID_PATH = "datareporting/state.json";
     private static final String FHR_DIR_PATH = "healthreport/";
     private static final String FHR_CLIENT_ID_PATH = FHR_DIR_PATH + "state.json";
 
     private GeckoProfile profile;
     private File profileDir;
+    private File[] filesToDeleteOnReset;
 
     public void setUp() throws Exception {
         super.setUp();
         profile = getTestProfile();
         profileDir = profile.getDir(); // Assumes getDir is tested.
-
-        // In local testing, it's possible to ^C out of the harness and not have tearDown called,
-        // hence reset. We can't clear the cache because Gecko is not running yet.
-        resetTest(false);
+        filesToDeleteOnReset = new File[] {
+                getClientIdFile(),
+                getFHRClientIdFile(),
+                getFHRClientIdParentDir(),
+        };
     }
 
     public void tearDown() throws Exception {
         // Don't clear cache because who knows what state Gecko is in.
-        resetTest(false);
+        deleteClientIDFiles();
         super.tearDown();
     }
 
-    private void resetTest(final boolean resetJSCache) {
-        if (resetJSCache) {
-            resetJSCache();
+    private void deleteClientIDFiles() {
+        Log.d(LOGTAG, "deleteClientIDFiles: begin");
+
+        for (final File file : filesToDeleteOnReset) {
+            file.delete(); // can't check return value because the file may not exist before deletion.
+            fAssertFalse("Deleted file in reset does not exist", file.exists()); // sanity check.
         }
-        getClientIdFile().delete();
-        getFHRClientIdFile().delete();
-        getFHRClientIdParentDir().delete();
+
+        Log.d(LOGTAG, "deleteClientIDFiles: end");
     }
 
-    // TODO: If the intent service runs in the background, it could break this test. The service is disabled
-    // on non-official builds (e.g. this one) but that may not be the case on TBPL.
     public void testUnifiedTelemetryClientId() throws Exception {
         blockForReadyAndLoadJS(TEST_JS);
-        resetJSCache(); // Must be called after Gecko is loaded.
         fAssertTrue("Profile directory exists", profileDir.exists());
 
+        // Important note: we cannot stop Gecko from running while we run this test and
+        // Gecko is capable of creating client ID files while we run this test. However,
+        // ClientID.jsm will not touch modify the client ID files on disk if its client
+        // ID cache is filled. As such, we prevent it from touching the disk by intentionally
+        // priming the cache & deleting the files it added now, and resetting the cache at the
+        // latest possible moment before we attempt to test the client ID file.
+        //
+        // This is fragile because it relies on the ClientID cache's implementation, however,
+        // some alternatives (e.g. changing file system permissions, file locking) are worse
+        // because they can fire error handling code, which is not currently under test.
+        //
+        // First, we delete the test files - we don't want the cache prime to fail which could happen if
+        // these files are around & corrupted from a previous test/install. Then we prime the cache,
+        // and delete the files the cache priming added, so the tests are ready to add their own version
+        // of these files.
+        deleteClientIDFiles();
+        primeJsClientIdCache();
+        deleteClientIDFiles();
+
         // TODO: If these tests weren't so expensive to run in automation,
         // this should be two separate tests to avoid storing state between tests.
-        testJavaCreatesClientId();
-        resetTest(true);
-        testJsCreatesClientId();
-        resetTest(true);
-        testJavaMigratesFromHealthReport();
-        resetTest(true);
-        testJsMigratesFromHealthReport();
+        testJavaCreatesClientId(); // leaves cache filled.
+        deleteClientIDFiles();
+        testJsCreatesClientId(); // leaves cache filled.
+        deleteClientIDFiles();
+        testJavaMigratesFromHealthReport(); // leaves cache filled.
+        deleteClientIDFiles();
+        testJsMigratesFromHealthReport(); // leaves cache filled.
 
         getJS().syncCall("endTest");
     }
 
     /**
      * Scenario: Java creates client ID:
      *   * Fennec starts on fresh profile
      *   * Java code creates the client ID in datareporting/state.json
      *   * Js accesses client ID from the same file
      *   * Assert the client IDs are the same
      */
     private void testJavaCreatesClientId() throws Exception {
+        Log.d(LOGTAG, "testJavaCreatesClientId: start");
+
         fAssertFalse("Client id file does not exist yet", getClientIdFile().exists());
 
         final String clientIdFromJava = getClientIdFromJava();
+        resetJSCache();
         final String clientIdFromJS = getClientIdFromJS();
         fAssertEquals("Client ID from Java equals ID from JS", clientIdFromJava, clientIdFromJS);
 
         final String clientIdFromJavaAgain = getClientIdFromJava();
         final String clientIdFromJSCache = getClientIdFromJS();
         resetJSCache();
         final String clientIdFromJSFileAgain = getClientIdFromJS();
         fAssertEquals("Same client ID retrieved from Java", clientIdFromJava, clientIdFromJavaAgain);
@@ -95,18 +119,21 @@ public class testUnifiedTelemetryClientI
     /**
      * Scenario: JS creates client ID
      *   * Fennec starts on a fresh profile
      *   * Js creates the client ID in datareporting/state.json
      *   * Java access the client ID from the same file
      *   * Assert the client IDs are the same
      */
     private void testJsCreatesClientId() throws Exception {
+        Log.d(LOGTAG, "testJsCreatesClientId: start");
+
         fAssertFalse("Client id file does not exist yet", getClientIdFile().exists());
 
+        resetJSCache();
         final String clientIdFromJS = getClientIdFromJS();
         final String clientIdFromJava = getClientIdFromJava();
         fAssertEquals("Client ID from JS equals ID from Java", clientIdFromJS, clientIdFromJava);
 
         final String clientIdFromJSCache = getClientIdFromJS();
         final String clientIdFromJavaAgain = getClientIdFromJava();
         resetJSCache();
         final String clientIdFromJSFileAgain = getClientIdFromJS();
@@ -119,24 +146,27 @@ public class testUnifiedTelemetryClientI
      * Scenario: Java migrates client ID from FHR client ID file.
      *   * FHR file already exists.
      *   * Fennec starts on fresh profile
      *   * Java code merges client ID to datareporting/state.json from healthreport/state.json
      *   * Js accesses client ID from the same file
      *   * Assert the client IDs are the same
      */
     private void testJavaMigratesFromHealthReport() throws Exception {
+        Log.d(LOGTAG, "testJavaMigratesFromHealthReport: start");
+
         fAssertFalse("Client id file does not exist yet", getClientIdFile().exists());
         fAssertFalse("Health report file does not exist yet", getFHRClientIdFile().exists());
 
         final String expectedClientId = UUID.randomUUID().toString();
         createFHRClientIdFile(expectedClientId);
 
         final String clientIdFromJava = getClientIdFromJava();
         fAssertEquals("Health report client ID merged by Java", expectedClientId, clientIdFromJava);
+        resetJSCache();
         final String clientIdFromJS = getClientIdFromJS();
         fAssertEquals("Merged client ID read by JS", expectedClientId, clientIdFromJS);
 
         final String clientIdFromJavaAgain = getClientIdFromJava();
         final String clientIdFromJSCache = getClientIdFromJS();
         resetJSCache();
         final String clientIdFromJSFileAgain = getClientIdFromJS();
         fAssertEquals("Same client ID retrieved from Java", expectedClientId, clientIdFromJavaAgain);
@@ -148,53 +178,66 @@ public class testUnifiedTelemetryClientI
      * Scenario: JS merges client ID from FHR client ID file.
      *   * FHR file already exists.
      *   * Fennec starts on a fresh profile
      *   * Js merges the client ID to datareporting/state.json from healthreport/state.json
      *   * Java access the client ID from the same file
      *   * Assert the client IDs are the same
      */
     private void testJsMigratesFromHealthReport() throws Exception {
+        Log.d(LOGTAG, "testJsMigratesFromHealthReport: start");
+
         fAssertFalse("Client id file does not exist yet", getClientIdFile().exists());
         fAssertFalse("Health report file does not exist yet", getFHRClientIdFile().exists());
 
         final String expectedClientId = UUID.randomUUID().toString();
         createFHRClientIdFile(expectedClientId);
 
+        resetJSCache();
         final String clientIdFromJS = getClientIdFromJS();
         fAssertEquals("Health report client ID merged by JS", expectedClientId, clientIdFromJS);
         final String clientIdFromJava = getClientIdFromJava();
         fAssertEquals("Merged client ID read by Java", expectedClientId, clientIdFromJava);
 
         final String clientIdFromJavaAgain = getClientIdFromJava();
         final String clientIdFromJSCache = getClientIdFromJS();
         resetJSCache();
         final String clientIdFromJSFileAgain = getClientIdFromJS();
         fAssertEquals("Same client ID retrieved from Java", expectedClientId, clientIdFromJavaAgain);
         fAssertEquals("Same client ID retrieved from JS cache", expectedClientId, clientIdFromJSCache);
         fAssertEquals("Same client ID retrieved from JS file", expectedClientId, clientIdFromJSFileAgain);
-
     }
 
     private String getClientIdFromJava() throws IOException {
         // This assumes implementation details: it assumes the client ID
         // file is created when Java attempts to retrieve it if it does not exist.
         final String clientId = profile.getClientId();
         fAssertNotNull("Returned client ID is not null", clientId);
         fAssertTrue("Client ID file exists after getClientId call", getClientIdFile().exists());
         return clientId;
     }
 
     private String getClientIdFromJS() {
         return getBlockingFromJsString("clientId");
     }
 
     /**
+     * Must be called after Gecko is loaded.
+     */
+    private void primeJsClientIdCache() {
+        // Not the cleanest way, but it works.
+        getClientIdFromJS();
+    }
+
+    /**
      * Resets the client ID cache in ClientID.jsm. This method *must* be called after
      * Gecko is loaded or else this method will hang.
+     *
+     * Note: we do this for very specific reasons - see the comment in the test method
+     * ({@link #testUnifiedTelemetryClientId()}) for more.
      */
     private void resetJSCache() {
         // HACK: the backing JS method is a promise with no return value. Rather than writing a method
         // to handle this (for time reasons), I call the get String method and don't access the return value.
         getBlockingFromJsString("reset");
     }
 
     private File getClientIdFile() {