Bug 1506816 - Check for action errors in Normandy tests, and fix revealed problems. r=Gijs

Differential Revision: https://phabricator.services.mozilla.com/D12156

--HG--
extra : moz-landing-system : lando

toolkit/components/normandy/actions/BaseAction.jsm

@@ -22,6 +22,7 @@ class BaseAction {
   constructor() {
     this.state = BaseAction.STATE_PREPARING;
     this.log = LogManager.getLogger(`action.${this.name}`);
+    this.lastError = null;
 
     try {
       this._preExecution();
@@ -30,9 +31,14 @@ class BaseAction {
         this.state = BaseAction.STATE_READY;
       }
     } catch (err) {
-      err.message = `Could not initialize action ${this.name}: ${err.message}`;
-      Cu.reportError(err);
-      this.fail(Uptake.ACTION_PRE_EXECUTION_ERROR);
+      // Sometimes err.message is editable. If it is, add helpful details.
+      // Otherwise log the helpful details and move on.
+      try {
+        err.message = `Could not initialize action ${this.name}: ${err.message}`;
+      } catch (_e) {
+        this.log.error(`Could not initialize action ${this.name}, error follows.`);
+      }
+      this.fail(err);
     }
   }
 
@@ -51,7 +57,7 @@ class BaseAction {
     this.state = BaseAction.STATE_DISABLED;
   }
 
-  fail() {
+  fail(err) {
     switch (this.state) {
       case BaseAction.STATE_PREPARING: {
         Uptake.reportAction(this.name, Uptake.ACTION_PRE_EXECUTION_ERROR);
@@ -62,6 +68,8 @@ class BaseAction {
       }
     }
     this.state = BaseAction.STATE_FAILED;
+    this.lastError = err;
+    Cu.reportError(err);
   }
 
   // Gets the name of the action. Does not necessarily match the
@@ -141,7 +149,7 @@ class BaseAction {
           status = Uptake.ACTION_SUCCESS;
         } catch (err) {
           status = Uptake.ACTION_POST_EXECUTION_ERROR;
-            // Sometimes Error.message can be updated in place. This gives better messages when debugging errors.
+          // Sometimes Error.message can be updated in place. This gives better messages when debugging errors.
           try {
             err.message = `Could not run postExecution hook for ${this.name}: ${err.message}`;
           } catch (err) {
@@ -149,6 +157,7 @@ class BaseAction {
             this.log.debug(`Could not run postExecution hook for ${this.name}`);
           }
 
+          this.lastError = err;
           Cu.reportError(err);
         }
         break;

toolkit/components/normandy/actions/PreferenceRollbackAction.jsm

@@ -55,6 +55,5 @@ class PreferenceRollbackAction extends BaseAction {
 
   async _finalize() {
     await PreferenceRollouts.saveStartupPrefs();
-    await PreferenceRollouts.closeDB();
   }
 }

toolkit/components/normandy/actions/PreferenceRolloutAction.jsm

@@ -170,6 +170,5 @@ class PreferenceRolloutAction extends BaseAction {
 
   async _finalize() {
     await PreferenceRollouts.saveStartupPrefs();
-    await PreferenceRollouts.closeDB();
   }
 }

toolkit/components/normandy/test/browser/browser_BaseAction.js

@@ -27,21 +27,23 @@ class NoopAction extends BaseAction {
   }
 }
 
+NoopAction._errorToThrow = new Error("test error");
+
 class FailPreExecutionAction extends NoopAction {
   _preExecution() {
-    throw new Error("Test error");
+    throw NoopAction._errorToThrow;
   }
 }
 
 class FailRunAction extends NoopAction {
   _run(recipe) {
-    throw new Error("Test error");
+    throw NoopAction._errorToThrow;
   }
 }
 
 class FailFinalizeAction extends NoopAction {
   _finalize() {
-    throw new Error("Test error");
+    throw NoopAction._errorToThrow;
   }
 }
 
@@ -128,6 +130,7 @@ decorate_task(
     const recipe = recipeFactory();
     const action = new FailPreExecutionAction();
     is(action.state, FailPreExecutionAction.STATE_FAILED, "Action should fail during pre-execution fail");
+    is(action.lastError, NoopAction._errorToThrow, "The thrown error should be stored in lastError");
 
     // Should not throw, even though the action is in a disabled state.
     await action.runRecipe(recipe);
@@ -136,6 +139,7 @@ decorate_task(
     // Should not throw, even though the action is in a disabled state.
     await action.finalize();
     is(action.state, FailPreExecutionAction.STATE_FINALIZED, "Action should be finalized");
+    is(action.lastError, NoopAction._errorToThrow, "lastError should not have changed");
 
     is(action._testRunFlag, false, "_run should not have been called");
     is(action._testFinalizeFlag, false, "_finalize should not have been called");

toolkit/components/normandy/test/browser/browser_actions_AddonStudyAction.js

@@ -56,6 +56,7 @@ decorate_task(
     const action = new AddonStudyAction();
     const enrollSpy = sinon.spy(action, "enroll");
     await action.runRecipe(recipe);
+    is(action.lastError, null, "lastError should be null");
     Assert.deepEqual(enrollSpy.args, [], "enroll should not be called");
     Assert.deepEqual(sendEventStub.args, [], "no events should be sent");
   },
@@ -95,6 +96,7 @@ decorate_task(
     const recipe = addonStudyRecipeFactory({ arguments: { name: "conflicting", addonUrl } });
     const action = new AddonStudyAction();
     await action.runRecipe(recipe);
+    is(action.lastError, null, "lastError should be null");
 
     const addon = await AddonManager.getAddonByID(FIXTURE_ADDON_ID);
     is(addon.version, "0.1", "The installed add-on should not be replaced");
@@ -124,6 +126,7 @@ decorate_task(
     const recipe = addonStudyRecipeFactory({ arguments: { name: "success", addonUrl } });
     const action = new AddonStudyAction();
     await action.runRecipe(recipe);
+    is(action.lastError, null, "lastError should be null");
 
     await webExtStartupPromise;
     addon = await AddonManager.getAddonByID(FIXTURE_ADDON_ID);
@@ -270,6 +273,7 @@ decorate_task(
     await action.runRecipe(recipe);
     await action.finalize();
     is(action.state, AddonStudyAction.STATE_FINALIZED, "the action should be finalized");
+    is(action.lastError, null, "lastError should be null");
     Assert.deepEqual(enrollSpy.args, [], "enroll should not be called");
     Assert.deepEqual(sendEventStub.args, [], "no events should be sent");
   },
@@ -299,6 +303,7 @@ decorate_task(
     const action = new AddonStudyAction();
     const unenrollSpy = sinon.stub(action, "unenroll");
     await action.finalize();
+    is(action.lastError, null, "lastError should be null");
     Assert.deepEqual(unenrollSpy.args, [[study.recipeId, "recipe-not-seen"]], "unenroll should be called");
   },
 );

toolkit/components/normandy/test/browser/browser_actions_ConsoleLogAction.js

@@ -10,6 +10,7 @@ add_task(async function logging_works() {
   try {
     const recipe = {id: 1, arguments: {message: "Hello, world!"}};
     await action.runRecipe(recipe);
+    is(action.lastError, null, "lastError should be null");
     Assert.deepEqual(infoStub.args, ["Hello, world!"], "the message should be logged");
   } finally {
     infoStub.restore();
@@ -28,6 +29,7 @@ decorate_task(
       // message is required
       let recipe = {id: 1, arguments: {}};
       await action.runRecipe(recipe);
+      is(action.lastError, null, "lastError should be null");
       Assert.deepEqual(infoStub.args, [], "no message should be logged");
       Assert.deepEqual(reportRecipeStub.args, [[recipe.id, Uptake.RECIPE_EXECUTION_ERROR]]);
 
@@ -36,6 +38,7 @@ decorate_task(
       // message must be a string
       recipe = {id: 1, arguments: {message: 1}};
       await action.runRecipe(recipe);
+      is(action.lastError, null, "lastError should be null");
       Assert.deepEqual(infoStub.args, [], "no message should be logged");
       Assert.deepEqual(reportRecipeStub.args, [[recipe.id, Uptake.RECIPE_EXECUTION_ERROR]]);
     } finally {

toolkit/components/normandy/test/browser/browser_actions_PreferenceRollbackAction.js

@@ -33,6 +33,7 @@ decorate_task(
     const action = new PreferenceRollbackAction();
     await action.runRecipe(recipe);
     await action.finalize();
+    is(action.lastError, null, "lastError should be null");
 
     // rollout prefs are reset
     is(Services.prefs.getIntPref("test.pref1"), 1, "integer pref should be rolled back");
@@ -91,6 +92,7 @@ decorate_task(
     const action = new PreferenceRollbackAction();
     await action.runRecipe(recipe);
     await action.finalize();
+    is(action.lastError, null, "lastError should be null");
 
     is(Services.prefs.getIntPref("test.pref"), 1, "pref should not change");
     is(Services.prefs.getPrefType("app.normandy.startupRolloutPrefs.test.pref"), Services.prefs.PREF_INVALID, "no startup pref should be added");
@@ -128,6 +130,7 @@ decorate_task(
     const action = new PreferenceRollbackAction();
     await action.runRecipe(recipe);
     await action.finalize();
+    is(action.lastError, null, "lastError should be null");
 
     Assert.deepEqual(sendEventStub.args, [], "an unenrollFailure event should not be sent");
     Assert.deepEqual(
@@ -157,6 +160,7 @@ decorate_task(
     const action = new PreferenceRollbackAction();
     await action.runRecipe(recipe);
     await action.finalize();
+    is(action.lastError, null, "lastError should be null");
 
     is(Services.prefs.getIntPref("test.pref"), 1, "pref shouldn't change");
     is(Services.prefs.getPrefType("app.normandy.startupRolloutPrefs.test.pref"), Services.prefs.PREF_INVALID, "startup pref should not be set");
@@ -193,6 +197,7 @@ decorate_task(
     const action = new PreferenceRollbackAction();
     await action.runRecipe(recipe);
     await action.finalize();
+    is(action.lastError, null, "lastError should be null");
 
     is(Services.prefs.getDefaultBranch("").getCharPref("test.pref"), "builtin value", "default branch should be reset");
     is(Services.prefs.getCharPref("test.pref"), "user value", "user branch should remain the same");

toolkit/components/normandy/test/browser/browser_actions_PreferenceRolloutAction.js

@@ -28,6 +28,7 @@ decorate_task(
     const action = new PreferenceRolloutAction();
     await action.runRecipe(recipe);
     await action.finalize();
+    is(action.lastError, null, "lastError should be null");
 
     // rollout prefs are set
     is(Services.prefs.getIntPref("test.pref1"), 1, "integer pref should be set");
@@ -92,6 +93,7 @@ decorate_task(
     let action = new PreferenceRolloutAction();
     await action.runRecipe(recipe);
     await action.finalize();
+    is(action.lastError, null, "lastError should be null");
 
     const defaultBranch = Services.prefs.getDefaultBranch("");
     is(defaultBranch.getIntPref("test.pref1"), 1, "pref1 should be set");
@@ -110,6 +112,7 @@ decorate_task(
     action = new PreferenceRolloutAction();
     await action.runRecipe(recipe);
     await action.finalize();
+    is(action.lastError, null, "lastError should be null");
 
     /* Todo because of bug 1502410 and bug 1505941 */
     todo_is(Services.prefs.getPrefType("test.pref1"), Services.prefs.PREF_INVALID, "pref1 should be removed");
@@ -185,6 +188,7 @@ decorate_task(
     const action = new PreferenceRolloutAction();
     await action.runRecipe(recipe);
     await action.finalize();
+    is(action.lastError, null, "lastError should be null");
 
     is(Services.prefs.getIntPref("test.pref"), 2, "pref should be updated");
     is(Services.prefs.getIntPref("app.normandy.startupRolloutPrefs.test.pref"), 2, "startup pref should be set");
@@ -245,11 +249,13 @@ decorate_task(
     await action.runRecipe(recipe1);
     await action.runRecipe(recipe2);
     await action.finalize();
+    is(action.lastError, null, "lastError should be null");
 
     // running recipe2 in a separate session shouldn't change things
     action = new PreferenceRolloutAction();
     await action.runRecipe(recipe2);
     await action.finalize();
+    is(action.lastError, null, "lastError should be null");
 
     is(Services.prefs.getIntPref("test.pref1"), 1, "pref1 is set to recipe1's value");
     is(Services.prefs.getIntPref("test.pref2"), 1, "pref2 is set to recipe1's value");
@@ -306,6 +312,7 @@ decorate_task(
     const action = new PreferenceRolloutAction();
     await action.runRecipe(recipe);
     await action.finalize();
+    is(action.lastError, null, "lastError should be null");
 
     is(Services.prefs.getCharPref("test.pref"), "not an int", "the pref should not be modified");
     is(Services.prefs.getPrefType("app.normandy.startupRolloutPrefs.test.pref"), Services.prefs.PREF_INVALID, "startup pref is not set");
@@ -339,6 +346,7 @@ decorate_task(
     const action = new PreferenceRolloutAction();
     await action.runRecipe(recipe);
     await action.finalize();
+    is(action.lastError, null, "lastError should be null");
 
     is(Services.prefs.getCharPref("test.pref"), "user value", "user branch value should be preserved");
     is(Services.prefs.getDefaultBranch("").getCharPref("test.pref"), "rollout value", "default branch value should change");
@@ -377,6 +385,7 @@ decorate_task(
     const action = new PreferenceRolloutAction();
     await action.runRecipe(recipe);
     await action.finalize();
+    is(action.lastError, null, "lastError should be null");
 
     is(Services.prefs.getIntPref("test.pref"), 2, "original user branch value still visible");
     is(Services.prefs.getDefaultBranch("").getIntPref("test.pref"), 1, "default branch was set");
@@ -405,11 +414,13 @@ decorate_task(
     let action = new PreferenceRolloutAction();
     await action.runRecipe(recipe);
     await action.finalize();
+    is(action.lastError, null, "lastError should be null");
 
     // run a second time
     action = new PreferenceRolloutAction();
     await action.runRecipe(recipe);
     await action.finalize();
+    is(action.lastError, null, "lastError should be null");
 
     Assert.deepEqual(
       sendEventStub.args,
@@ -448,6 +459,7 @@ decorate_task(
     const action = new PreferenceRolloutAction();
     await action.runRecipe(recipe);
     await action.finalize();
+    is(action.lastError, null, "lastError should be null");
 
     is(Services.prefs.getIntPref("test.pref"), 1, "pref should not change");