fix: adroll bugsnag issue #1929
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces a comprehensive suite of unit tests for the Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@shrouti1507 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1929 +/- ##
===========================================
+ Coverage 57.54% 57.83% +0.29%
===========================================
Files 485 485
Lines 16506 16506
Branches 3303 3315 +12
===========================================
+ Hits 9498 9547 +49
+ Misses 5743 5695 -48
+ Partials 1265 1264 -1 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
packages/analytics-js-integrations/__tests__/integrations/Adroll/browser.test.js (3)
20-29: Enhance initialization test coverageThe initialization tests only cover the happy path. Consider adding tests for:
- Error cases with missing or invalid advId/pixId
- Verification of script injection
- Multiple initialization attempts
Example test cases to add:
test('should throw error when advId is missing', () => { expect(() => new Adroll({ pixId: 'TEST_PIX_ID' })).toThrow(); }); test('should inject adroll script', () => { adroll.init(); const script = document.querySelector('script[src*="adroll"]'); expect(script).toBeTruthy(); });
54-96: Add validation tests for identify methodWhile the current tests cover the basic functionality, consider adding:
- Email format validation tests
- User ID tracking tests
- Tests for handling special characters in email
Example test cases to add:
test('should handle invalid email format', () => { adroll.identify({ message: { traits: { email: 'invalid-email' } } }); expect(window.__adroll.record_adroll_email).not.toHaveBeenCalled(); }); test('should track user ID if available', () => { adroll.identify({ message: { userId: 'user123', traits: { email: '[email protected]' } } }); expect(window._adroll_user_id).toBe('user123'); });
166-211: Enhance page tracking test coverageThe current tests only cover basic page view tracking. Consider adding tests for:
- Different URL formats and components
- Page properties (title, referrer, search)
- Error cases in URL handling
Example test cases to add:
test('should handle complex URLs with query parameters', () => { Object.defineProperty(window, 'location', { value: { pathname: '/products', href: 'https://test.com/products?id=123', search: '?id=123' }, writable: true }); adroll.page({ message: { name: 'Products', properties: { title: 'Product List', referrer: 'https://test.com' } } }); expect(window.__adroll.record_user).toHaveBeenCalledWith( expect.objectContaining({ path: '/products', search: '?id=123', title: 'Product List', referrer: 'https://test.com' }) ); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/analytics-js-integrations/__tests__/integrations/Adroll/browser.test.js(1 hunks)packages/analytics-js-integrations/src/integrations/Adroll/browser.js(1 hunks)
🧰 Additional context used
🪛 Biome
packages/analytics-js-integrations/src/integrations/Adroll/browser.js
[error] 75-75: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 GitHub Check: codecov/patch
packages/analytics-js-integrations/src/integrations/Adroll/browser.js
[warning] 77-77: packages/analytics-js-integrations/src/integrations/Adroll/browser.js#L77
Added line #L77 was not covered by tests
🔇 Additional comments (4)
packages/analytics-js-integrations/src/integrations/Adroll/browser.js (3)
75-75: LGTM: Delete operator usage is appropriate here
While static analysis flags the delete operator for performance reasons, its usage here is justified as:
- It's in a conditional block (not frequently called)
- It's modifying a temporary object
- Using
undefinedassignment would leave an enumerable property, which might affect AdRoll's processing
🧰 Tools
🪛 Biome
[error] 75-75: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
64-64: LGTM: Robust fix for Bugsnag alerts
The addition of .toString() before .toLowerCase() calls is a solid fix that will prevent Bugsnag alerts by ensuring type safety when handling events. The implementation is consistent across all event checks and maintains the existing functionality while making it more robust.
Also applies to: 68-68, 70-70, 77-77
77-79: Verify test coverage for custom event mapping
This code path handles custom event mapping to AdRoll segments, but line 77 is reported as not covered by tests. Please ensure there are test cases that:
- Map a custom event to a segmentId
- Verify the correct assignment of
adroll_segments
✅ Verification successful
Let me gather more context about the test coverage by checking the actual test implementations.
Test coverage for custom event mapping is properly implemented
The test file includes comprehensive coverage for custom event mapping:
- The
Track testssuite includes setup of custom event mapping:{ from: 'custom event', to: 'ghi789' } - Tests verify correct
adroll_segmentsassignment for different event types:- Product events (
abc123) - Order events (
def456) - Page events (
page123)
- Product events (
- The implementation includes test cases for both mapped and unmapped events
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check test coverage for custom event mapping
# Look for test cases in the test file
rg -A 5 "custom.*event" "packages/analytics-js-integrations/__tests__/integrations/Adroll/browser.test.js"
# Check if there are tests verifying segmentId assignment
rg -A 5 "adroll_segments" "packages/analytics-js-integrations/__tests__/integrations/Adroll/browser.test.js"
Length of output: 634
Script:
#!/bin/bash
# Check the full test file content to understand the test coverage
cat "packages/analytics-js-integrations/__tests__/integrations/Adroll/browser.test.js"
Length of output: 5397
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 77-77: packages/analytics-js-integrations/src/integrations/Adroll/browser.js#L77
Added line #L77 was not covered by tests
packages/analytics-js-integrations/__tests__/integrations/Adroll/browser.test.js (1)
31-52: LGTM! Comprehensive loading state tests
The tests thoroughly cover both positive and negative cases for loading states, including the relationship between isLoaded and isReady.
packages/analytics-js-integrations/__tests__/integrations/Adroll/browser.test.js
Outdated
Show resolved
Hide resolved
packages/analytics-js-integrations/__tests__/integrations/Adroll/browser.test.js
Outdated
Show resolved
Hide resolved
size-limit report 📦
|
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/analytics-js-integrations/__tests__/integrations/Adroll/browser.test.js (1)
208-215: Restorewindow.locationafter modifying it in testsIn the
beforeEachhook of the "Page tests" suite,window.locationis being overridden. To prevent potential side effects on other tests, it's advisable to store the originalwindow.locationand restore it in theafterEachhook.Consider applying this change:
+ let originalLocation; beforeEach(() => { + originalLocation = window.location; Object.defineProperty(window, 'location', { value: { pathname: '/', href: 'https://www.test-host.com/', }, writable: true, }); }); + afterEach(() => { + window.location = originalLocation; + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/analytics-js-integrations/__tests__/integrations/Adroll/browser.test.js(1 hunks)
🧰 Additional context used
🪛 Biome
packages/analytics-js-integrations/__tests__/integrations/Adroll/browser.test.js
[error] 16-16: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 17-17: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/analytics-js-integrations/__tests__/integrations/Adroll/browser.test.js
Show resolved
Hide resolved
PR Description
Please include a summary of the change along with the relevant motivation and context.
Linear task (optional)
https://linear.app/rudderstack/issue/INT-2873/bugsnag-alerts
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security
Summary by CodeRabbit
New Features
Adrollintegration, covering initialization, loading states, user identification, event tracking, and page tracking.Bug Fixes
trackmethod to ensure event variables are processed correctly by converting them to strings before applying transformations.Tests
Adrollintegration, ensuring robust performance and reliability.