Skip to content

Commit 6ca1102

Browse files
FarhanAnjum-optiFarhanAnjum-opti
committed
update: refactor get_variation return structure and change tests accordingly
1 parent 265d82b commit 6ca1102

File tree

2 files changed

+144
-56
lines changed

2 files changed

+144
-56
lines changed

‎tests/test_decision_service.py‎

Lines changed: 51 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -457,9 +457,10 @@ def test_get_variation__experiment_not_running(self):
457457
) as mock_lookup, mock.patch(
458458
"optimizely.user_profile.UserProfileService.save"
459459
) as mock_save:
460-
variation, _, _ = self.decision_service.get_variation(
460+
variation_result = self.decision_service.get_variation(
461461
self.project_config, experiment, user, None
462462
)
463+
variation = variation_result['variation']
463464
self.assertIsNone(
464465
variation
465466
)
@@ -500,7 +501,7 @@ def test_get_variation__bucketing_id_provided(self):
500501
"optimizely.bucketer.Bucketer.bucket",
501502
return_value=[self.project_config.get_variation_from_id("211127", "211129"), []],
502503
) as mock_bucket:
503-
variation, _, _ = self.decision_service.get_variation(
504+
_ = self.decision_service.get_variation(
504505
self.project_config,
505506
experiment,
506507
user,
@@ -535,9 +536,9 @@ def test_get_variation__user_whitelisted_for_variation(self):
535536
) as mock_lookup, mock.patch(
536537
"optimizely.user_profile.UserProfileService.save"
537538
) as mock_save:
538-
variation, _, _ = self.decision_service.get_variation(
539+
variation = self.decision_service.get_variation(
539540
self.project_config, experiment, user, user_profile_tracker
540-
)
541+
)['variation']
541542
self.assertEqual(
542543
entities.Variation("111128", "control"),
543544
variation,
@@ -573,9 +574,9 @@ def test_get_variation__user_has_stored_decision(self):
573574
) as mock_audience_check, mock.patch(
574575
"optimizely.bucketer.Bucketer.bucket"
575576
) as mock_bucket:
576-
variation, _, _ = self.decision_service.get_variation(
577+
variation = self.decision_service.get_variation(
577578
self.project_config, experiment, user, user_profile_tracker
578-
)
579+
)['variation']
579580
self.assertEqual(
580581
entities.Variation("111128", "control"),
581582
variation,
@@ -619,9 +620,9 @@ def test_get_variation__user_bucketed_for_new_experiment__user_profile_tracker_a
619620
"optimizely.bucketer.Bucketer.bucket",
620621
return_value=[entities.Variation("111129", "variation"), []],
621622
) as mock_bucket:
622-
variation, _, _ = self.decision_service.get_variation(
623+
variation = self.decision_service.get_variation(
623624
self.project_config, experiment, user, user_profile_tracker
624-
)
625+
)['variation']
625626
self.assertEqual(
626627
entities.Variation("111129", "variation"),
627628
variation,
@@ -669,9 +670,9 @@ def test_get_variation__user_does_not_meet_audience_conditions(self):
669670
) as mock_bucket, mock.patch(
670671
"optimizely.user_profile.UserProfileService.save"
671672
) as mock_save:
672-
variation, _, _ = self.decision_service.get_variation(
673+
variation = self.decision_service.get_variation(
673674
self.project_config, experiment, user, user_profile_tracker
674-
)
675+
)['variation']
675676
self.assertIsNone(
676677
variation
677678
)
@@ -719,14 +720,14 @@ def test_get_variation__ignore_user_profile_when_specified(self):
719720
) as mock_lookup, mock.patch(
720721
"optimizely.user_profile.UserProfileService.save"
721722
) as mock_save:
722-
variation, _, _ = self.decision_service.get_variation(
723+
variation = self.decision_service.get_variation(
723724
self.project_config,
724725
experiment,
725726
user,
726727
user_profile_tracker,
727728
[],
728729
options=['IGNORE_USER_PROFILE_SERVICE'],
729-
)
730+
)['variation']
730731
self.assertEqual(
731732
entities.Variation("111129", "variation"),
732733
variation,
@@ -796,16 +797,22 @@ def test_get_variation_cmab_experiment_user_in_traffic_allocation(self):
796797
'logger') as mock_logger:
797798

798799
# Call get_variation with the CMAB experiment
799-
variation, reasons, cmab_uuid = self.decision_service.get_variation(
800+
variation_result = self.decision_service.get_variation(
800801
self.project_config,
801802
cmab_experiment,
802803
user,
803804
None
804805
)
806+
cmab_uuid = variation_result['cmab_uuid']
807+
variation = variation_result['variation']
808+
error = variation_result['error']
809+
reasons = variation_result['reasons']
805810

806811
# Verify the variation and cmab_uuid
807812
self.assertEqual(entities.Variation('111151', 'variation_1'), variation)
808813
self.assertEqual('test-cmab-uuid-123', cmab_uuid)
814+
self.assertStrictFalse(error)
815+
self.assertIn('User "test_user" is in variation "variation_1" of experiment cmab_experiment.', reasons)
809816

810817
# Verify logger was called
811818
mock_logger.info.assert_any_call('User "test_user" is in variation '
@@ -844,16 +851,23 @@ def test_get_variation_cmab_experiment_user_not_in_traffic_allocation(self):
844851
'logger') as mock_logger:
845852

846853
# Call get_variation with the CMAB experiment
847-
variation, reasons, cmab_uuid = self.decision_service.get_variation(
854+
variation_result = self.decision_service.get_variation(
848855
self.project_config,
849856
cmab_experiment,
850857
user,
851858
None
852859
)
860+
variation = variation_result['variation']
861+
cmab_uuid = variation_result['cmab_uuid']
862+
error = variation_result['error']
863+
reasons = variation_result['reasons']
853864

854865
# Verify we get no variation and CMAB service wasn't called
855866
self.assertIsNone(variation)
856867
self.assertIsNone(cmab_uuid)
868+
self.assertStrictFalse(error)
869+
self.assertIn('User "test_user" not in CMAB experiment "cmab_experiment" due to traffic allocation.',
870+
reasons)
857871
mock_cmab_decision.assert_not_called()
858872

859873
# Verify logger was called
@@ -888,25 +902,25 @@ def test_get_variation_cmab_experiment_service_error(self):
888902
mock.patch('optimizely.helpers.audience.does_user_meet_audience_conditions', return_value=[True, []]), \
889903
mock.patch('optimizely.bucketer.Bucketer.bucket_to_entity_id', return_value=['$', []]), \
890904
mock.patch('optimizely.decision_service.DecisionService._get_decision_for_cmab_experiment',
891-
return_value={'error': True, 'result': None, 'reasons': ['CMAB service error']}), \
892-
mock.patch.object(self.decision_service,
893-
'logger') as mock_logger:
905+
return_value={'error': True, 'result': None, 'reasons': ['CMAB service error']}):
894906

895907
# Call get_variation with the CMAB experiment
896-
variation, reasons, cmab_uuid = self.decision_service.get_variation(
908+
variation_result = self.decision_service.get_variation(
897909
self.project_config,
898910
cmab_experiment,
899911
user,
900912
None
901913
)
914+
variation = variation_result['variation']
915+
cmab_uuid = variation_result['cmab_uuid']
916+
reasons = variation_result['reasons']
917+
error = variation_result['error']
902918

903919
# Verify we get no variation due to CMAB service error
904920
self.assertIsNone(variation)
905921
self.assertIsNone(cmab_uuid)
906922
self.assertIn('CMAB service error', reasons)
907-
908-
# Verify logger was called
909-
mock_logger.error.assert_any_call('CMAB decision fetch failed with status: CMAB service error')
923+
self.assertStrictTrue(error)
910924

911925
def test_get_variation_cmab_experiment_deep_mock_500_error(self):
912926
"""Test the full flow of a CMAB experiment with a 500 error from the HTTP request layer."""
@@ -1015,17 +1029,22 @@ def test_get_variation_cmab_experiment_forced_variation(self):
10151029
) as mock_cmab_decision:
10161030

10171031
# Call get_variation with the CMAB experiment
1018-
variation, reasons, cmab_uuid = self.decision_service.get_variation(
1032+
variation_result = self.decision_service.get_variation(
10191033
self.project_config,
10201034
cmab_experiment,
10211035
user,
10221036
None
10231037
)
1038+
variation = variation_result['variation']
1039+
reasons = variation_result['reasons']
1040+
cmab_uuid = variation_result['cmab_uuid']
1041+
error = variation_result['error']
10241042

10251043
# Verify we get the forced variation
10261044
self.assertEqual(forced_variation, variation)
10271045
self.assertIsNone(cmab_uuid)
10281046
self.assertIn('User is forced into variation', reasons)
1047+
self.assertStrictFalse(error)
10291048

10301049
# Verify CMAB-specific methods weren't called
10311050
mock_bucket.assert_not_called()
@@ -1072,17 +1091,22 @@ def test_get_variation_cmab_experiment_with_whitelisted_variation(self):
10721091
) as mock_cmab_decision:
10731092

10741093
# Call get_variation with the CMAB experiment
1075-
variation, reasons, cmab_uuid = self.decision_service.get_variation(
1094+
variation_result = self.decision_service.get_variation(
10761095
self.project_config,
10771096
cmab_experiment,
10781097
user,
10791098
None
10801099
)
1100+
variation = variation_result['variation']
1101+
cmab_uuid = variation_result['cmab_uuid']
1102+
reasons = variation_result['reasons']
1103+
error = variation_result['error']
10811104

10821105
# Verify we get the whitelisted variation
10831106
self.assertEqual(whitelisted_variation, variation)
10841107
self.assertIsNone(cmab_uuid)
10851108
self.assertIn('User is whitelisted into variation', reasons)
1109+
self.assertStrictFalse(error)
10861110

10871111
# Verify CMAB-specific methods weren't called
10881112
mock_bucket.assert_not_called()
@@ -1353,7 +1377,7 @@ def test_get_variation_for_feature__returns_variation_for_feature_in_experiment(
13531377
)
13541378
decision_patch = mock.patch(
13551379
"optimizely.decision_service.DecisionService.get_variation",
1356-
return_value=[expected_variation, [], None],
1380+
return_value={'variation': expected_variation, 'cmab_uuid': None, 'reasons': [], 'error': False},
13571381
)
13581382
with decision_patch as mock_decision, self.mock_decision_logger:
13591383
variation_received, _ = self.decision_service.get_variation_for_feature(
@@ -1485,7 +1509,7 @@ def test_get_variation_for_feature__returns_variation_for_feature_in_group(self)
14851509
)
14861510
with mock.patch(
14871511
"optimizely.decision_service.DecisionService.get_variation",
1488-
return_value=(expected_variation, [], None),
1512+
return_value={'variation': expected_variation, 'cmab_uuid': None, 'reasons': [], 'error': False},
14891513
) as mock_decision:
14901514
variation_received, _ = self.decision_service.get_variation_for_feature(
14911515
self.project_config, feature, user, options=None
@@ -1520,7 +1544,7 @@ def test_get_variation_for_feature__returns_none_for_user_not_in_experiment(self
15201544

15211545
with mock.patch(
15221546
"optimizely.decision_service.DecisionService.get_variation",
1523-
return_value=[None, [], None],
1547+
return_value={'variation': None, 'cmab_uuid': None, 'reasons': [], 'error': False},
15241548
) as mock_decision:
15251549
variation_received, _ = self.decision_service.get_variation_for_feature(
15261550
self.project_config, feature, user
@@ -1552,7 +1576,7 @@ def test_get_variation_for_feature__returns_none_for_user_in_group_experiment_no
15521576
feature = self.project_config.get_feature_from_key("test_feature_in_group")
15531577
with mock.patch(
15541578
"optimizely.decision_service.DecisionService.get_variation",
1555-
return_value=[None, [], None],
1579+
return_value={'variation': None, 'cmab_uuid': None, 'reasons': [], 'error': False},
15561580
) as mock_decision:
15571581
variation_received, _ = self.decision_service.get_variation_for_feature(
15581582
self.project_config, feature, user, False

0 commit comments

Comments
 (0)