test(execution): 32 testes para RateLimitManager (#52)#609
Open
nikolasdehor wants to merge 1 commit intoSynkraAI:mainfrom
Open
test(execution): 32 testes para RateLimitManager (#52)#609nikolasdehor wants to merge 1 commit intoSynkraAI:mainfrom
nikolasdehor wants to merge 1 commit intoSynkraAI:mainfrom
Conversation
Cobertura completa: constructor, isRateLimitError (10 cenários), calculateDelay (backoff exponencial + retryAfter + maxDelay), executeWithRetry (sucesso, retry, maxRetries, erros não-rate-limit), metrics, events, resetMetrics, formatStatus, withRateLimit wrapper e getGlobalManager singleton.
nikolasdehor
requested review from
Pedrovaleriolopez and
oalanicolas
as code owners
github-actions
bot
added
area: agents
|
@nikolasdehor is attempting to deploy a commit to the Pedro Valério Lopez's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughA comprehensive test suite for RateLimitManager is introduced, validating constructor behavior, rate-limit error detection, delay calculation with exponential backoff, retry execution with recovery, metrics tracking, event emission, the withRateLimit wrapper function, and the global singleton pattern across 319 test lines. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can get early access to new features in CodeRabbit.Enable the |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![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
🧹 Nitpick comments (1)
tests/core/execution/rate-limit-manager.test.js (1)
13-14: Use the repository’s absolute import form here.The
../../../hop chain makes this test brittle to moves and breaks the JS import convention defined for this codebase. As per coding guidelines, "Use absolute imports instead of relative imports in all code."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/execution/rate-limit-manager.test.js` around lines 13 - 14, Replace the brittle relative import of RateLimitManager with the repository's absolute import path: locate the lines importing RateLimitManager and destructuring withRateLimit and getGlobalManager, and update the require(...) to use the project's absolute module path (the canonical package/module name used across the repo) so the test imports RateLimitManager, withRateLimit, and getGlobalManager via the absolute import form rather than '../../../...'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/core/execution/rate-limit-manager.test.js`:
- Around line 295-306: The test currently never exercises retry behavior because
original resolves immediately; update the spec for withRateLimit to force a
retry by making original reject once with the rate-limit error (e.g.,
mockRejectedValueOnce(new RateLimitError(...) or a sentinel) and then resolve on
the next call, then call the wrapped function and assert both the final result
and that the manager was used (e.g., expect(mgr.sleep).toHaveBeenCalled() or
expect(mgr.metrics.totalRetries).toBeGreaterThan(0)); reference the
withRateLimit wrapper and the RateLimitManager instance (mgr) and use the
original mock's rejected-then-resolved sequence to prove retry logic runs.
- Around line 4-5: The test header currently claims it covers preemptiveThrottle
but the file never exercises that function; either remove "preemptiveThrottle"
from the opening comment so it accurately lists only executeWithRetry,
calculateDelay, isRateLimitError, withRateLimit, metrics and events, or add a
missing spec that imports and invokes preemptiveThrottle (e.g., a new
describe/it block that calls preemptiveThrottle with appropriate stubbed inputs
and asserts expected behavior). Ensure the unique symbol preemptiveThrottle is
referenced in the new spec if you choose to add it and keep the header and
actual tests consistent.
- Around line 152-156: The test currently checks that mgr.calculateDelay(10,
err) is <= 30000 which is too loose; change the assertion to require the exact
capped value by asserting expect(mgr.calculateDelay(10, err)).toBe(30000). This
targets the calculateDelay behavior (with baseDelay: 1000 and attempt 10) to
ensure the cap logic in the RateLimitManager is enforced precisely.
---
Nitpick comments:
In `@tests/core/execution/rate-limit-manager.test.js`:
- Around line 13-14: Replace the brittle relative import of RateLimitManager
with the repository's absolute import path: locate the lines importing
RateLimitManager and destructuring withRateLimit and getGlobalManager, and
update the require(...) to use the project's absolute module path (the canonical
package/module name used across the repo) so the test imports RateLimitManager,
withRateLimit, and getGlobalManager via the absolute import form rather than
'../../../...'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0ed8cda5-3680-4b63-85ed-4793b2ac6631
📒 Files selected for processing (1)
tests/core/execution/rate-limit-manager.test.js
Comment on lines
+4
to
+5
| * Cobre executeWithRetry, calculateDelay, isRateLimitError, | ||
| * preemptiveThrottle, metrics, events e withRateLimit wrapper. |
There was a problem hiding this comment.
Remove preemptiveThrottle from the header or add the missing spec.
This file never exercises preemptiveThrottle, so the opening comment currently overstates the covered surface.
📝 Minimal doc fix
- * Cobre executeWithRetry, calculateDelay, isRateLimitError,
- * preemptiveThrottle, metrics, events e withRateLimit wrapper.
+ * Cobre executeWithRetry, calculateDelay, isRateLimitError,
+ * metrics, events e withRateLimit wrapper.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| * Cobre executeWithRetry, calculateDelay, isRateLimitError, | |
| * preemptiveThrottle, metrics, events e withRateLimit wrapper. | |
| * Cobre executeWithRetry, calculateDelay, isRateLimitError, | |
| * metrics, events e withRateLimit wrapper. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/execution/rate-limit-manager.test.js` around lines 4 - 5, The test
header currently claims it covers preemptiveThrottle but the file never
exercises that function; either remove "preemptiveThrottle" from the opening
comment so it accurately lists only executeWithRetry, calculateDelay,
isRateLimitError, withRateLimit, metrics and events, or add a missing spec that
imports and invokes preemptiveThrottle (e.g., a new describe/it block that calls
preemptiveThrottle with appropriate stubbed inputs and asserts expected
behavior). Ensure the unique symbol preemptiveThrottle is referenced in the new
spec if you choose to add it and keep the header and actual tests consistent.
Comment on lines
+152
to
+156
| it('deve limitar ao maxDelay', () => { | ||
| const err = new Error('rate limited'); | ||
| // attempt 10: 1000 * 2^9 = 512000 → capped at 30000 | ||
| const delay = mgr.calculateDelay(10, err); | ||
| expect(delay).toBeLessThanOrEqual(30000); |
There was a problem hiding this comment.
Assert the capped value, not just the upper bound.
With baseDelay: 1000 and attempt 10, this path should always return exactly 30000. The current matcher would still pass if the cap logic regressed and returned a smaller value.
✅ Tighten the cap assertion
it('deve limitar ao maxDelay', () => {
const err = new Error('rate limited');
// attempt 10: 1000 * 2^9 = 512000 → capped at 30000
const delay = mgr.calculateDelay(10, err);
- expect(delay).toBeLessThanOrEqual(30000);
+ expect(delay).toBe(30000);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/execution/rate-limit-manager.test.js` around lines 152 - 156, The
test currently checks that mgr.calculateDelay(10, err) is <= 30000 which is too
loose; change the assertion to require the exact capped value by asserting
expect(mgr.calculateDelay(10, err)).toBe(30000). This targets the calculateDelay
behavior (with baseDelay: 1000 and attempt 10) to ensure the cap logic in the
RateLimitManager is enforced precisely.
Comment on lines
+295
to
+306
| describe('withRateLimit', () => { | ||
| it('deve wrappear função com retry automático', async () => { | ||
| const mgr = new RateLimitManager({ maxRetries: 2, baseDelay: 1 }); | ||
| mgr.sleep = jest.fn().mockResolvedValue(undefined); | ||
|
|
||
| const original = jest.fn().mockResolvedValue('result'); | ||
| const wrapped = withRateLimit(original, mgr, { label: 'test' }); | ||
|
|
||
| const result = await wrapped('arg1', 'arg2'); | ||
| expect(result).toBe('result'); | ||
| expect(original).toHaveBeenCalledWith('arg1', 'arg2'); | ||
| }); |
There was a problem hiding this comment.
This wrapper spec does not prove the manager is actually involved.
Because original succeeds on the first call, the test would still pass if withRateLimit() simply returned original and ignored mgr entirely. Force one rate-limit failure and assert a retry side effect such as mgr.metrics.totalRetries or a sleep() call.
🔍 Stronger wrapper coverage
- const original = jest.fn().mockResolvedValue('result');
+ const original = jest.fn()
+ .mockRejectedValueOnce(new Error('rate limit'))
+ .mockResolvedValueOnce('result');
const wrapped = withRateLimit(original, mgr, { label: 'test' });
const result = await wrapped('arg1', 'arg2');
expect(result).toBe('result');
- expect(original).toHaveBeenCalledWith('arg1', 'arg2');
+ expect(original).toHaveBeenNthCalledWith(1, 'arg1', 'arg2');
+ expect(original).toHaveBeenNthCalledWith(2, 'arg1', 'arg2');
+ expect(mgr.sleep).toHaveBeenCalledTimes(1);
+ expect(mgr.metrics.totalRetries).toBe(1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/execution/rate-limit-manager.test.js` around lines 295 - 306, The
test currently never exercises retry behavior because original resolves
immediately; update the spec for withRateLimit to force a retry by making
original reject once with the rate-limit error (e.g., mockRejectedValueOnce(new
RateLimitError(...) or a sentinel) and then resolve on the next call, then call
the wrapped function and assert both the final result and that the manager was
used (e.g., expect(mgr.sleep).toHaveBeenCalled() or
expect(mgr.metrics.totalRetries).toBeGreaterThan(0)); reference the
withRateLimit wrapper and the RateLimitManager instance (mgr) and use the
original mock's rejected-then-resolved sequence to prove retry logic runs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resumo
Adiciona 32 testes unitarios para o modulo
RateLimitManagerque nao tinha cobertura.Cobertura
Closes #52 (parcial)
Plano de teste
npx jest tests/core/execution/rate-limit-manager.test.js-- 32/32 passandosleep()para evitar delays reaisSummary by CodeRabbit