[FSSDK-11149] Ruby: Implement CMAB Client #364
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request implements a new CMAB client in Ruby along with unit tests to ensure correct behavior for various response scenarios. Key changes include:
- Implementation of the CMAB client with retry logic and HTTP error handling.
- Addition of new unit tests to confirm client behavior under normal and error conditions.
- Introduction of custom constants and exception classes for CMAB error management.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| spec/cmab_client_spec.rb | Provides comprehensive tests for CMAB client functionality |
| lib/optimizely/helpers/constants.rb | Adds CMAB-specific constants for error messages |
| lib/optimizely/exceptions.rb | Introduces custom CMAB error exception classes |
| lib/optimizely/cmab/cmab_client.rb | Implements the CMAB client with request, retry, and response logic |
lib/optimizely/cmab/cmab_client.rb
Outdated
| # Contains parameters for maximum retries, backoff intervals, and multipliers. | ||
| attr_reader :max_retries, :initial_backoff, :max_backoff, :backoff_multiplier | ||
|
|
||
| def initialize(max_retries: DEFAULT_MAX_RETRIES, initial_backoff: DEFAULT_INITIAL_BACKOFF, max_backoff: DEFAULT_BACKOFF_MULTIPLIER, backoff_multiplier: DEFAULT_BACKOFF_MULTIPLIER) |
There was a problem hiding this comment.
The default value for 'max_backoff' is incorrectly set to DEFAULT_BACKOFF_MULTIPLIER; it should likely reference DEFAULT_MAX_BACKOFF to correctly reflect the intended maximum backoff duration.
| def initialize(max_retries: DEFAULT_MAX_RETRIES, initial_backoff: DEFAULT_INITIAL_BACKOFF, max_backoff: DEFAULT_BACKOFF_MULTIPLIER, backoff_multiplier: DEFAULT_BACKOFF_MULTIPLIER) | |
| def initialize(max_retries: DEFAULT_MAX_RETRIES, initial_backoff: DEFAULT_INITIAL_BACKOFF, max_backoff: DEFAULT_MAX_BACKOFF, backoff_multiplier: DEFAULT_BACKOFF_MULTIPLIER) |
Copilot uses AI. Check for mistakes.
lib/optimizely/cmab/cmab_client.rb
Outdated
| error_message = Optimizely::Helpers::Constants::CMAB_FETCH_FAILED % 'Exhausted all retries for CMAB request.' | ||
| @logger.log(Logger::ERROR, error_message) | ||
| raise Optimizely::CmabFetchError, error_message |
There was a problem hiding this comment.
[nitpick] The error message and subsequent code after the retry loop appear unreachable since the loop already raises an error when max retries are exceeded; consider removing this redundant code.
| error_message = Optimizely::Helpers::Constants::CMAB_FETCH_FAILED % 'Exhausted all retries for CMAB request.' | |
| @logger.log(Logger::ERROR, error_message) | |
| raise Optimizely::CmabFetchError, error_message | |
| # Removed unreachable code after the retry loop. |
Copilot uses AI. Check for mistakes.
…esra/FSSDK-11149_add_cmab_client
lib/optimizely/cmab/cmab_client.rb
Outdated
| Kernel.sleep(backoff) | ||
|
|
||
| backoff = [ | ||
| backoff * (retry_config.backoff_multiplier**(attempt + 1)), |
There was a problem hiding this comment.
as we are using the previous backoff to get the new one, it should be
backoff = [
backoff * retry_config.backoff_multiplier,
retry_config.max_backoff
].min
spec/cmab_client_spec.rb
Outdated
| expect(Kernel).not_to have_received(:sleep) | ||
| end | ||
|
|
||
| it 'should not return 200 status' do |
There was a problem hiding this comment.
maybe rename it to should raise error non non success status
spec/cmab_client_spec.rb
Outdated
| expect(Kernel).not_to have_received(:sleep) | ||
| end | ||
|
|
||
| it 'should return invalid json' do |
There was a problem hiding this comment.
how about should raise error on invalid json response?
spec/cmab_client_spec.rb
Outdated
| expect(Kernel).not_to have_received(:sleep) | ||
| end | ||
|
|
||
| it 'should return HTTP exception' do |
There was a problem hiding this comment.
how about should raise error on http client exception
spec/cmab_client_spec.rb
Outdated
| expect(Kernel).not_to have_received(:sleep) | ||
| end | ||
|
|
||
| it 'should return invalid response structure' do |
There was a problem hiding this comment.
should raise error on invalid response structure
Summary
Test plan
Created new unit test cases
Issues