feat (audiences): Audience combinations #175

mjc1283 · 2018-10-16T16:53:30Z

This adds support for audience combinations on experiments. If experiment.audienceConditions is present, it will be used as a condition tree where the leaf conditions are audience Ids.

Condition tree evaluation and custom attribute condition evaluation are split into separate modules. Condition tree evaluation accepts a leaf evaluator function argument.
The audience evaluator uses the condition tree evaluator on both the experiment audience conditions and the individual audience conditions.
Project config methods are updated to support audience combinations. getExperimentAudienceConditions returns audienceConditions if it exists, otherwise it falls back to audienceIds.
The audience evaluator treats conditions trees and flat lists of audience ids (the old format) the same way, because a flat list of audience ids is a valid condition tree with an implicit OR condition.

condition evaluator generic to any kind of leaf evaulator. Update audience evaluator to use the generic condition evaluator, providing a leaf evaluator that uses the custom attribute condition evaluator module.

… audiences

nchilada

I haven't looked at the tests yet, but the code under test looks really good! My only important suggestion relates to a JSDoc.

I'll scan through the tests after lunch!

nchilada · 2018-10-17T19:07:09Z

packages/optimizely-sdk/lib/core/audience_evaluator/index.js

-   *                                         determining if the audience conditions are met. If not
-   *                                         provided, defaults to an empty object.
-   * @return {Boolean}  True if the user attributes match the given audience conditions
+   * @param  {Array}    audienceConditions    Audience conditions to match the user attributes against - can be an array


Can we declare something like {Array|Object|null|undefined}? Since we support a single leaf condition without any operators and we accept empty conditions too.

👍 Will do this.

packages/optimizely-sdk/lib/core/audience_evaluator/index.js

nchilada · 2018-10-17T19:17:14Z

packages/optimizely-sdk/lib/core/audience_evaluator/index.js

+    };
+
+    if (conditionTreeEvaluator.evaluate(audienceConditions, evaluateAudience)) {
+      return true;
    }

    return false;


Maybe

return conditionTreeEvaluator.evaluate(audienceConditions, evaluateAudience) || false;

? It's a little more concise and highlights the fact that we're converting falsy values (null) to false.

nchilada · 2018-10-17T19:26:47Z

packages/optimizely-sdk/lib/core/audience_evaluator/index.js

      }
+      return null;
+    };


Nit: for symmetry with customAttributeConditionEvaluator.evaluate, I wonder if we should export this functionality as audienceEvaluator.evaluate, and hoist all the other code up to DecisionService.prototype.__checkIfUserIsInAudience?

🔭:architecture_👨‍🚀:🤔

Interesting. This does feel cleaner. I could see the other code fitting into __checkIfUserIsInAudience. @mikeng13 thoughts?

With this approach, audience_evaluator ends up being pretty uninteresting, to the extent that it maybe shouldn't be a module.

Up to you. I was just thinking out loud, especially if there was interest in using the _.partial pattern as we do throughout client-js. Doesn't really make a difference

mjc1283 · 2018-10-17T20:08:48Z

packages/optimizely-sdk/lib/core/audience_evaluator/index.js

      }
+      return null;


@nchilada @mikeng13 Any thoughts on returning null here? Maybe this should throw an error, and project config should reject a datafile as invalid if an experiment has invalid audience ids.

I believe returning null is correct here - this function is being called at activate time so it's probably too late to decide that the datafile is invalid and to stop handling API calls. That said, we could add an additional check for invalid audience references during initialization. If we do so, we reduce this return null to a mere sanity check.

In the design doc I'd written:

If an audience can't be found in [audiences or typedAudiences], the SDK may fail gracefully or spectacularly, during initialization or during experiment activation—whatever it was doing before.

but I don't mind if someone want to propose that we consistently check for invalid audience references during initialization. @mikeng13 @thomaszurkan-optimizely what do you think?

coveralls · 2018-10-18T00:30:22Z

Coverage increased (+0.05%) to 97.38% when pulling f1dcb2d on mcarroll/audience-combinations into 5c18367 on master.

coveralls · 2018-10-18T00:30:23Z

Coverage increased (+0.09%) to 97.42% when pulling 34cc8ad on mcarroll/audience-combinations into 5c18367 on master.

nchilada

Tests look pretty good - just had some minor suggestions. Nice work @mjc1283!

nchilada · 2018-10-18T02:03:59Z

packages/optimizely-sdk/lib/core/condition_tree_evaluator/index.tests.js

+          it('should return false when operands evaluate to falses and nulls', function() {
+            var leafEvaluator = sinon.stub();
+            leafEvaluator.onCall(0).returns(false);
+            leafEvaluator.onCall(1).returns(null);


Maybe flip the order of these two return values, or add another test case, to prove that the evaluator doesn't short-circuit with null when it sees a null result?

nchilada · 2018-10-18T02:04:21Z

packages/optimizely-sdk/lib/core/condition_tree_evaluator/index.tests.js

+          it('should return null when operands evaluate to falses and nulls', function() {
+            var leafEvaluator = sinon.stub();
+            leafEvaluator.onCall(0).returns(null);
+            leafEvaluator.onCall(1).returns(false);


Maybe flip the order of these two return values, or add another test case, to prove that the evaluator doesn't short-circuit with false when it sees a false result?

nchilada · 2018-10-18T02:05:13Z

packages/optimizely-sdk/lib/core/condition_tree_evaluator/index.tests.js

+          it('should return null when operand evaluates to null', function() {
+            assert.isNull(conditionEvaluator.evaluate(['not', conditionA], function() { return null; }));
+          });
+        });


Other possible test cases, partly inspired by today's offline discussions:

no operands

additional operands

nchilada · 2018-10-18T02:11:56Z

packages/optimizely-sdk/lib/core/custom_attribute_condition_evaluator/index.tests.js

+    assert.isTrue(customAttributeEvaluator.evaluate(doubleCondition, userAttributes));
+  });
+
+  it('should return null when condition has an invalid type property', function() {


Another potential test case: 'missing type property'

nchilada · 2018-10-18T02:13:47Z

packages/optimizely-sdk/lib/core/custom_attribute_condition_evaluator/index.tests.js

+    });
+  });
+
+  describe('exact match type', function() {


Maybe also test the implicit "exact" match in cases where there's no match property at all. I believe this might be covered by the very first test case ('should return true there is a match') but a test case with a more specific description might be good.

nchilada · 2018-10-18T02:14:58Z

packages/optimizely-sdk/lib/core/custom_attribute_condition_evaluator/index.tests.js

+      assert.isFalse(result);
+    });
+
+    it('should return null if the user-provided value is not a number', function() {


Maybe also test this with the user-provided value being a number-like string

nchilada · 2018-10-18T02:15:07Z

packages/optimizely-sdk/lib/core/custom_attribute_condition_evaluator/index.tests.js

+      assert.isFalse(result);
+    });
+
+    it('should return null if the user-provided value is not a number', function() {


nchilada · 2018-10-18T02:16:36Z

packages/optimizely-sdk/lib/core/project_config/index.tests.js

+        var result = projectConfig.getExperimentAudienceConditions(configObj, 'audience_combinations_experiment');
+        assert.deepEqual(result, ['and', ['or', '3468206642', '3988293898'], ['or', '3988293899', '3468206646', '3468206647', '3468206644', '3468206643']]);
+      });
+    });


Possible additional test case: 'should return null [or undefined, I guess] if experiment has not audienceConditions or audienceIds'

I'm not sure this is worth testing.

No worries at all!

nchilada · 2018-10-18T02:19:49Z

packages/optimizely-sdk/lib/core/project_config/index.tests.js

+
+      it('should return experiment audienceConditions if experiment has audienceConditions', function() {
+        configObj = projectConfig.createProjectConfig(testDatafile.getTypedAudiencesConfig());
+        var result = projectConfig.getExperimentAudienceConditions(configObj, 'audience_combinations_experiment');


We should confirm that we prefer audienceConditions to audienceIds? Does this experiment also have audienceIds? A comment or some kind of easier-to-digest stub might help make that clear.

Will add a comment - this experiment does have both audienceIds and audienceConditions

nchilada · 2018-10-18T02:26:40Z

packages/optimizely-sdk/lib/optimizely/index.tests.js

+      });
+      assert.strictEqual(variableValue, 10);
+    });
+  });


I think I'm fine with the spareness of these test cases are, but can we at least use a sinon.spy to confirm that conditionTreeEvaluator.evaluate is called? (And FWIW, I think we can even assert that it's called once with the audience-ID leaf evaluator if we export that leaf evaluator function somewhere)

Do you think it'd satisfy the same need if we assert that audience_evaluator's evaluate was called? audience_evaluator has its own tests verifying the integration between evaluate and condition_tree_evaluator / custom_attribute_condition_evaluator.

Oh, that's right. Sure, that'd probably be even better (given that it's so hard to differentiate between the two places that make direct calls to conditionTreeEvaluator)

mikeproeng37

Looks good! Just a few small things.

mikeproeng37 · 2018-10-16T18:42:14Z

packages/optimizely-sdk/lib/core/condition_tree_evaluator/index.js

@@ -0,0 +1,125 @@
+/****************************************************************************
+ * Copyright 2016, 2018, Optimizely, Inc. and contributors                   *


nit: Remove 2016

mikeproeng37 · 2018-10-16T18:43:40Z

packages/optimizely-sdk/lib/core/condition_tree_evaluator/index.tests.js

@@ -0,0 +1,201 @@
+/****************************************************************************
+ * Copyright 2016, 2018, Optimizely, Inc. and contributors                  *


remove 2016

API tests for audience combinations.

Matt Carroll added 5 commits October 15, 2018 15:14

Move custom attribute condition evaluation logic to its own module. Make

066357b

condition evaluator generic to any kind of leaf evaulator. Update audience evaluator to use the generic condition evaluator, providing a leaf evaluator that uses the custom attribute condition evaluator module.

Update audience evaluator to handle complex audience conditions.

1c4e2dd

Fix doc comments in condition_evaluator

4be7e53

Rename files to better reflect purposes

a861ea6

Tests for top-level APIs using experiments/rollouts that have complex…

e84a5e1

… audiences

mjc1283 requested review from nchilada, mikeproeng37, aliabbasrizvi and thomaszurkan-optimizely October 16, 2018 16:53

nchilada reviewed Oct 17, 2018

View reviewed changes

mjc1283 commented Oct 17, 2018

View reviewed changes

Review fixes

f1dcb2d

nchilada approved these changes Oct 18, 2018

View reviewed changes

Test improvements from review

e21e4a6

mikeproeng37 approved these changes Oct 18, 2018

View reviewed changes

Matt Carroll added 2 commits October 18, 2018 16:41

Remove 2016 from license headers for new files

8a5d595

Add assertions that audienceEvaluator.evaluate was called in top-level

34cc8ad

API tests for audience combinations.

mjc1283 merged commit cb9d500 into master Oct 19, 2018

mjc1283 deleted the mcarroll/audience-combinations branch October 19, 2018 20:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat (audiences): Audience combinations #175

feat (audiences): Audience combinations #175

mjc1283 commented Oct 16, 2018

nchilada left a comment

nchilada Oct 17, 2018

mjc1283 Oct 17, 2018

nchilada Oct 17, 2018

mjc1283 Oct 17, 2018

nchilada Oct 17, 2018

mjc1283 Oct 17, 2018

mjc1283 Oct 17, 2018

nchilada Oct 17, 2018

mjc1283 Oct 17, 2018

nchilada Oct 17, 2018 •

edited

coveralls commented Oct 18, 2018

coveralls commented Oct 18, 2018 •

edited

nchilada left a comment

nchilada Oct 18, 2018

nchilada Oct 18, 2018

nchilada Oct 18, 2018

nchilada Oct 18, 2018

nchilada Oct 18, 2018

nchilada Oct 18, 2018

nchilada Oct 18, 2018

nchilada Oct 18, 2018

mjc1283 Oct 18, 2018

nchilada Oct 18, 2018

nchilada Oct 18, 2018

mjc1283 Oct 18, 2018

nchilada Oct 18, 2018

mjc1283 Oct 18, 2018

nchilada Oct 18, 2018

mikeproeng37 left a comment

mikeproeng37 Oct 16, 2018

mikeproeng37 Oct 16, 2018

		@@ -0,0 +1,125 @@
		/****************************************************************************
		* Copyright 2016, 2018, Optimizely, Inc. and contributors *

		@@ -0,0 +1,201 @@
		/****************************************************************************
		* Copyright 2016, 2018, Optimizely, Inc. and contributors *

feat (audiences): Audience combinations #175

feat (audiences): Audience combinations #175

Conversation

mjc1283 commented Oct 16, 2018

nchilada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nchilada Oct 17, 2018 • edited

Choose a reason for hiding this comment

coveralls commented Oct 18, 2018

coveralls commented Oct 18, 2018 • edited

nchilada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeproeng37 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nchilada Oct 17, 2018 •

edited

coveralls commented Oct 18, 2018 •

edited