🧪 [testing improvement] Add tests for handleRateLimit in apiUtils#365
🧪 [testing improvement] Add tests for handleRateLimit in apiUtils#365is0692vs wants to merge 1 commit into
Conversation
Co-authored-by: is0692vs <135803462+is0692vs@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
More reviews will be available in 49 minutes and 9 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds unit tests for the handleRateLimit utility function, covering scenarios with valid, missing, and invalid X-RateLimit-Reset headers. The reviewer suggested improving the robustness of these tests by explicitly asserting the error class using toThrow(RateLimitError) and catching the error to verify its custom properties, rather than relying on expect.objectContaining. Additionally, the reviewer recommended adding explicit return types to the test functions to ensure type safety.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| it('should throw RateLimitError with timestamp from X-RateLimit-Reset header', () => { | ||
| const res = new Response(null, { | ||
| headers: { 'X-RateLimit-Reset': '2000' } | ||
| }); | ||
| expect(() => handleRateLimit(res)).toThrowError(expect.objectContaining({ | ||
| name: 'RateLimitError', | ||
| resetAt: new Date(2000000) | ||
| })); | ||
| }); |
There was a problem hiding this comment.
Using expect.objectContaining inside toThrowError only asserts that the thrown value is an object with matching properties (like name: 'RateLimitError'). It does not guarantee that the thrown error is actually an instance of the RateLimitError class. To ensure type safety and prevent false positives, it is more robust to assert the error class using toThrow(RateLimitError) and then catch the error to verify its custom properties. Additionally, ensure functions have explicit return types to maintain type safety.
it('should throw RateLimitError with timestamp from X-RateLimit-Reset header', (): void => {
const res = new Response(null, {
headers: { 'X-RateLimit-Reset': '2000' }
});
expect((): void => { handleRateLimit(res); }).toThrow(RateLimitError);
try {
handleRateLimit(res);
} catch (error) {
expect((error as RateLimitError).resetAt).toEqual(new Date(2000000));
}
});References
- Maintain explicit return types for functions in TypeScript to ensure type safety and API clarity.
| it('should throw RateLimitError with fallback timestamp if header is missing', () => { | ||
| const res = new Response(); | ||
| expect(() => handleRateLimit(res)).toThrowError(expect.objectContaining({ | ||
| name: 'RateLimitError', | ||
| resetAt: new Date(4600000) | ||
| })); | ||
| }); |
There was a problem hiding this comment.
Using expect.objectContaining inside toThrowError only asserts that the thrown value is an object with matching properties (like name: 'RateLimitError'). It does not guarantee that the thrown error is actually an instance of the RateLimitError class. To ensure type safety and prevent false positives, it is more robust to assert the error class using toThrow(RateLimitError) and then catch the error to verify its custom properties. Additionally, ensure functions have explicit return types to maintain type safety.
it('should throw RateLimitError with fallback timestamp if header is missing', (): void => {
const res = new Response();
expect((): void => { handleRateLimit(res); }).toThrow(RateLimitError);
try {
handleRateLimit(res);
} catch (error) {
expect((error as RateLimitError).resetAt).toEqual(new Date(4600000));
}
});References
- Maintain explicit return types for functions in TypeScript to ensure type safety and API clarity.
| it('should throw RateLimitError with fallback timestamp if header is invalid', () => { | ||
| const res = new Response(null, { | ||
| headers: { 'X-RateLimit-Reset': 'invalid' } | ||
| }); | ||
| expect(() => handleRateLimit(res)).toThrowError(expect.objectContaining({ | ||
| name: 'RateLimitError', | ||
| resetAt: new Date(4600000) | ||
| })); | ||
| }); |
There was a problem hiding this comment.
Using expect.objectContaining inside toThrowError only asserts that the thrown value is an object with matching properties (like name: 'RateLimitError'). It does not guarantee that the thrown error is actually an instance of the RateLimitError class. To ensure type safety and prevent false positives, it is more robust to assert the error class using toThrow(RateLimitError) and then catch the error to verify its custom properties. Additionally, ensure functions have explicit return types to maintain type safety.
it('should throw RateLimitError with fallback timestamp if header is invalid', (): void => {
const res = new Response(null, {
headers: { 'X-RateLimit-Reset': 'invalid' }
});
expect((): void => { handleRateLimit(res); }).toThrow(RateLimitError);
try {
handleRateLimit(res);
} catch (error) {
expect((error as RateLimitError).resetAt).toEqual(new Date(4600000));
}
});References
- Maintain explicit return types for functions in TypeScript to ensure type safety and API clarity.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| import { handleErrorResponse, getAuthenticatedUser } from '../apiUtils'; | ||
| import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; | ||
| import { handleErrorResponse, getAuthenticatedUser, handleRateLimit } from '../apiUtils'; | ||
| import { RateLimitError } from '../types'; |
There was a problem hiding this comment.
RateLimitError
RateLimitError クラスがインポートされていますが、テスト内のアサーションでは文字列リテラル 'RateLimitError' のみを使用しており、クラス自体は一切参照されていません。このインポートは削除するか、expect.any(RateLimitError) や .toThrow(RateLimitError) による instanceof チェックとして活用することを推奨します。文字列比較だと name プロパティを手動で設定したあらゆるエラーが誤ってパスしてしまうため、型安全性が低くなります。
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/__tests__/apiUtils.test.ts
Line: 3
Comment:
**未使用インポート: `RateLimitError`**
`RateLimitError` クラスがインポートされていますが、テスト内のアサーションでは文字列リテラル `'RateLimitError'` のみを使用しており、クラス自体は一切参照されていません。このインポートは削除するか、`expect.any(RateLimitError)` や `.toThrow(RateLimitError)` による `instanceof` チェックとして活用することを推奨します。文字列比較だと `name` プロパティを手動で設定したあらゆるエラーが誤ってパスしてしまうため、型安全性が低くなります。
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| expect(() => handleRateLimit(res)).toThrowError(expect.objectContaining({ | ||
| name: 'RateLimitError', | ||
| resetAt: new Date(2000000) | ||
| })); |
There was a problem hiding this comment.
RateLimitError インポートを活用した instanceof チェックへの改善
現在は name: 'RateLimitError' という文字列で確認しているため、同じ name プロパティを持つ別クラスでもテストがパスしてしまいます。インポート済みの RateLimitError を使い .toThrow(RateLimitError) で型を明示的に検証することで、より堅牢なアサーションになります。
| expect(() => handleRateLimit(res)).toThrowError(expect.objectContaining({ | |
| name: 'RateLimitError', | |
| resetAt: new Date(2000000) | |
| })); | |
| expect(() => handleRateLimit(res)).toThrow(RateLimitError); | |
| expect(() => handleRateLimit(res)).toThrowError(expect.objectContaining({ | |
| resetAt: new Date(2000000) | |
| })); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/__tests__/apiUtils.test.ts
Line: 74-77
Comment:
**`RateLimitError` インポートを活用した `instanceof` チェックへの改善**
現在は `name: 'RateLimitError'` という文字列で確認しているため、同じ `name` プロパティを持つ別クラスでもテストがパスしてしまいます。インポート済みの `RateLimitError` を使い `.toThrow(RateLimitError)` で型を明示的に検証することで、より堅牢なアサーションになります。
```suggestion
expect(() => handleRateLimit(res)).toThrow(RateLimitError);
expect(() => handleRateLimit(res)).toThrowError(expect.objectContaining({
resetAt: new Date(2000000)
}));
```
How can I resolve this? If you propose a fix, please make it concise.1 participant
🎯 What: This PR addresses a testing gap by adding unit tests for the
handleRateLimitfunction insrc/lib/apiUtils.ts.📊 Coverage: The tests cover the happy path of parsing a valid
X-RateLimit-Resetheader and returning the associated timestamp, as well as the fallback logic when the header is missing or invalid.✨ Result: Test coverage for
src/lib/apiUtils.tshas been improved, and the logic for parsing and throwingRateLimitErroris now safeguarded against regressions.PR created automatically by Jules for task 15266095550055349427 started by @is0692vs
Greptile Summary
このPRは
src/lib/apiUtils.tsのhandleRateLimit関数に対するユニットテストを追加します。正常系（有効なX-RateLimit-Resetヘッダー）・ヘッダー欠損時のフォールバック・不正値（NaN）時のフォールバックの3ケースがカバーされています。vi.useFakeTimers()+vi.setSystemTime(new Date(1000000))で時刻を固定し、フォールバック計算（Math.floor(Date.now()/1000) + 3600 = 4600→new Date(4600000)）を決定論的にテストしている。RateLimitErrorクラスがインポートされているものの、アサーションで使われておらず未使用となっている点が唯一の改善余地。Confidence Score: 4/5
テスト専用ファイルの追加のみで、プロダクションコードへの変更はなく、安全にマージ可能。
追加されたテストはロジック的に正しく、タイムスタンプ計算も正確に検証されている。
RateLimitErrorの未使用インポートとアサーションの型チェックの弱さは軽微な改善余地にとどまる。特に注意が必要なファイルはなし。
src/lib/__tests__/apiUtils.test.tsの未使用インポートを整理するとよい。Important Files Changed
handleRateLimitの3ケース（正常系・ヘッダーなし・不正値）をカバーするテストを追加。未使用のRateLimitErrorインポートが残っている。Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["handleRateLimit(res) 呼び出し"] --> B{"X-RateLimit-Reset\nヘッダーが存在するか？"} B -- "あり" --> C["parseInt(header, 10)"] B -- "なし" --> E["fallback:\nMath.floor(Date.now()/1000)+3600"] C --> D{"Number.isFinite\n(resetTimestamp)?"} D -- "true (例: 2000)" --> F["throw new RateLimitError(2000)\nresetAt = new Date(2000000)"] D -- "false (例: NaN from 'invalid')" --> E E --> G["throw new RateLimitError(4600)\nresetAt = new Date(4600000)"] style F fill:#d4edda,stroke:#28a745 style G fill:#fff3cd,stroke:#ffc107Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "test: add unit tests for handleRateLimit..." | Re-trigger Greptile