Bug 1440779: BaseAction: move _preExecute call out of constructor r=mythmon,Gijs

Having _preExecute be called in the constructor makes it hard to test
it, but because it's a lifecycle hook that subclasses are intended to
override, testing it is very natural.

While we're here, move the initialization of AddonStudyAction from
_preExecute to the constructor, since it doesn't really make sense for
AddonStudyAction to be constructed without all members initialized.

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

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

toolkit/components/normandy/actions/AddonStudyAction.jsm

@@ -59,6 +59,11 @@ class AddonStudyAction extends BaseAction {
     return ActionSchemas["addon-study"];
   }
 
+  constructor() {
+    super();
+    this.seenRecipeIds = new Set();
+  }
+
   /**
    * This hook is executed once before any recipes have been processed, it is
    * responsible for:
@@ -72,8 +77,6 @@ class AddonStudyAction extends BaseAction {
       this.log.info("User has opted-out of opt-out experiments, disabling action.");
       this.disable();
     }
-
-    this.seenRecipeIds = new Set();
   }
 
   /**

toolkit/components/normandy/actions/BaseAction.jsm

@@ -23,6 +23,18 @@ class BaseAction {
     this.state = BaseAction.STATE_PREPARING;
     this.log = LogManager.getLogger(`action.${this.name}`);
     this.lastError = null;
+  }
+
+  /**
+   * Be sure to run the _preExecution() hook once during its
+   * lifecycle.
+   *
+   * This is not intended for overriding by subclasses.
+   */
+  _ensurePreExecution() {
+    if (this.state !== BaseAction.STATE_PREPARING) {
+      return;
+    }
 
     try {
       this._preExecution();
@@ -94,6 +106,8 @@ class BaseAction {
    * @throws If this action has already been finalized.
    */
   async runRecipe(recipe) {
+    this._ensurePreExecution();
+
     if (this.state === BaseAction.STATE_FINALIZED) {
       throw new Error("Action has already been finalized");
     }
@@ -138,6 +152,11 @@ class BaseAction {
    * recipes will be assumed to have been seen.
    */
   async finalize() {
+    // It's possible that no recipes matched us, so runRecipe() was
+    // never called. In that case, we should ensure that we call
+    // _preExecute() here.
+    this._ensurePreExecution();
+
     let status;
     switch (this.state) {
       case BaseAction.STATE_FINALIZED: {

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

@@ -6,10 +6,7 @@ ChromeUtils.import("resource://normandy/lib/Uptake.jsm", this);
 class NoopAction extends BaseAction {
   constructor() {
     super();
-    // this._testPreExecutionFlag is set by _preExecution, called in the constructor
-    if (this._testPreExecutionFlag === undefined) {
-      this._testPreExecutionFlag = false;
-    }
+    this._testPreExecutionFlag = false;
     this._testRunFlag = false;
     this._testFinalizeFlag = false;
   }
@@ -52,18 +49,25 @@ decorate_task(
   withStub(Uptake, "reportRecipe"),
   withStub(Uptake, "reportAction"),
   async () => {
-    const action = new NoopAction();
-    is(action._testPreExecutionFlag, true, "_preExecution should be called on a new action");
+    let action = new NoopAction();
+    is(action._testPreExecutionFlag, false, "_preExecution should not have been called on a new action");
     is(action._testRunFlag, false, "_run has should not have been called on a new action");
     is(action._testFinalizeFlag, false, "_finalize should not be called on a new action");
 
     const recipe = recipeFactory();
     await action.runRecipe(recipe);
+    is(action._testPreExecutionFlag, true, "_preExecution should be called when a recipe is executed");
     is(action._testRunFlag, true, "_run should be called when a recipe is executed");
     is(action._testFinalizeFlag, false, "_finalize should not have been called when a recipe is executed");
 
     await action.finalize();
     is(action._testFinalizeFlag, true, "_finalizeExecution should be called when finalize was called");
+
+    action = new NoopAction();
+    await action.finalize();
+    is(action._testPreExecutionFlag, true, "_preExecution should be called when finalized even if no recipes");
+    is(action._testRunFlag, false, "_run should be called if no recipes were run");
+    is(action._testFinalizeFlag, true, "_finalize should be called when finalized");
   }
 );
 
@@ -129,12 +133,13 @@ decorate_task(
   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");
+    is(action.state, FailPreExecutionAction.STATE_PREPARING, "Pre-execution should not happen immediately");
 
-    // Should not throw, even though the action is in a disabled state.
+    // Should fail, putting the action into a "failed" state, but the
+    // entry point `runRecipe()` itself should not throw an exception.
     await action.runRecipe(recipe);
-    is(action.state, FailPreExecutionAction.STATE_FAILED, "Action should remain failed");
+    is(action.state, FailPreExecutionAction.STATE_FAILED, "Action fails if pre-execution fails");
+    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.finalize();

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

@@ -267,10 +267,10 @@ decorate_task(
   async function testOptOut(sendEventStub, mockPreferences) {
     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);
+    is(action.state, AddonStudyAction.STATE_DISABLED, "the action should be disabled");
     await action.finalize();
     is(action.state, AddonStudyAction.STATE_FINALIZED, "the action should be finalized");
     is(action.lastError, null, "lastError should be null");