fix: decision service for group and multiple experiments. #322

ozayr-zaviar · 2021-04-23T07:08:40Z

Summary

get_variation_for_feature code updated to fix group and multiple experiment issues.

Fixes:

Currently only first experiment is picked through feature.experimentIds[0]. Which is fixed by using loop.
Group experiments are evaluated first then individual experiments are evaluated instead of evaluating first occurring experiment in feature like in other SDKs which is fixed by removing condition for group experiments.

Test plan

New test cases for group experiments should pass in FSC

Issues

Need to fix test cases for get_variation_for_feature after current changes in Python.

jaeopt

LGTM with a comment fix suggested

jaeopt · 2021-04-23T20:54:12Z

optimizely/decision_service.py

-                decide_reasons += reasons
-                if experiment and experiment.id in feature.experimentIds:
+
+        # Next check if the feature is being experimented on


Remove "Next"

msohailhussain · 2021-04-26T21:36:52Z

Remove the code in project_config.py where
feature.groupId is set since it's no more of use after changing the logic.

coveralls · 2021-04-27T11:59:37Z

Coverage decreased (-0.04%) to 95.838% when pulling f0d9f60 on uzair/decision-service-for-group-fixes into 46651fa on master.

msohailhussain

I am not convinced on unit test changes, doesn't make sense to mock the value and expect the same value. Either we should mock Bucketer or remove these unit tests.

msohailhussain · 2021-04-27T00:13:20Z

tests/test_decision_service.py

            "test_user",
            None,
            False
        )

    def test_get_variation_for_feature__returns_none_for_user_not_in_group(self):
        """ Test that get_variation_for_feature returns None for
-    user not in group and the feature is not part of a rollout. """
+        user not in group and the feature is not part of a rollout. """


indent like before.

msohailhussain · 2021-04-27T20:51:23Z

tests/test_decision_service.py

@@ -1277,7 +1277,6 @@ def test_get_variation_for_feature__returns_variation_if_user_not_in_experiment_
            side_effect=[[False, []], [True, []]],
        ) as mock_audience_check, self.mock_decision_logger as mock_decision_service_logging, mock.patch(
                "optimizely.bucketer.Bucketer.bucket", return_value=[expected_variation, []]):
-


don't remove existing line.

msohailhussain · 2021-04-27T20:57:07Z

tests/test_decision_service.py

-            )
-        mock_decision_service_logging.error.assert_called_once_with(
-            enums.Errors.INVALID_GROUP_ID.format("_get_variation_for_feature")
+        variation_received, _ = self.decision_service.get_variation_for_feature(


Please add logging.

jaeopt

Those test cases updated all turn out to be same - "returns rollout decisions when feature tests cannot find variations".
Can we fix them (and add more tests) to test

group exclusion in a feature
multiple non-grouped tests in a feature

jaeopt · 2021-04-27T23:34:54Z

tests/test_decision_service.py

-        ) as mock_get_experiment_in_group, mock.patch(
-            "optimizely.decision_service.DecisionService.get_variation"
-        ) as mock_decision:
+                "optimizely.decision_service.DecisionService.get_variation",


This mock does not give a test for "group". It tests the cases for feature-tests not bucketed.
Can we mock bucket to control group-exclusion cases?

jaeopt · 2021-04-27T23:36:58Z

tests/test_decision_service.py

-            "optimizely.decision_service.DecisionService.get_experiment_in_group",
-            return_value=(self.project_config.get_experiment_from_key("group_exp_1"), []),
-        ) as mock_get_experiment_in_group, mock.patch(
            "optimizely.decision_service.DecisionService.get_variation",
            return_value=(expected_variation, []),
        ) as mock_decision:


This mock does not give a test for "group". It tests the cases for feature-tests not bucketed.
Can we mock bucket to control group-exclusion cases?

jaeopt · 2021-04-27T23:41:11Z

tests/test_decision_service.py

        feature = self.project_config.get_feature_from_key("test_feature_in_group")
        feature.groupId = "aabbccdd"


We do not use this feature "groupId" any more. This test is not valid any more.

jaeopt · 2021-04-27T23:45:50Z

tests/test_decision_service.py

-            "optimizely.decision_service.DecisionService.get_experiment_in_group",
-            return_value=[self.project_config.get_experiment_from_key("group_exp_2"), []],
+            "optimizely.decision_service.DecisionService.get_variation",
+            return_value=[None, []],
        ) as mock_decision:


Same test as above.

msohailhussain · 2021-04-30T22:20:44Z

tests/base.py

+                    'forcedVariations': {},
+                    'trafficAllocation': [
+                        {'entityId': '', 'endOfRange': 2500},
+                        {'entityId': '', 'endOfRange': 5000},


can we generate datafile like this?

msohailhussain · 2021-04-30T23:20:40Z

tests/base.py

+                    'forcedVariations': {},
+                    'trafficAllocation': [
+                        {'entityId': '222239', 'endOfRange': 2500},
+                        {'entityId': '', 'endOfRange': 5000},


need to discuss offline.

msohailhussain · 2021-04-30T23:21:23Z

tests/test_config.py

@@ -501,7 +501,7 @@ def test_init__with_v4_datafile(self):
                '211111',
                {'number_of_projects': entities.Variable('131', 'number_of_projects', 'integer', '10')},
            ),
-            'test_feature_in_group': entities.FeatureFlag('91113', 'test_feature_in_group', ['32222'], '', {}, '19228'),
+            'test_feature_in_group': entities.FeatureFlag('91113', 'test_feature_in_group', ['32222'], '', {}),


why 19228 removed.

msohailhussain · 2021-04-30T23:28:35Z

tests/test_decision_service.py

+    def test_get_variation_for_feature__returns_none_for_user_in_group_experiment_not_associated_with_feature(
+        self,
+    ):
+        """ Test that if a user is in the mutex group but the experiment is


Need to revise description, doesn't make sense. let's talk offline.

msohailhussain · 2021-04-30T23:29:32Z

tests/test_decision_service.py

-            "test_user",
-            None,
-            False
+            self.project_config, self.project_config.get_experiment_from_id("32222"), "test_user", None, False


Why we are asserting get_experiment_from_id.

msohailhussain · 2021-04-30T23:35:14Z

tests/base.py

+                            'audienceIds': ['11160'],
+                            'layerId': '211183',
+                            'variations': [
+                                {'key': 'var_1', 'id': '38901'},


append experiment key name with the key.

msohailhussain · 2021-04-30T23:35:31Z

tests/base.py

+                            'audienceIds': ['11160'],
+                            'layerId': '211184',
+                            'variations': [
+                                {'key': 'var_1', 'id': '38905'}


msohailhussain · 2021-04-30T23:35:40Z

tests/base.py

+                            'audienceIds': ['11160'],
+                            'layerId': '211185',
+                            'variations': [
+                                {'key': 'var_1', 'id': '38906'}


same as above.

msohailhussain

lgtm

jaeopt

The new tests look great.
I suggest a few small changes and add one more test.

jaeopt · 2021-05-04T00:28:53Z

tests/test_decision_service.py

+            )
+
+        mock_config_logging.debug.assert_called_with('Assigned bucket 2400 to user with bucketing ID "test_user".')
+        mock_generate_bucket_value.assert_called_with('test_user42222')


Suggested change

mock_generate_bucket_value.assert_called_with('test_user42222')

mock_generate_bucket_value.assert_called_with('test_user19229')

group bucketing is more interesting here

jaeopt · 2021-05-04T00:29:28Z

tests/test_decision_service.py

+                variation_received,
+            )
+        mock_config_logging.debug.assert_called_with('Assigned bucket 4000 to user with bucketing ID "test_user".')
+        mock_generate_bucket_value.assert_called_with('test_user42223')


same as above

jaeopt · 2021-05-04T00:29:34Z

tests/test_decision_service.py

+                variation_received,
+            )
+        mock_config_logging.debug.assert_called_with('Assigned bucket 6500 to user with bucketing ID "test_user".')
+        mock_generate_bucket_value.assert_called_with('test_user42224')


same as above

jaeopt · 2021-05-04T00:31:23Z

tests/test_decision_service.py

+        self,
+    ):
+        """ Test that if a user is in the mutex group and the user bucket value should be equal to 2500
+        or greater than 5000."""


Suggested change

or greater than 5000."""

or less than 5000."""

jaeopt · 2021-05-04T00:31:36Z

tests/test_decision_service.py

+        self,
+    ):
+        """ Test that if a user is in the mutex group and the user bucket value should be equal to 5000
+        or greater than 7500."""


Suggested change

or greater than 7500."""

or less than 7500."""

jaeopt · 2021-05-04T00:36:27Z

tests/test_decision_service.py

+                variation_received,
+            )
+
+        mock_generate_bucket_value.assert_called_with('test_user211147')


Suggested change

mock_generate_bucket_value.assert_called_with('test_user211147')

mock_generate_bucket_value.assert_called_with('test_user19229')

jaeopt · 2021-05-04T00:36:58Z

tests/test_decision_service.py

+        self,
+    ):
+        """ Test that if a user is in the non-mutex group and the user bucket value should be equal to 2500
+        or greater than 5000."""


Suggested change

or greater than 5000."""

or less than 5000."""

jaeopt · 2021-05-04T00:37:20Z

tests/test_decision_service.py

+        self,
+    ):
+        """ Test that if a user is in the non-mutex group and the user bucket value should be equal to 5000
+        or greater than 7500."""


Suggested change

or greater than 7500."""

or less than 7500."""

jaeopt · 2021-05-04T00:39:39Z

tests/test_decision_service.py

+            )
+
+        mock_generate_bucket_value.assert_called_with('test_user211147')
+        mock_config_logging.debug.assert_called_with('Assigned bucket 8000 to user with bucketing ID "test_user".')


These tests look great.
One missing test is that in multiple experiments, one of the test can be set to missing target by audience (instead of traffic allocation). We do not need to repeat multiple traffic allocation tests. One of them can be changed to use audience-mismatch.

jaeopt

LGTM

code chnages for get_variation_for_feature

3992620

optimizely-codereview-bot assigned ozayr-zaviar Apr 23, 2021

ozayr-zaviar requested a review from msohailhussain April 23, 2021 07:09

ozayr-zaviar removed their assignment Apr 23, 2021

ozayr-zaviar changed the title ~~refactor: decision service for group and multiple experiments.~~ fix: decision service for group and multiple experiments. Apr 23, 2021

jaeopt approved these changes Apr 23, 2021

View reviewed changes

ozayr-zaviar added 2 commits April 26, 2021 16:53

comments and testcases addressed

62987b0

unused variable fixed

3655fc6

ozayr-zaviar and others added 2 commits April 27, 2021 16:41

test cases order corrected

10431d2

linting fixed

8d69a02

msohailhussain reviewed Apr 27, 2021

View reviewed changes

jaeopt requested changes Apr 27, 2021

View reviewed changes

ozayr-zaviar and others added 8 commits April 29, 2021 17:48

data entered for new test cases

f4c9e7e

linting fixed

5ef8e26

fixes

cd59fa8

test case in user context fixed

569720a

config hardcoded feature experiment mapping fixed

8fb6b73

added testcases for group and experiments

777e975

indentation fixed

c2d6752

comments addressed

790ee40

msohailhussain marked this pull request as ready for review April 30, 2021 22:05

msohailhussain requested a review from a team as a code owner April 30, 2021 22:05

optimizely-codereview-bot added the 🤖 24 hrs label Apr 30, 2021

msohailhussain reviewed Apr 30, 2021

View reviewed changes

tests/base.py

'audienceIds': ['11160'],

'layerId': '211184',

'variations': [

{'key': 'var_1', 'id': '38905'}

Copy link

Contributor

msohailhussain Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here.

msohailhussain reviewed Apr 30, 2021

View reviewed changes

empty traffic allocations removed before first non empty allocation

bad76e7

msohailhussain approved these changes May 3, 2021

View reviewed changes

msohailhussain requested a review from jaeopt May 3, 2021 19:14

jaeopt requested changes May 4, 2021

View reviewed changes

optimizely-codereview-bot removed the 🤖 24 hrs label May 4, 2021

optimizely-codereview-bot assigned ozayr-zaviar May 4, 2021

ozayr-zaviar added 2 commits May 4, 2021 17:25

comments addressed

804fdb6

newline added

f0d9f60

jaeopt approved these changes May 4, 2021

View reviewed changes

jaeopt merged commit 4470ca9 into master May 4, 2021

jaeopt deleted the uzair/decision-service-for-group-fixes branch May 4, 2021 18:40

Mat001 mentioned this pull request May 28, 2021

Chore: prepare for 3.9.0 release #329

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: decision service for group and multiple experiments. #322

fix: decision service for group and multiple experiments. #322

ozayr-zaviar commented Apr 23, 2021 •

edited

Loading

jaeopt left a comment

jaeopt Apr 23, 2021

msohailhussain commented Apr 26, 2021

coveralls commented Apr 27, 2021 •

edited

Loading

msohailhussain left a comment

msohailhussain Apr 27, 2021

msohailhussain Apr 27, 2021

msohailhussain Apr 27, 2021

jaeopt left a comment

jaeopt Apr 27, 2021

jaeopt Apr 27, 2021

jaeopt Apr 27, 2021

jaeopt Apr 27, 2021

msohailhussain Apr 30, 2021

msohailhussain Apr 30, 2021

msohailhussain Apr 30, 2021

msohailhussain Apr 30, 2021

msohailhussain Apr 30, 2021

msohailhussain Apr 30, 2021

msohailhussain Apr 30, 2021

msohailhussain Apr 30, 2021

msohailhussain left a comment

jaeopt left a comment

jaeopt May 4, 2021

jaeopt May 4, 2021

jaeopt May 4, 2021

jaeopt May 4, 2021

jaeopt May 4, 2021

jaeopt May 4, 2021

jaeopt May 4, 2021

jaeopt May 4, 2021

jaeopt May 4, 2021

jaeopt left a comment

		feature = self.project_config.get_feature_from_key("test_feature_in_group")
		feature.groupId = "aabbccdd"

	mock_generate_bucket_value.assert_called_with('test_user42222')
	mock_generate_bucket_value.assert_called_with('test_user19229')

fix: decision service for group and multiple experiments. #322

fix: decision service for group and multiple experiments. #322

Conversation

ozayr-zaviar commented Apr 23, 2021 • edited Loading

Summary

Fixes:

Test plan

Issues

jaeopt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msohailhussain commented Apr 26, 2021

coveralls commented Apr 27, 2021 • edited Loading

msohailhussain 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

jaeopt 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

msohailhussain left a comment

Choose a reason for hiding this comment

jaeopt 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

jaeopt left a comment

Choose a reason for hiding this comment

ozayr-zaviar commented Apr 23, 2021 •

edited

Loading

coveralls commented Apr 27, 2021 •

edited

Loading