fix: decision service for group and multiple experiments. by ozayr-zaviar · Pull Request #322 · optimizely/python-sdk

ozayr-zaviar · Apr 23, 2021

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 · Apr 23, 2021

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 · Apr 26, 2021

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

coveralls · Apr 27, 2021

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 · Apr 27, 2021

tests/test_decision_service.py

    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 · Apr 27, 2021

tests/test_decision_service.py

            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 · Apr 27, 2021

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 · Apr 27, 2021

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 · Apr 27, 2021

tests/test_decision_service.py

        with mock.patch(
-            "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 · Apr 27, 2021

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 · Apr 27, 2021

tests/test_decision_service.py

        with mock.patch(
-            "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 · Apr 30, 2021

tests/base.py

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


can we generate datafile like this?

msohailhussain · Apr 30, 2021

tests/base.py

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


need to discuss offline.

msohailhussain · Apr 30, 2021

tests/test_config.py

                {'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 · Apr 30, 2021

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 · Apr 30, 2021

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 · Apr 30, 2021

tests/base.py

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


append experiment key name with the key.

msohailhussain · Apr 30, 2021

tests/base.py

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


msohailhussain · Apr 30, 2021

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 · May 4, 2021

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 · May 4, 2021

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 · May 4, 2021

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 · May 4, 2021

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 · May 4, 2021

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 · May 4, 2021

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 · May 4, 2021

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 · May 4, 2021

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 · May 4, 2021

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

		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')

Search code, repositories, users, issues, pull requests...

Comments

Conversation

ozayr-zaviar commented Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Fixes:

Test plan

Issues

Uh oh!

jaeopt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msohailhussain commented Apr 26, 2021

Uh oh!

coveralls commented Apr 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msohailhussain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaeopt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msohailhussain left a comment

Choose a reason for hiding this comment

Uh oh!

jaeopt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ozayr-zaviar commented Apr 23, 2021 •

edited

Loading

coveralls commented Apr 27, 2021 •

edited

Loading