test(analytics): comprehensive integration and edge-case tests#3308
test(analytics): comprehensive integration and edge-case tests#3308PierreBrisorgueil merged 3 commits intomasterfrom
Conversation
|
Caution Review failedPull request was closed or merged during review WalkthroughUpdated skill documentation to clarify test requirements and timing thresholds. Added two new comprehensive Jest test suites for the analytics module covering middleware behavior, service initialization, event-driven workflows, feature flag integration, and failure resilience scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds expanded Jest coverage for the modules/analytics module, focusing on resilience behavior (SDK failures/shutdown) and broader “comprehensive” scenarios (Express middleware + init wiring + feature-flag edge cases).
Changes:
- Add resilience-focused unit tests for
analytics.service(sync/async SDK failures, shutdown safety, optional args). - Add a large “comprehensive” test suite intended to cover Express middleware behavior, module init wiring, and
requireFeatureFlagedge cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| modules/analytics/tests/analytics.service.resilience.unit.tests.js | New resilience/unit tests for AnalyticsService error propagation, shutdown safety, and argument forwarding. |
| modules/analytics/tests/analytics.comprehensive.unit.tests.js | New broad test suite covering intended middleware integration, init wiring, and feature-flag middleware edge cases. |
| // Second call — client is null, returns undefined synchronously | ||
| AnalyticsService.shutdown(); |
There was a problem hiding this comment.
Second shutdown() call is invoked without await/assertion. Since shutdown is an async function, this can leave an unresolved Promise (and can surface as an unhandled rejection if implementation changes). Await the second call (or await expect(...).resolves...).
| // Second call — client is null, returns undefined synchronously | |
| AnalyticsService.shutdown(); | |
| // Second call — client is null; still await in case implementation remains async | |
| await AnalyticsService.shutdown(); |
| jest.unstable_mockModule('../services/analytics.service.js', () => ({ | ||
| default: { | ||
| track: mockTrack, | ||
| init: jest.fn(), | ||
| identify: jest.fn(), | ||
| groupIdentify: jest.fn(), | ||
| shutdown: jest.fn(), | ||
| }, | ||
| })); | ||
|
|
||
| const mod = await import('../middlewares/analytics.middleware.js'); | ||
| analyticsMiddleware = mod.default; | ||
|
|
||
| app = express(); | ||
| app.use((req, _res, next) => { | ||
| req.user = { _id: 'user-int-1' }; | ||
| req.organization = { _id: 'org-int-1' }; | ||
| next(); | ||
| }); | ||
| app.use(analyticsMiddleware); | ||
|
|
There was a problem hiding this comment.
These tests import ../middlewares/analytics.middleware.js, but modules/analytics currently has no middlewares/ directory or analytics.middleware.js file, so this suite will fail at import time. Update the import path to the actual middleware location or add the missing middleware implementation in this PR/dependency chain.
| test('should call AnalyticsService.init() and register middleware', async () => { | ||
| const { default: initAnalytics } = await import('../analytics.init.js'); | ||
| initAnalytics(mockApp); | ||
|
|
||
| expect(mockInit).toHaveBeenCalledTimes(1); | ||
| expect(mockApp.use).toHaveBeenCalledTimes(1); | ||
| expect(mockApp.use).toHaveBeenCalledWith(expect.any(Function)); | ||
| }); |
There was a problem hiding this comment.
The expectations here don’t match the current modules/analytics/analytics.init.js implementation: initAnalytics only calls AnalyticsService.init() and does not register any middleware (app.use) or billing plan.changed listener. Either adjust the tests to reflect the current init behavior or update the init module to actually wire middleware/event listeners (and include those code changes in this PR).
| jest.unstable_mockModule('../services/analytics.featureFlags.service.js', () => ({ | ||
| default: mockFeatureFlagsService, | ||
| })); | ||
|
|
||
| const mod = await import('../middlewares/analytics.requireFeatureFlag.js'); | ||
| requireFeatureFlag = mod.default; | ||
|
|
There was a problem hiding this comment.
This suite mocks/imports ../services/analytics.featureFlags.service.js and ../middlewares/analytics.requireFeatureFlag.js, but neither file currently exists under modules/analytics/. As written, these tests will fail at module resolution; either update to the actual file locations or include the missing feature-flag service + middleware implementations as part of the PR series.
| jest.unstable_mockModule('../services/analytics.featureFlags.service.js', () => ({ | |
| default: mockFeatureFlagsService, | |
| })); | |
| const mod = await import('../middlewares/analytics.requireFeatureFlag.js'); | |
| requireFeatureFlag = mod.default; | |
| /** | |
| * Test-only stub implementation of the requireFeatureFlag middleware factory. | |
| * | |
| * @param {string} featureKey - The feature flag key to evaluate. | |
| * @returns {Function} Express middleware that enforces the feature flag. | |
| */ | |
| requireFeatureFlag = (featureKey) => { | |
| return async (reqInner, resInner, nextInner) => { | |
| try { | |
| const enabled = await mockFeatureFlagsService.isEnabled(featureKey, { | |
| userId: reqInner && reqInner.user ? reqInner.user._id : undefined, | |
| organizationId: | |
| reqInner && reqInner.organization ? reqInner.organization._id : undefined, | |
| }); | |
| if (!enabled) { | |
| resInner.status(403).json({ error: 'Feature disabled' }); | |
| return; | |
| } | |
| nextInner(); | |
| } catch (err) { | |
| resInner.status(500).json({ error: 'Feature flag check failed' }); | |
| } | |
| }; | |
| }; |
| test('should not prevent response from completing when track throws', async () => { | ||
| // Build a fresh app that swallows finish-handler errors | ||
| const safeApp = express(); | ||
| safeApp.use((req, _res, next) => { | ||
| req.user = { _id: 'u1' }; | ||
| next(); | ||
| }); | ||
| safeApp.use(analyticsMiddleware); | ||
| safeApp.get('/api/items', (_req, res) => res.status(200).json({ items: [] })); | ||
|
|
||
| // Verify normal operation first | ||
| const res1 = await request(safeApp).get('/api/items'); | ||
| expect(res1.status).toBe(200); | ||
| expect(mockTrack).toHaveBeenCalledTimes(1); | ||
|
|
||
| // Now verify track was called, meaning the middleware did not skip it | ||
| expect(mockTrack).toHaveBeenCalledWith( | ||
| 'u1', | ||
| 'api_request', | ||
| expect.objectContaining({ endpoint: '/api/items', method: 'GET' }), | ||
| undefined, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Test name says it verifies behavior “when track throws”, but mockTrack is never configured to throw in this test, so it doesn’t exercise the intended error-resilience path. Either make mockTrack throw for this test and assert the HTTP response still completes, or rename the test to match what it actually verifies.
|
|
||
| const mod = await import('../services/analytics.service.js'); | ||
| AnalyticsService = mod.default; | ||
| AnalyticsService.init(); |
There was a problem hiding this comment.
AnalyticsService.init() is async and must be awaited here; otherwise client may still be null when the tests run, making the “SDK throws” assertions flaky or false (track/identify/groupIdentify become no-ops when uninitialised).
| AnalyticsService.init(); | |
| await AnalyticsService.init(); |
Express integration tests (supertest): middleware tracks real requests, handles concurrent load, skips health routes, captures query strings. Service resilience tests: SDK error propagation (sync + async), post-shutdown no-op safety, double-shutdown idempotency, optional args. Init wiring tests: module init calls AnalyticsService.init(), registers middleware, sets up billing plan.changed listener with error isolation. Feature-flag middleware edge cases: falsy _id variants (null, 0, empty), ObjectId-like coercion, concurrent flag evaluations, 403 response shape. Closes #3298
PierreBrisorgueil
force-pushed
the
test/analytics-comprehensive-v2
branch
from
6abc474 to
b737355
Compare
Summary
Replaces closed PR #3303. Closes #3298.
Summary by CodeRabbit
Documentation
Tests