Skip to content

Commit fd81a29

Browse files
FarhanAnjum-optiFarhanAnjum-opti
committed
refactor: streamline fetch_decision method and enhance test cases for improved clarity and functionality
1 parent 8c8573a commit fd81a29

File tree

2 files changed

+146
-87
lines changed

2 files changed

+146
-87
lines changed

‎optimizely/cmab/cmab_client.py‎

Lines changed: 5 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -98,18 +98,11 @@ def fetch_decision(
9898
"cmabUUID": cmab_uuid,
9999
}]
100100
}
101-
102-
try:
103-
if self.retry_config:
104-
variation_id = self._do_fetch_with_retry(url, request_body, self.retry_config, timeout)
105-
else:
106-
variation_id = self._do_fetch(url, request_body, timeout)
107-
return variation_id
108-
109-
except requests.RequestException as e:
110-
error_message = Errors.CMAB_FETCH_FAILED.format(str(e))
111-
self.logger.error(error_message)
112-
raise CmabFetchError(error_message)
101+
if self.retry_config:
102+
variation_id = self._do_fetch_with_retry(url, request_body, self.retry_config, timeout)
103+
else:
104+
variation_id = self._do_fetch(url, request_body, timeout)
105+
return variation_id
113106

114107
def _do_fetch(self, url: str, request_body: Dict[str, Any], timeout: float) -> str:
115108
"""Perform a single fetch request to the CMAB prediction service.

‎tests/test_cmab_client.py‎

Lines changed: 141 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -1,169 +1,235 @@
11
import unittest
22
import json
3-
from unittest.mock import MagicMock, patch
3+
from unittest.mock import MagicMock, patch, call
44
from optimizely.cmab.cmab_client import DefaultCmabClient, CmabRetryConfig
55
from requests.exceptions import RequestException
66
from optimizely.helpers.enums import Errors
77
from optimizely.exceptions import CmabFetchError, CmabInvalidResponseError
88

99

10-
class TestDefaultCmabClient_do_fetch(unittest.TestCase):
10+
class TestDefaultCmabClient(unittest.TestCase):
1111
def setUp(self):
1212
self.mock_http_client = MagicMock()
1313
self.mock_logger = MagicMock()
14-
self.client = DefaultCmabClient(http_client=self.mock_http_client, logger=self.mock_logger)
14+
self.retry_config = CmabRetryConfig(max_retries=3, initial_backoff=0.01, max_backoff=1, backoff_multiplier=2)
15+
self.client = DefaultCmabClient(
16+
http_client=self.mock_http_client,
17+
logger=self.mock_logger,
18+
retry_config=None
19+
)
20+
self.rule_id = 'test_rule'
21+
self.user_id = 'user123'
22+
self.attributes = {'attr1': 'value1', 'attr2': 'value2'}
23+
self.cmab_uuid = 'uuid-1234'
24+
self.expected_url = f"https://prediction.cmab.optimizely.com/predict/{self.rule_id}"
25+
self.expected_body = {
26+
"instances": [{
27+
"visitorId": self.user_id,
28+
"experimentId": self.rule_id,
29+
"attributes": [
30+
{"id": "attr1", "value": "value1", "type": "custom_attribute"},
31+
{"id": "attr2", "value": "value2", "type": "custom_attribute"}
32+
],
33+
"cmabUUID": self.cmab_uuid,
34+
}]
35+
}
36+
self.expected_headers = {'Content-Type': 'application/json'}
1537

16-
def test_do_fetch_success(self):
38+
def test_fetch_decision_returns_success_no_retry(self):
1739
mock_response = MagicMock()
1840
mock_response.status_code = 200
1941
mock_response.json.return_value = {
2042
'predictions': [{'variation_id': 'abc123'}]
2143
}
2244
self.mock_http_client.post.return_value = mock_response
23-
24-
result = self.client._do_fetch('http://fake-url', {'some': 'data'}, 1.0)
45+
result = self.client.fetch_decision(self.rule_id, self.user_id, self.attributes, self.cmab_uuid)
2546
self.assertEqual(result, 'abc123')
47+
self.mock_http_client.post.assert_called_once_with(
48+
self.expected_url,
49+
data=json.dumps(self.expected_body),
50+
headers=self.expected_headers,
51+
timeout=10.0
52+
)
2653

27-
def test_do_fetch_http_exception(self):
54+
def test_fetch_decision_returns_http_exception_no_retry(self):
2855
self.mock_http_client.post.side_effect = RequestException('Connection error')
2956

3057
with self.assertRaises(CmabFetchError) as context:
31-
self.client._do_fetch('http://fake-url', {'some': 'data'}, 1.0)
58+
self.client.fetch_decision(self.rule_id, self.user_id, self.attributes, self.cmab_uuid)
3259

60+
self.mock_http_client.post.assert_called_once()
3361
self.mock_logger.error.assert_called_with(Errors.CMAB_FETCH_FAILED.format('Connection error'))
3462
self.assertIn('Connection error', str(context.exception))
3563

36-
def test_do_fetch_non_2xx_status(self):
64+
def test_fetch_decision_returns_non_2xx_status_no_retry(self):
3765
mock_response = MagicMock()
3866
mock_response.status_code = 500
3967
self.mock_http_client.post.return_value = mock_response
4068

4169
with self.assertRaises(CmabFetchError) as context:
42-
self.client._do_fetch('http://fake-url', {'some': 'data'}, 1.0)
70+
self.client.fetch_decision(self.rule_id, self.user_id, self.attributes, self.cmab_uuid)
4371

72+
self.mock_http_client.post.assert_called_once_with(
73+
self.expected_url,
74+
data=json.dumps(self.expected_body),
75+
headers=self.expected_headers,
76+
timeout=10.0
77+
)
4478
self.mock_logger.error.assert_called_with(Errors.CMAB_FETCH_FAILED.format(str(mock_response.status_code)))
4579
self.assertIn(str(mock_response.status_code), str(context.exception))
4680

47-
def test_do_fetch_invalid_json(self):
81+
def test_fetch_decision_returns_invalid_json_no_retry(self):
4882
mock_response = MagicMock()
4983
mock_response.status_code = 200
5084
mock_response.json.side_effect = json.JSONDecodeError("Expecting value", "", 0)
5185
self.mock_http_client.post.return_value = mock_response
5286

5387
with self.assertRaises(CmabInvalidResponseError) as context:
54-
self.client._do_fetch('http://fake-url', {'some': 'data'}, 1.0)
88+
self.client.fetch_decision(self.rule_id, self.user_id, self.attributes, self.cmab_uuid)
5589

90+
self.mock_http_client.post.assert_called_once_with(
91+
self.expected_url,
92+
data=json.dumps(self.expected_body),
93+
headers=self.expected_headers,
94+
timeout=10.0
95+
)
5696
self.mock_logger.error.assert_called_with(Errors.INVALID_CMAB_FETCH_RESPONSE)
5797
self.assertIn(Errors.INVALID_CMAB_FETCH_RESPONSE, str(context.exception))
5898

59-
def test_do_fetch_invalid_response_structure(self):
99+
def test_fetch_decision_returns_invalid_response_structure_no_retry(self):
60100
mock_response = MagicMock()
61101
mock_response.status_code = 200
62102
mock_response.json.return_value = {'no_predictions': []}
63103
self.mock_http_client.post.return_value = mock_response
64104

65105
with self.assertRaises(CmabInvalidResponseError) as context:
66-
self.client._do_fetch('http://fake-url', {'some': 'data'}, 1.0)
106+
self.client.fetch_decision(self.rule_id, self.user_id, self.attributes, self.cmab_uuid)
67107

108+
self.mock_http_client.post.assert_called_once_with(
109+
self.expected_url,
110+
data=json.dumps(self.expected_body),
111+
headers=self.expected_headers,
112+
timeout=10.0
113+
)
68114
self.mock_logger.error.assert_called_with(Errors.INVALID_CMAB_FETCH_RESPONSE)
69115
self.assertIn(Errors.INVALID_CMAB_FETCH_RESPONSE, str(context.exception))
70116

71-
72-
class TestDefaultCmabClientWithRetry(unittest.TestCase):
73-
def setUp(self):
74-
self.mock_http_client = MagicMock()
75-
self.mock_logger = MagicMock()
76-
self.retry_config = CmabRetryConfig(max_retries=2, initial_backoff=0.01, max_backoff=1, backoff_multiplier=2)
77-
self.client = DefaultCmabClient(
117+
@patch('time.sleep', return_value=None)
118+
def test_fetch_decision_returns_success_with_retry_on_first_try(self, mock_sleep):
119+
# Create client with retry
120+
client_with_retry = DefaultCmabClient(
78121
http_client=self.mock_http_client,
79122
logger=self.mock_logger,
80123
retry_config=self.retry_config
81124
)
82125

83-
@patch("time.sleep", return_value=None)
84-
def test_do_fetch_with_retry_success_on_first_try(self, _):
126+
# Mock successful response
85127
mock_response = MagicMock()
86128
mock_response.status_code = 200
87129
mock_response.json.return_value = {
88-
"predictions": [{"variation_id": "abc123"}]
130+
'predictions': [{'variation_id': 'abc123'}]
89131
}
90132
self.mock_http_client.post.return_value = mock_response
91133

92-
result = self.client._do_fetch_with_retry("http://fake-url", {}, self.retry_config, 1.0)
93-
self.assertEqual(result, "abc123")
134+
result = client_with_retry.fetch_decision(self.rule_id, self.user_id, self.attributes, self.cmab_uuid)
135+
136+
# Verify result and request parameters
137+
self.assertEqual(result, 'abc123')
138+
self.mock_http_client.post.assert_called_once_with(
139+
self.expected_url,
140+
data=json.dumps(self.expected_body),
141+
headers=self.expected_headers,
142+
timeout=10.0
143+
)
94144
self.assertEqual(self.mock_http_client.post.call_count, 1)
145+
mock_sleep.assert_not_called()
146+
147+
@patch('time.sleep', return_value=None)
148+
def test_fetch_decision_returns_success_with_retry_on_third_try(self, mock_sleep):
149+
client_with_retry = DefaultCmabClient(
150+
http_client=self.mock_http_client,
151+
logger=self.mock_logger,
152+
retry_config=self.retry_config
153+
)
95154

96-
@patch("time.sleep", return_value=None)
97-
def test_do_fetch_with_retry_success_on_retry(self, _):
98-
# First call fails, second call succeeds
155+
# Create failure and success responses
99156
failure_response = MagicMock()
100157
failure_response.status_code = 500
101158

102159
success_response = MagicMock()
103160
success_response.status_code = 200
104161
success_response.json.return_value = {
105-
"predictions": [{"variation_id": "xyz456"}]
162+
'predictions': [{'variation_id': 'xyz456'}]
106163
}
107164

165+
# First two calls fail, third succeeds
108166
self.mock_http_client.post.side_effect = [
167+
failure_response,
109168
failure_response,
110169
success_response
111170
]
112171

113-
result = self.client._do_fetch_with_retry("http://fake-url", {}, self.retry_config, 1.0)
114-
self.assertEqual(result, "xyz456")
115-
self.assertEqual(self.mock_http_client.post.call_count, 2)
116-
self.mock_logger.info.assert_called_with("Retrying CMAB request (attempt: 1) after 0.01 seconds...")
117-
118-
@patch("time.sleep", return_value=None)
119-
def test_do_fetch_with_retry_exhausts_all_attempts(self, _):
120-
failure_response = MagicMock()
121-
failure_response.status_code = 500
122-
123-
self.mock_http_client.post.return_value = failure_response
124-
125-
with self.assertRaises(CmabFetchError):
126-
self.client._do_fetch_with_retry("http://fake-url", {}, self.retry_config, 1.0)
127-
128-
self.assertEqual(self.mock_http_client.post.call_count, 3) # 1 original + 2 retries
129-
self.mock_logger.error.assert_called_with(
130-
Errors.CMAB_FETCH_FAILED.format("Exhausted all retries for CMAB request."))
172+
result = client_with_retry.fetch_decision(self.rule_id, self.user_id, self.attributes, self.cmab_uuid)
131173

174+
self.assertEqual(result, 'xyz456')
175+
self.assertEqual(self.mock_http_client.post.call_count, 3)
132176

133-
class TestDefaultCmabClientFetchDecision(unittest.TestCase):
134-
def setUp(self):
135-
self.mock_http_client = MagicMock()
136-
self.mock_logger = MagicMock()
137-
self.retry_config = CmabRetryConfig(max_retries=2, initial_backoff=0.01, max_backoff=1, backoff_multiplier=2)
138-
self.client = DefaultCmabClient(
139-
http_client=self.mock_http_client,
140-
logger=self.mock_logger,
141-
retry_config=None
177+
# Verify all HTTP calls used correct parameters
178+
self.mock_http_client.post.assert_called_with(
179+
self.expected_url,
180+
data=json.dumps(self.expected_body),
181+
headers=self.expected_headers,
182+
timeout=10.0
142183
)
143-
self.rule_id = 'test_rule'
144-
self.user_id = 'user123'
145-
self.attributes = {'attr1': 'value1'}
146-
self.cmab_uuid = 'uuid-1234'
147184

148-
@patch.object(DefaultCmabClient, '_do_fetch', return_value='var-abc')
149-
def test_fetch_decision_success_no_retry(self, mock_do_fetch):
150-
result = self.client.fetch_decision(self.rule_id, self.user_id, self.attributes, self.cmab_uuid)
151-
self.assertEqual(result, 'var-abc')
152-
mock_do_fetch.assert_called_once()
185+
# Verify retry logging
186+
self.mock_logger.info.assert_has_calls([
187+
call("Retrying CMAB request (attempt: 1) after 0.01 seconds..."),
188+
call("Retrying CMAB request (attempt: 2) after 0.02 seconds...")
189+
])
153190

154-
@patch.object(DefaultCmabClient, '_do_fetch_with_retry', return_value='var-xyz')
155-
def test_fetch_decision_success_with_retry(self, mock_do_fetch_with_retry):
191+
# Verify sleep was called with correct backoff times
192+
mock_sleep.assert_has_calls([
193+
call(0.01),
194+
call(0.02)
195+
])
196+
197+
@patch('time.sleep', return_value=None)
198+
def test_fetch_decision_exhausts_all_retry_attempts(self, mock_sleep):
156199
client_with_retry = DefaultCmabClient(
157200
http_client=self.mock_http_client,
158201
logger=self.mock_logger,
159202
retry_config=self.retry_config
160203
)
161-
result = client_with_retry.fetch_decision(self.rule_id, self.user_id, self.attributes, self.cmab_uuid)
162-
self.assertEqual(result, 'var-xyz')
163-
mock_do_fetch_with_retry.assert_called_once()
164204

165-
@patch.object(DefaultCmabClient, '_do_fetch', side_effect=RequestException("Network error"))
166-
def test_fetch_decision_request_exception(self, mock_do_fetch):
205+
# Create failure response
206+
failure_response = MagicMock()
207+
failure_response.status_code = 500
208+
209+
# All attempts fail
210+
self.mock_http_client.post.return_value = failure_response
211+
167212
with self.assertRaises(CmabFetchError):
168-
self.client.fetch_decision(self.rule_id, self.user_id, self.attributes, self.cmab_uuid)
169-
self.mock_logger.error.assert_called_with(Errors.CMAB_FETCH_FAILED.format("Network error"))
213+
client_with_retry.fetch_decision(self.rule_id, self.user_id, self.attributes, self.cmab_uuid)
214+
215+
# Verify all attempts were made (1 initial + 3 retries)
216+
self.assertEqual(self.mock_http_client.post.call_count, 4)
217+
218+
# Verify retry logging
219+
self.mock_logger.info.assert_has_calls([
220+
call("Retrying CMAB request (attempt: 1) after 0.01 seconds..."),
221+
call("Retrying CMAB request (attempt: 2) after 0.02 seconds..."),
222+
call("Retrying CMAB request (attempt: 3) after 0.08 seconds...")
223+
])
224+
225+
# Verify sleep was called for each retry
226+
mock_sleep.assert_has_calls([
227+
call(0.01),
228+
call(0.02),
229+
call(0.08)
230+
])
231+
232+
# Verify final error
233+
self.mock_logger.error.assert_called_with(
234+
Errors.CMAB_FETCH_FAILED.format('Exhausted all retries for CMAB request.')
235+
)

0 commit comments

Comments
 (0)