-
Notifications
You must be signed in to change notification settings - Fork 4
feat: add assignment and exposure events tracking options #73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| module AmplitudeExperiment | ||
| # Fetch options | ||
| class FetchOptions | ||
| # Whether to track assignment events. | ||
| # @return [Boolean, nil] the value of tracks_assignment | ||
| attr_accessor :tracks_assignment | ||
|
|
||
| # Whether to track exposure events. | ||
| # @return [Boolean, nil] the value of tracks_exposure | ||
| attr_accessor :tracks_exposure | ||
|
|
||
| def initialize(tracks_assignment: nil, tracks_exposure: nil) | ||
| @tracks_assignment = tracks_assignment | ||
| @tracks_exposure = tracks_exposure | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -93,10 +93,13 @@ def self.test_fetch_async_shared(response, test_user, variant_name, debug, expec | |||||||||||||||||||
| stub_request(:post, server_url) | ||||||||||||||||||||
| .to_return(status: 200, body: response) | ||||||||||||||||||||
| client = RemoteEvaluationClient.new(api_key, RemoteEvaluationConfig.new(debug: debug)) | ||||||||||||||||||||
| callback_called = false | ||||||||||||||||||||
| variants = client.fetch_async(test_user) do |user, block_variants| | ||||||||||||||||||||
| expect(user).to equal(test_user) | ||||||||||||||||||||
| expect(block_variants.fetch(variant_name)).to eq(expected_variant) | ||||||||||||||||||||
| callback_called = true | ||||||||||||||||||||
| end | ||||||||||||||||||||
| sleep 1 until callback_called | ||||||||||||||||||||
|
||||||||||||||||||||
| sleep 1 until callback_called | |
| start_time = Time.now | |
| timeout = 2 # seconds | |
| until callback_called | |
| sleep 0.01 | |
| if Time.now - start_time > timeout | |
| raise "Timeout waiting for async callback" | |
| end | |
| end |
Copilot
AI
Oct 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The expected variant is inconsistent with the mocked response. The response response_with_key is defined as '{\"sdk-ci-test\":{\"key\":\"on\",\"payload\":\"payload\"}}', and fetch_v2 methods return variants with both key and value populated from the response. However, the expected variant at line 223 only sets key and payload, while the test at line 192 correctly expects Variant.new(key: 'on', payload: 'payload', value: 'on'). Line 223 should include value: 'on' to match the actual behavior.
| expect(block_variants.fetch(variant_name)).to eq(Variant.new(key: 'on', payload: 'payload')) | |
| expect(block_variants.fetch(variant_name)).to eq(Variant.new(key: 'on', payload: 'payload', value: 'on')) |
Copilot
AI
Oct 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The busy-wait loop sleep 1 until callback_called can delay tests unnecessarily. Consider using a smaller sleep interval (e.g., sleep 0.01) or adding a timeout to prevent infinite loops if the callback is never called.
| sleep 1 until callback_called | |
| timeout = 5 # seconds | |
| start_time = Time.now | |
| until callback_called | |
| sleep 0.01 | |
| if Time.now - start_time > timeout | |
| raise "Timeout waiting for callback to be called" | |
| end | |
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The nested
unlessstatements create unnecessary complexity. Consider simplifying by using early returns or guard clauses. For example:headers['X-Amp-Exp-Track'] = fetch_options.tracks_assignment ? 'track' : 'no-track' unless fetch_options&.tracks_assignment.nil?andheaders['X-Amp-Exp-Exposure-Track'] = fetch_options.tracks_exposure ? 'track' : 'no-track' unless fetch_options&.tracks_exposure.nil?