Bug 1506816 - Check for action errors in Normandy tests, and fix revealed problems. r=Gijs
authorMichael Cooper <mcooper@mozilla.com>
Mon, 19 Nov 2018 18:23:54 +0000
changeset 447020 d765759658a28e9be580381abeca165cb661a0bd
parent 447019 23f1f900d990a4b0d3156959a02ab18ff7cf8f39
child 447021 7614f586cc31d36590f4c539209e7168ba7c169a
push id35065
push userrmaries@mozilla.com
push dateMon, 19 Nov 2018 21:56:32 +0000
treeherdermozilla-central@bd4cebdbed4b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs
bugs1506816
milestone65.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1506816 - Check for action errors in Normandy tests, and fix revealed problems. r=Gijs Differential Revision: https://phabricator.services.mozilla.com/D12156
toolkit/components/normandy/actions/BaseAction.jsm
toolkit/components/normandy/actions/PreferenceRollbackAction.jsm
toolkit/components/normandy/actions/PreferenceRolloutAction.jsm
toolkit/components/normandy/test/browser/browser_BaseAction.js
toolkit/components/normandy/test/browser/browser_actions_AddonStudyAction.js
toolkit/components/normandy/test/browser/browser_actions_ConsoleLogAction.js
toolkit/components/normandy/test/browser/browser_actions_PreferenceRollbackAction.js
toolkit/components/normandy/test/browser/browser_actions_PreferenceRolloutAction.js
--- a/toolkit/components/normandy/actions/BaseAction.jsm
+++ b/toolkit/components/normandy/actions/BaseAction.jsm
@@ -17,27 +17,33 @@ var EXPORTED_SYMBOLS = ["BaseAction"];
  *
  * Other methods should be overridden with care, to maintain the life
  * cycle events and error reporting implemented by this class.
  */
 class BaseAction {
   constructor() {
     this.state = BaseAction.STATE_PREPARING;
     this.log = LogManager.getLogger(`action.${this.name}`);
+    this.lastError = null;
 
     try {
       this._preExecution();
       // if _preExecution changed the state, don't overwrite it
       if (this.state === BaseAction.STATE_PREPARING) {
         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);
     }
   }
 
   get schema() {
     return {
       type: "object",
       properties: {},
     };
@@ -46,27 +52,29 @@ class BaseAction {
   /**
    * Disable the action for a non-error reason, such as the user opting out of
    * this type of action.
    */
   disable() {
     this.state = BaseAction.STATE_DISABLED;
   }
 
-  fail() {
+  fail(err) {
     switch (this.state) {
       case BaseAction.STATE_PREPARING: {
         Uptake.reportAction(this.name, Uptake.ACTION_PRE_EXECUTION_ERROR);
         break;
       }
       default: {
         Cu.reportError(new Error("BaseAction.fail() called at unexpected time"));
       }
     }
     this.state = BaseAction.STATE_FAILED;
+    this.lastError = err;
+    Cu.reportError(err);
   }
 
   // Gets the name of the action. Does not necessarily match the
   // server slug for the action.
   get name() {
     return this.constructor.name;
   }
 
@@ -136,24 +144,25 @@ class BaseAction {
         throw new Error("Action has already been finalized");
       }
       case BaseAction.STATE_READY: {
         try {
           await this._finalize();
           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) {
             // Sometimes Error.message cannot be updated. Log a warning, and move on.
             this.log.debug(`Could not run postExecution hook for ${this.name}`);
           }
 
+          this.lastError = err;
           Cu.reportError(err);
         }
         break;
       }
       case BaseAction.STATE_DISABLED: {
         this.log.debug(`Skipping post-execution hook for ${this.name} because it is disabled.`);
         status = Uptake.ACTION_SUCCESS;
         break;
--- a/toolkit/components/normandy/actions/PreferenceRollbackAction.jsm
+++ b/toolkit/components/normandy/actions/PreferenceRollbackAction.jsm
@@ -50,11 +50,10 @@ class PreferenceRollbackAction extends B
       default: {
         throw new Error(`Unexpected state when rolling back ${rolloutSlug}: ${rollout.state}`);
       }
     }
   }
 
   async _finalize() {
     await PreferenceRollouts.saveStartupPrefs();
-    await PreferenceRollouts.closeDB();
   }
 }
--- a/toolkit/components/normandy/actions/PreferenceRolloutAction.jsm
+++ b/toolkit/components/normandy/actions/PreferenceRolloutAction.jsm
@@ -165,11 +165,10 @@ class PreferenceRolloutAction extends Ba
         PrefUtils.setPref("default", prefSpec.preferenceName, prefSpec.value);
       }
     }
     return anyChanged;
   }
 
   async _finalize() {
     await PreferenceRollouts.saveStartupPrefs();
-    await PreferenceRollouts.closeDB();
   }
 }
--- a/toolkit/components/normandy/test/browser/browser_BaseAction.js
+++ b/toolkit/components/normandy/test/browser/browser_BaseAction.js
@@ -22,31 +22,33 @@ class NoopAction extends BaseAction {
     this._testRunFlag = true;
   }
 
   _finalize() {
     this._testFinalizeFlag = true;
   }
 }
 
+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;
   }
 }
 
 // Test that constructor and override methods are run
 decorate_task(
   withStub(Uptake, "reportRecipe"),
   withStub(Uptake, "reportAction"),
   async () => {
@@ -123,24 +125,26 @@ decorate_task(
 // Test an action with a failing pre-execution step
 decorate_task(
   withStub(Uptake, "reportRecipe"),
   withStub(Uptake, "reportAction"),
   async function(reportRecipeStub, reportActionStub) {
     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);
     is(action.state, FailPreExecutionAction.STATE_FAILED, "Action should remain failed");
 
     // 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");
 
     Assert.deepEqual(
       reportRecipeStub.args,
       [[recipe.id, Uptake.RECIPE_ACTION_DISABLED]],
       "Recipe should report recipe status as action disabled",
--- a/toolkit/components/normandy/test/browser/browser_actions_AddonStudyAction.js
+++ b/toolkit/components/normandy/test/browser/browser_actions_AddonStudyAction.js
@@ -51,16 +51,17 @@ decorate_task(
         name: study.name,
         description: study.description,
         addonUrl: study.addonUrl,
       },
     });
     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");
   },
 );
 
 // Test that if the add-on fails to install, the database is cleaned up and the
 // error is correctly reported.
 decorate_task(
@@ -90,16 +91,17 @@ decorate_task(
   withSendEventStub,
   withInstalledWebExtension({ version: "0.1", id: FIXTURE_ADDON_ID }),
   async function conflictingEnrollment(studies, sendEventStub, [installedAddonId, installedAddonFile]) {
     is(installedAddonId, FIXTURE_ADDON_ID, "Generated, installed add-on should have the same ID as the fixture");
     const addonUrl = FIXTURE_ADDON_URL;
     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");
 
     Assert.deepEqual(await AddonStudies.getAll(), [], "There should be no enrolled studies");
 
     Assert.deepEqual(
       sendEventStub.args,
@@ -119,16 +121,17 @@ decorate_task(
     const addonUrl = FIXTURE_ADDON_URL;
 
     let addon = await AddonManager.getAddonByID(FIXTURE_ADDON_ID);
     is(addon, null, "Before enroll, the add-on is not installed");
 
     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);
     ok(addon, "After start is called, the add-on is installed");
 
     Assert.deepEqual(addon.installTelemetryInfo, {source: "internal"},
                      "Got the expected installTelemetryInfo");
 
@@ -265,16 +268,17 @@ decorate_task(
     mockPreferences.set("app.shield.optoutstudies.enabled", false);
     const action = new AddonStudyAction();
     is(action.state, AddonStudyAction.STATE_DISABLED, "the action should be disabled");
     const enrollSpy = sinon.spy(action, "enroll");
     const recipe = addonStudyRecipeFactory();
     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");
   },
 );
 
 // Test that the action does not execute paused recipes
 decorate_task(
   ensureAddonCleanup,
@@ -294,11 +298,12 @@ decorate_task(
 // Test that enroll is not called if recipe is already enrolled
 decorate_task(
   ensureAddonCleanup,
   AddonStudies.withStudies([addonStudyFactory()]),
   async function enrollTwiceFail([study]) {
     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");
   },
 );
--- a/toolkit/components/normandy/test/browser/browser_actions_ConsoleLogAction.js
+++ b/toolkit/components/normandy/test/browser/browser_actions_ConsoleLogAction.js
@@ -5,16 +5,17 @@ ChromeUtils.import("resource://normandy/
 
 // Test that logging works
 add_task(async function logging_works() {
   const action = new ConsoleLogAction();
   const infoStub = sinon.stub(action.log, "info");
   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();
   }
 });
 
 
 // test that argument validation works
@@ -23,23 +24,25 @@ decorate_task(
   async function arguments_are_validated(reportRecipeStub) {
     const action = new ConsoleLogAction();
     const infoStub = sinon.stub(action.log, "info");
 
     try {
       // 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]]);
 
       reportRecipeStub.reset();
 
       // 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 {
       infoStub.restore();
     }
   },
 );
--- a/toolkit/components/normandy/test/browser/browser_actions_PreferenceRollbackAction.js
+++ b/toolkit/components/normandy/test/browser/browser_actions_PreferenceRollbackAction.js
@@ -28,16 +28,17 @@ decorate_task(
       ],
     });
 
     const recipe = {id: 1, arguments: {rolloutSlug: "test-rollout"}};
 
     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");
     is(Services.prefs.getCharPref("test.pref2"), "builtin value", "string pref should be rolled back");
     is(Services.prefs.getBoolPref("test.pref3"), false, "boolean pref should be rolled back");
 
     // start up prefs are unset
     is(Services.prefs.getPrefType("app.normandy.startupRolloutPrefs.test.pref1"), Services.prefs.PREF_INVALID, "integer startup pref should be unset");
@@ -86,16 +87,17 @@ decorate_task(
       preferences: [{preferenceName: "test.pref", value: 1, previousValue: 1}],
     });
 
     let recipe = {id: 1, arguments: {rolloutSlug: "graduated-rollout"}};
 
     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");
 
     // rollout in the DB hasn't changed
     Assert.deepEqual(
       await PreferenceRollouts.getAll(),
       [{
@@ -123,16 +125,17 @@ decorate_task(
   withSendEventStub,
   withStub(Uptake, "reportRecipe"),
   async function rollback_without_rollout(sendEventStub, reportRecipeStub) {
     let recipe = {id: 1, arguments: {rolloutSlug: "missing-rollout"}};
 
     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(
       reportRecipeStub.args,
       [[recipe.id, Uptake.RECIPE_SUCCESS]],
       "recipe should be reported as succesful",
     );
   },
@@ -152,16 +155,17 @@ decorate_task(
       state: PreferenceRollouts.STATE_ROLLED_BACK,
       preferences: [{preferenceName: "test.pref", value: 2, previousValue: 1}],
     };
     PreferenceRollouts.add(rollout);
 
     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");
 
     // rollout in db was updated
     Assert.deepEqual(await PreferenceRollouts.getAll(), [rollout], "Rollout shouldn't change in db");
 
     // Telemetry is updated
@@ -188,16 +192,17 @@ decorate_task(
       ],
     });
 
     const recipe = {id: 1, arguments: {rolloutSlug: "test-rollout"}};
 
     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");
 
     // Cleanup
     Services.prefs.deleteBranch("test.pref");
     Services.prefs.getDefaultBranch("").deleteBranch("test.pref");
   },
--- a/toolkit/components/normandy/test/browser/browser_actions_PreferenceRolloutAction.js
+++ b/toolkit/components/normandy/test/browser/browser_actions_PreferenceRolloutAction.js
@@ -23,16 +23,17 @@ decorate_task(
           {preferenceName: "test.pref3", value: "it works"},
         ],
       },
     };
 
     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");
     is(Services.prefs.getBoolPref("test.pref2"), true, "boolean pref should be set");
     is(Services.prefs.getCharPref("test.pref3"), "it works", "string pref should be set");
 
     // start up prefs are set
     is(Services.prefs.getIntPref("app.normandy.startupRolloutPrefs.test.pref1"), 1, "integer startup pref should be set");
@@ -87,16 +88,17 @@ decorate_task(
           {preferenceName: "test.pref2", value: 1},
         ],
       },
     };
 
     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");
     is(defaultBranch.getIntPref("test.pref2"), 1, "pref2 should be set");
     is(Services.prefs.getIntPref("app.normandy.startupRolloutPrefs.test.pref1"), 1, "startup pref1 should be set");
     is(Services.prefs.getIntPref("app.normandy.startupRolloutPrefs.test.pref2"), 1, "startup pref2 should be set");
 
     // update existing enrollment
@@ -105,16 +107,17 @@ decorate_task(
       // pref2's value is updated
       {preferenceName: "test.pref2", value: 2},
       // pref3 is added
       {preferenceName: "test.pref3", value: 2},
     ];
     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");
     is(Services.prefs.getIntPref("test.pref2"), 2, "pref2 should be updated");
     is(Services.prefs.getIntPref("test.pref3"), 2, "pref3 should be added");
 
     is(Services.prefs.getPrefType(
       "app.normandy.startupRolloutPrefs.test.pref1"),
@@ -180,16 +183,17 @@ decorate_task(
         slug: "test-rollout",
         preferences: [{preferenceName: "test.pref", value: 2}],
       },
     };
 
     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");
 
     // rollout in the DB has been ungraduated
     Assert.deepEqual(
       await PreferenceRollouts.getAll(),
       [{
@@ -240,21 +244,23 @@ decorate_task(
       },
     };
 
     // running both in the same session
     let action = new PreferenceRolloutAction();
     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");
     is(Services.prefs.getPrefType("test.pref3"), Services.prefs.PREF_INVALID, "pref3 is not set");
 
     is(Services.prefs.getIntPref("app.normandy.startupRolloutPrefs.test.pref1"), 1, "startup pref1 is set to recipe1's value");
     is(Services.prefs.getIntPref("app.normandy.startupRolloutPrefs.test.pref2"), 1, "startup pref2 is set to recipe1's value");
     is(Services.prefs.getPrefType("app.normandy.startupRolloutPrefs.test.pref3"), Services.prefs.PREF_INVALID, "startup pref3 is not set");
@@ -301,16 +307,17 @@ decorate_task(
         slug: "test-rollout",
         preferences: [{preferenceName: "test.pref", value: 1}],
       },
     };
 
     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");
 
     Assert.deepEqual(await PreferenceRollouts.getAll(), [], "no rollout is stored in the db");
     Assert.deepEqual(
       sendEventStub.args,
       [["enrollFailed", "preference_rollout", recipe.arguments.slug, {reason: "invalid type", preference: "test.pref"}]],
@@ -334,16 +341,17 @@ decorate_task(
         slug: "test-rollout",
         preferences: [{preferenceName: "test.pref", value: "rollout value"}],
       },
     };
 
     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");
 
     Assert.deepEqual(
       await PreferenceRollouts.getAll(),
       [{
         slug: "test-rollout",
@@ -372,16 +380,17 @@ decorate_task(
     };
 
     // Set a pref on the user branch only
     Services.prefs.setIntPref("test.pref", 2);
 
     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");
     is(Services.prefs.getIntPref("app.normandy.startupRolloutPrefs.test.pref"), 1, "startup pref is est");
 
     // Cleanup
     Services.prefs.getDefaultBranch("").deleteBranch("test.pref");
   },
@@ -400,21 +409,23 @@ decorate_task(
         preferences: [{preferenceName: "test.pref", value: 1}],
       },
     };
 
     // run once
     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,
       [["enroll", "preference_rollout", "test-rollout", {}]],
       "only an enrollment event should be generated",
     );
 
     Assert.deepEqual(
@@ -443,16 +454,17 @@ decorate_task(
         slug: "test-rollout",
         preferences: [{preferenceName: "test.pref", value: 1}],
       },
     };
 
     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");
 
     // start up pref isn't set
     is(Services.prefs.getPrefType(
       "app.normandy.startupRolloutPrefs.test.pref"),
       Services.prefs.PREF_INVALID,
       "startup pref1 should not be set",