Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 71 additions & 2 deletions src/lib/__tests__/apiUtils.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { describe, it, expect, vi, beforeEach } from 'vitest';
import { handleErrorResponse, getAuthenticatedUser } from '../apiUtils';
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
import { handleErrorResponse, handleRateLimit, getAuthenticatedUser } from '../apiUtils';
import { RateLimitError } from '@/lib/types';
import { NextResponse } from 'next/server';
import { getServerSession } from "next-auth";

Expand Down Expand Up @@ -56,6 +57,74 @@ describe('apiUtils', () => {
});
});


describe('handleRateLimit', () => {
let originalDateNow: () => number;
const mockNow = 1700000000000; // 2023-11-14T22:13:20.000Z

beforeEach(() => {
originalDateNow = Date.now;
Date.now = vi.fn(() => mockNow);
});

afterEach(() => {
Date.now = originalDateNow;
});

it('should throw RateLimitError with X-RateLimit-Reset header timestamp', () => {
const resetTimestamp = 1700003600;
const res = new Response(null, {
headers: {
'X-RateLimit-Reset': resetTimestamp.toString(),
},
});

expect(() => handleRateLimit(res)).toThrow(RateLimitError);

try {
handleRateLimit(res);
} // eslint-disable-next-line @typescript-eslint/no-explicit-any
catch (error: any) {
expect(error).toBeInstanceOf(RateLimitError);
expect(error.resetAt.getTime()).toBe(resetTimestamp * 1000);
}
Comment on lines +82 to +90

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Calling handleRateLimit twice is redundant. Additionally, in TypeScript strict mode, the caught error is typed as unknown, so accessing error.resetAt directly will cause a compilation error. Capturing the thrown error and asserting on it is cleaner and type-safe.

      let thrownError: unknown;
      try {
        handleRateLimit(res);
      } catch (error) {
        thrownError = error;
      }

      expect(thrownError).toBeInstanceOf(RateLimitError);
      expect((thrownError as RateLimitError).resetAt.getTime()).toBe(resetTimestamp * 1000);

Comment on lines +84 to +90

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 TypeScript のストリクトモードでは catch ブロックの変数は unknown 型になります。error.resetAterror.resetAt.getTime() へのアクセスはコンパイルエラーとなります(TS18046: 'error' is of type 'unknown')。同パターンが3テスト全てに存在します。

Suggested change
try {
handleRateLimit(res);
} catch (error) {
expect(error).toBeInstanceOf(RateLimitError);
expect(error.resetAt.getTime()).toBe(resetTimestamp * 1000);
}
try {
handleRateLimit(res);
} catch (error) {
expect(error).toBeInstanceOf(RateLimitError);
expect((error as RateLimitError).resetAt.getTime()).toBe(resetTimestamp * 1000);
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/__tests__/apiUtils.test.ts
Line: 84-89

Comment:
TypeScript のストリクトモードでは `catch` ブロックの変数は `unknown` 型になります。`error.resetAt``error.resetAt.getTime()` へのアクセスはコンパイルエラーとなります(`TS18046: 'error' is of type 'unknown'`)。同パターンが3テスト全てに存在します。

```suggestion
      try {
        handleRateLimit(res);
      } catch (error) {
        expect(error).toBeInstanceOf(RateLimitError);
        expect((error as RateLimitError).resetAt.getTime()).toBe(resetTimestamp * 1000);
      }
```

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +82 to +90

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 catch ブロックのアサーションがサイレントスキップされる可能性

各テストで handleRateLimit(res) を2回呼び出しています(expect().toThrow() で1回、try/catch でもう1回)。try ブロックが何らかの理由でスローしない場合、catch 内の expect(error.resetAt.getTime())... は完全に実行されず、テストが誤ってグリーンになります。resetAt の値検証はこのパターンが唯一担っているため、expect.assertions(N) でアサーション実行数を保証することを検討してください。同パターンが3テスト全てに存在します。

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/__tests__/apiUtils.test.ts
Line: 82-89

Comment:
**catch ブロックのアサーションがサイレントスキップされる可能性**

各テストで `handleRateLimit(res)` を2回呼び出しています(`expect().toThrow()` で1回、`try/catch` でもう1回)。`try` ブロックが何らかの理由でスローしない場合、catch 内の `expect(error.resetAt.getTime())...` は完全に実行されず、テストが誤ってグリーンになります。`resetAt` の値検証はこのパターンが唯一担っているため、`expect.assertions(N)` でアサーション実行数を保証することを検討してください。同パターンが3テスト全てに存在します。

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!

});

it('should throw RateLimitError with default timestamp (1 hour from now) if header is missing', () => {
const res = new Response(null);

expect(() => handleRateLimit(res)).toThrow(RateLimitError);

try {
handleRateLimit(res);
} // eslint-disable-next-line @typescript-eslint/no-explicit-any
catch (error: any) {
expect(error).toBeInstanceOf(RateLimitError);
const expectedResetTimestamp = Math.floor(mockNow / 1000) + 3600;
expect(error.resetAt.getTime()).toBe(expectedResetTimestamp * 1000);
}
Comment on lines +96 to +105

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Calling handleRateLimit twice is redundant. Additionally, in TypeScript strict mode, the caught error is typed as unknown, so accessing error.resetAt directly will cause a compilation error. Capturing the thrown error and asserting on it is cleaner and type-safe.

      let thrownError: unknown;
      try {
        handleRateLimit(res);
      } catch (error) {
        thrownError = error;
      }

      expect(thrownError).toBeInstanceOf(RateLimitError);
      const expectedResetTimestamp = Math.floor(mockNow / 1000) + 3600;
      expect((thrownError as RateLimitError).resetAt.getTime()).toBe(expectedResetTimestamp * 1000);

});

it('should throw RateLimitError with default timestamp if header is invalid', () => {
const res = new Response(null, {
headers: {
'X-RateLimit-Reset': 'invalid-timestamp',
},
});

expect(() => handleRateLimit(res)).toThrow(RateLimitError);

try {
handleRateLimit(res);
} // eslint-disable-next-line @typescript-eslint/no-explicit-any
catch (error: any) {
expect(error).toBeInstanceOf(RateLimitError);
const expectedResetTimestamp = Math.floor(mockNow / 1000) + 3600;
expect(error.resetAt.getTime()).toBe(expectedResetTimestamp * 1000);
}
Comment on lines +115 to +124

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Calling handleRateLimit twice is redundant. Additionally, in TypeScript strict mode, the caught error is typed as unknown, so accessing error.resetAt directly will cause a compilation error. Capturing the thrown error and asserting on it is cleaner and type-safe.

      let thrownError: unknown;
      try {
        handleRateLimit(res);
      } catch (error) {
        thrownError = error;
      }

      expect(thrownError).toBeInstanceOf(RateLimitError);
      const expectedResetTimestamp = Math.floor(mockNow / 1000) + 3600;
      expect((thrownError as RateLimitError).resetAt.getTime()).toBe(expectedResetTimestamp * 1000);

});
});

describe('getAuthenticatedUser', () => {
it('should return user object if session is valid', async () => {
vi.mocked(getServerSession).mockResolvedValueOnce({
Expand Down
Loading