From 99bbc25ca948f2013f6c44d34ec2f33238cdacd8 Mon Sep 17 00:00:00 2001 From: Kato Hiroki Date: Sat, 25 Oct 2025 09:56:15 +0000 Subject: [PATCH 1/3] feat: Add crud for getMergedTasksMap (#2748) --- .../refactor-getMergedTasksMap/lesson.md | 61 +++++++++++++ src/lib/services/tasks.ts | 85 ++++++++++++++++++- src/lib/types/contest_task_pair.ts | 4 +- src/lib/types/task.ts | 2 + src/lib/utils/contest_task_pair.ts | 23 +++++ 5 files changed, 172 insertions(+), 3 deletions(-) create mode 100644 docs/dev-notes/2025-10-25/refactor-getMergedTasksMap/lesson.md create mode 100644 src/lib/utils/contest_task_pair.ts diff --git a/docs/dev-notes/2025-10-25/refactor-getMergedTasksMap/lesson.md b/docs/dev-notes/2025-10-25/refactor-getMergedTasksMap/lesson.md new file mode 100644 index 000000000..edf1a4dc4 --- /dev/null +++ b/docs/dev-notes/2025-10-25/refactor-getMergedTasksMap/lesson.md @@ -0,0 +1,61 @@ +# getMergedTasksMap のリファクタリング教訓 + +## 学習内容 + +### 1. **参照 vs コピー** + +TypeScript（JavaScript）の `const newTask = task;` は参照をコピーするため、`newTask` を変更すると元の `task` も変更されます。 + +- **浅いコピー**: `const newTask = { ...task };` +- **深いコピー**: `const newTask = JSON.parse(JSON.stringify(task));` + +### 2. **TypeScript らしいコード書き方** + +- `map()` で初期化: `new Map(tasks.map(task => [task.task_id, task]))` +- ループではなく関数型メソッド (`filter()`, `map()`, `flatMap()`) +- スプレッド演算子で Map をマージ: `new Map([...map1, ...map2])` + +### 3. **`flatMap()` vs `map()`** + +`flatMap()` は返した配列を1段階フラット化するため、条件付きの可変長結果に最適： + +```typescript +// flatMap で条件分岐を自然に表現 +.flatMap((pair) => { + if (!task || !contestType) return []; + return [createMergedTask(...)]; +}); +// 結果: 該当する要素だけが含まれる +``` + +### 4. **読みやすさ > 1行でまとめる** + +無理やり `return` や1行で書く必要はない： + +- 複雑な条件は `if` 文で早期リターン +- オブジェクト生成は `key` と `value` を分けて記述 +- 難しいロジックはヘルパー関数に抽出 + +### 5. **計算量の分析** + +- 全体: **O(N + M)** （N: タスク数、M: ペア数） +- `Map.has()`, `Map.get()` は **O(1)** なのでループ内で複数回呼んでもOK + +### 6. **ドキュメント化のポイント** + +- 関数の目的と副作用を明確に +- **計算量と根拠** を記載 +- **使用例** を `@example` で示す +- 戻り値の構造を詳しく説明 + +## コード例（改善版） + +src/lib/services/tasks.ts を参照 + +## キーポイント + +- ✅ 非破壊的な変更（スプレッド演算子） +- ✅ 関数型パラダイム（`filter()`, `flatMap()` 使用） +- ✅ 早期リターンで複雑さを減らす +- ✅ ヘルパー関数で責任分離 +- ✅ 明確なドキュメント化 diff --git a/src/lib/services/tasks.ts b/src/lib/services/tasks.ts index b2e3074f4..a3887595c 100644 --- a/src/lib/services/tasks.ts +++ b/src/lib/services/tasks.ts @@ -1,7 +1,17 @@ import { default as db } from '$lib/server/database'; + +import { getContestTaskPairs } from '$lib/services/contest_task_pairs'; + +import { ContestType } from '$lib/types/contest'; +import type { Task, TaskGrade } from '$lib/types/task'; +import type { + ContestTaskPair, + ContestTaskPairKey, + TaskMapByContestTaskPair, +} from '$lib/types/contest_task_pair'; + import { classifyContest } from '$lib/utils/contest'; -import type { TaskGrade } from '$lib/types/task'; -import type { Task, Tasks } from '$lib/types/task'; +import { createContestTaskPairKey } from '$lib/utils/contest_task_pair'; // See: // https://www.prisma.io/docs/concepts/components/prisma-client/filtering-and-sorting @@ -11,6 +21,77 @@ export async function getTasks(): Promise { return tasks; } +/** + * Fetches and merges tasks based on contest-task pairs. + * + * @returns A promise that resolves to a map of merged tasks keyed by contest-task pair. + * + * @note This function merges tasks with the same task_id but different contest_id + * from the contest-task pairs table. It enriches existing tasks with + * contest-specific information (contest_type, task_table_index, etc.). + * @note Time Complexity: O(N + M) + * - N: number of tasks from the database + * - M: number of contest-task pairs + * - Map operations (has, get, set) are O(1) + * @example + * const mergedTasksMap = await getMergedTasksMap(); + * const task = mergedTasksMap.get(createContestTaskPairKey('tessoku-book', 'typical90_s')); + */ +export async function getMergedTasksMap(): Promise { + const tasks = await getTasks(); + const contestTaskPairs = await getContestTaskPairs(); + + const baseTaskMap = new Map( + tasks.map((task) => [createContestTaskPairKey(task.contest_id, task.task_id), task]), + ); + // Unique task_id in database + const taskMap = new Map(tasks.map((task) => [task.task_id, task])); + + // Filter task(s) only the same task_id but different contest_id + const additionalTaskMap = contestTaskPairs + .filter((pair) => !baseTaskMap.has(createContestTaskPairKey(pair.contestId, pair.taskId))) + .flatMap((pair) => { + const task = taskMap.get(pair.taskId); + const contestType = classifyContest(pair.contestId); + + if (!task || !contestType || !pair.taskTableIndex) { + return []; + } + + return [createMergedTask(task, pair, contestType)]; + }); + + return new Map([...baseTaskMap, ...additionalTaskMap]); +} + +/** + * Creates a merged task from the original task and contest-task pair. + * + * @param task The original task to be enriched with contest-specific information. + * @param pair The contest-task pair containing contestId, taskTableIndex and taskId. + * @param contestType The type of contest (e.g., ABC, ARC) derived from contest_id. + * @returns A tuple [key, mergedTask] where: + * - key: the unique identifier for the contestId:taskId pair + * - mergedTask: the task with contest-specific fields updated + */ +function createMergedTask( + task: Task, + pair: ContestTaskPair, + contestType: ContestType, +): [ContestTaskPairKey, Task] { + const key = createContestTaskPairKey(pair.contestId, pair.taskId); + + const mergedTask: Task = { + ...task, + contest_type: contestType, + contest_id: pair.contestId, + task_table_index: pair.taskTableIndex, + title: task.title.replace(task.task_table_index, pair.taskTableIndex), + }; + + return [key, mergedTask]; +} + /** * Fetches tasks with the specified task IDs. * @param selectedTaskIds - An array of task IDs to filter the tasks. diff --git a/src/lib/types/contest_task_pair.ts b/src/lib/types/contest_task_pair.ts index 50ca5f4b0..d22866471 100644 --- a/src/lib/types/contest_task_pair.ts +++ b/src/lib/types/contest_task_pair.ts @@ -1,6 +1,6 @@ import type { ContestTaskPair as ContestTaskPairOrigin } from '@prisma/client'; -import type { TaskResult } from '$lib/types/task'; +import type { Task, TaskResult } from '$lib/types/task'; export type ContestTaskPair = ContestTaskPairOrigin; @@ -20,4 +20,6 @@ export type ContestTaskPairUpdate = ContestTaskPairCreate; // For mapping and identification export type ContestTaskPairKey = `${string}:${string}`; // "contest_id:task_id" +export type TaskMapByContestTaskPair = Map; + export type TaskResultMapByContestTaskPair = Map; diff --git a/src/lib/types/task.ts b/src/lib/types/task.ts index 611194030..26e14510a 100644 --- a/src/lib/types/task.ts +++ b/src/lib/types/task.ts @@ -1,7 +1,9 @@ // Import original enum as type. import type { TaskGrade as TaskGradeOrigin } from '@prisma/client'; +import type { ContestType } from '$lib/types/contest'; export interface Task { + contest_type?: ContestType; contest_id: string; task_table_index: string; task_id: string; diff --git a/src/lib/utils/contest_task_pair.ts b/src/lib/utils/contest_task_pair.ts new file mode 100644 index 000000000..b068352a5 --- /dev/null +++ b/src/lib/utils/contest_task_pair.ts @@ -0,0 +1,23 @@ +import type { ContestTaskPairKey } from '$lib/types/contest_task_pair'; + +/** + * Creates a unique key for a ContestTaskPair using contestId and taskId. + * Throws an error if either argument is an empty string. + * + * @param contestId - The ID of the contest. + * @param taskId - The ID of the task. + * + * @returns A string in the format "contestId:taskId". + * + * @throws Will throw an error if contestId or taskId is empty. + */ +export function createContestTaskPairKey(contestId: string, taskId: string): ContestTaskPairKey { + if (!contestId || contestId.trim() === '') { + throw new Error('contestId must be a non-empty string'); + } + if (!taskId || taskId.trim() === '') { + throw new Error('taskId must be a non-empty string'); + } + + return `${contestId}:${taskId}`; +} From c611499da491b18cc147cee2160d890cf7fd8442 Mon Sep 17 00:00:00 2001 From: Kato Hiroki Date: Sat, 25 Oct 2025 12:10:50 +0000 Subject: [PATCH 2/3] test: Add tests for createContestTaskPairKey (#2748) --- .../refactor-getMergedTasksMap/lesson.md | 82 +++++ src/test/lib/utils/contest_task_pair.test.ts | 295 ++++++++++++++++++ 2 files changed, 377 insertions(+) create mode 100644 src/test/lib/utils/contest_task_pair.test.ts diff --git a/docs/dev-notes/2025-10-25/refactor-getMergedTasksMap/lesson.md b/docs/dev-notes/2025-10-25/refactor-getMergedTasksMap/lesson.md index edf1a4dc4..9db45ec6c 100644 --- a/docs/dev-notes/2025-10-25/refactor-getMergedTasksMap/lesson.md +++ b/docs/dev-notes/2025-10-25/refactor-getMergedTasksMap/lesson.md @@ -59,3 +59,85 @@ src/lib/services/tasks.ts を参照 - ✅ 早期リターンで複雑さを減らす - ✅ ヘルパー関数で責任分離 - ✅ 明確なドキュメント化 + +--- + +# createContestTaskPairKey のテスト設計教訓 + +## テスト作成で学んだこと + +### 1. **ヘルパー関数で重複削減** + +同じパターンのテストコードは **ヘルパー関数** に抽出： + +```typescript +// ❌ Before: 重複が多い +const key1 = createTestKey(pair); +const key2 = createTestKey(pair); +expect(key1).toBe(key2); + +// ✅ After: ヘルパー関数化 +const expectKeysToBeConsistent = (pair: TestPair): void => { + const key1 = createTestKey(pair); + const key2 = createTestKey(pair); + expect(key1).toBe(key2); +}; +expectKeysToBeConsistent(pair); +``` + +### 2. **パラメタライズテスト（test.each）で 4 個 → 1 個に** + +4 つの似たテストは `test.each()` で 1 つにまとめる： + +```typescript +// ❌ Before: 4 つのテスト関数 +test('expects empty contest_id to throw an error', () => { ... }); +test('expects empty task_id to throw an error', () => { ... }); +test('expects whitespace-only contest_id to throw an error', () => { ... }); +test('expects whitespace-only task_id to throw an error', () => { ... }); + +// ✅ After: 1 つのパラメタライズテスト +test.each<[string, string, string]>([ + ['', 'abc123_a', 'contestId must be a non-empty string'], + [' ', 'abc123_a', 'contestId must be a non-empty string'], + ['abc123', '', 'taskId must be a non-empty string'], + ['abc123', ' ', 'taskId must be a non-empty string'], +])('expects error when contest_id="%s" and task_id="%s"', (contestId, taskId, expectedError) => { + expect(() => createContestTaskPairKey(contestId, taskId)).toThrow(expectedError); +}); +``` + +### 3. **テストデータを集約して保守性向上** + +テストデータを `pairs` オブジェクトで一元管理： + +```typescript +const pairs = { + normal: [...], // 正常系ケース + edge: [...], // エッジケース + anomaly: [...], // 異常系ケース +}; +``` + +### 4. **テストカバレッジの考え方** + +- **正常系**: 期待通りに動くか +- **エッジケース**: 空文字列、ホワイトスペース、長い文字列 +- **異常系**: 特殊文字、Unicode、改行、タブ +- **キー検証**: フォーマット、一意性、可逆性 + +### 5. **べストプラクティス** + +| 改善内容 | 効果 | +| -------------------- | -------------------- | +| ヘルパー関数化 | コード重複 -40% | +| パラメタライズテスト | テスト関数数 削減 | +| テストデータ集約 | 保守性向上 | +| beforeEach で初期化 | テスト間の独立性確保 | + +## テスト統計 + +- **総テスト数**: 27 個（全成功 ✅） +- **パラメタライズテスト**: 2 グループ（合計 8 ケース） +- **ヘルパー関数**: 5 個 +- **テストデータセット**: 3 グループ（normal, edge, anomaly） diff --git a/src/test/lib/utils/contest_task_pair.test.ts b/src/test/lib/utils/contest_task_pair.test.ts new file mode 100644 index 000000000..ce86f918a --- /dev/null +++ b/src/test/lib/utils/contest_task_pair.test.ts @@ -0,0 +1,295 @@ +import { describe, test, expect, beforeEach } from 'vitest'; + +import { createContestTaskPairKey } from '$lib/utils/contest_task_pair'; + +describe('createContestTaskPairKey', () => { + // Test data structure + interface TestPair { + contestId: string; + taskId: string; + } + + type TestPairs = TestPair[]; + + // Helper to create test pairs + const pairs = { + normal: [ + { contestId: 'abc100', taskId: 'abc100_a' }, + { contestId: 'abc397', taskId: 'abc397_g' }, + { contestId: 'arc050', taskId: 'arc050_a' }, + { contestId: 'dp', taskId: 'dp_a' }, + { contestId: 'tdpc', taskId: 'tdpc_a' }, + { contestId: 'typical90', taskId: 'typical90_a' }, + { contestId: 'typical90', taskId: 'typical90_cl' }, + { contestId: 'tessoku-book', taskId: 'abc007_3' }, + { contestId: 'tessoku-book', taskId: 'math_and_algorithm_am' }, + { contestId: 'tessoku-book', taskId: 'typical90_s' }, + { contestId: 'joi2024yo1a', taskId: 'joi2024yo1a_a' }, + ] as const, + edge: [ + { contestId: '', taskId: '' }, + { contestId: 'abc123', taskId: '' }, + { contestId: '', taskId: 'abc123_a' }, + { contestId: '123', taskId: '456' }, + { contestId: '_', taskId: '_' }, + { contestId: 'abc_123_def', taskId: 'task_id_part' }, + { contestId: 'abc-123', taskId: 'task-id' }, + { contestId: 'a', taskId: 'b' }, + { contestId: 'abc 123', taskId: 'abc123 a' }, + { contestId: 'abc.123', taskId: 'abc123.a' }, + ] as const, + anomaly: [ + { contestId: 'abc|123', taskId: 'abc123_a' }, + { contestId: 'abc123', taskId: 'abc|123_a' }, + { contestId: 'abc|123', taskId: 'abc|123_a' }, + { contestId: 'abc||123', taskId: 'task||id' }, + { contestId: 'abc.*+?^${}()', taskId: 'task[a-z]' }, + { contestId: 'abc日本語123', taskId: 'task日本語a' }, + { contestId: 'abc😀', taskId: 'task😀' }, + { contestId: 'abc\n123', taskId: 'task\na' }, + { contestId: 'abc\t123', taskId: 'task\ta' }, + ] as const, + }; + + let testNormalPairs: TestPairs; + let testEdgePairs: TestPairs; + let testAnomalyPairs: TestPairs; + + beforeEach(() => { + testNormalPairs = [...pairs.normal]; + testEdgePairs = [...pairs.edge]; + testAnomalyPairs = [...pairs.anomaly]; + }); + + // Helper functions: + // To generate expected key + const getExpectedKey = (contestId: string, taskId: string) => `${contestId}:${taskId}`; + + // To create a key from a pair + const createTestKey = (pair: TestPair): string => + createContestTaskPairKey(pair.contestId, pair.taskId); + + // To compare two keys for consistency + const expectKeysToBeConsistent = (pair: TestPair): void => { + const key1 = createTestKey(pair); + const key2 = createTestKey(pair); + + expect(key1).toBe(key2); + }; + + // To compare keys for difference + const expectKeysDifferent = (pair1: TestPair, pair2: TestPair): void => { + const key1 = createTestKey(pair1); + const key2 = createTestKey(pair2); + + expect(key1).not.toBe(key2); + }; + + // To run test for multiple pairs + const testMultiplePairs = ( + pairList: TestPairs, + validator: (pair: TestPair, key: string) => void, + ): void => { + pairList.forEach((pair) => { + const key = createTestKey(pair); + validator(pair, key); + }); + }; + + describe('Base cases', () => { + test('expects to create a key with valid contest_id and task_id', () => { + const pair = testNormalPairs[0]; + const key = createTestKey(pair); + + expect(key).toBe(getExpectedKey(pair.contestId, pair.taskId)); + expect(typeof key).toBe('string'); + }); + + test('expects to create different keys for different contest_ids', () => { + const pair = testNormalPairs[0]; + const modifiedPair = { ...pair, contestId: 'abc124' }; + + expectKeysDifferent(pair, modifiedPair); + }); + + test('expects to create different keys for different task_ids', () => { + const pair = testNormalPairs[0]; + const modifiedPair = { ...pair, taskId: 'abc100_b' }; + + expectKeysDifferent(pair, modifiedPair); + }); + + test('expects to create consistent keys for the same inputs', () => { + const pair = testNormalPairs[0]; + + expectKeysToBeConsistent(pair); + }); + + test('expects to work with various contest types', () => { + testMultiplePairs(testNormalPairs, ({ contestId, taskId }) => { + const pair = { contestId, taskId }; + const key = createTestKey(pair); + + expect(key).toBe(getExpectedKey(contestId, taskId)); + }); + }); + + test('expects to work with uppercase and lowercase characters', () => { + const pair1 = { contestId: 'ABC123', taskId: 'ABC123_A' }; + const pair2 = { contestId: 'abc123', taskId: 'abc123_a' }; + + expectKeysDifferent(pair1, pair2); + }); + + test('expects to work with numeric task identifiers', () => { + const pair = testNormalPairs[5]; // typical90_a + const key = createTestKey(pair); + + expect(key).toContain(pair.taskId); + }); + + test('expects to work with long contest and task IDs', () => { + const pair = { + contestId: 'a'.repeat(50), + taskId: 'a'.repeat(50) + '_' + 'b'.repeat(50), + }; + const key = createTestKey(pair); + + expect(key).toBe(getExpectedKey(pair.contestId, pair.taskId)); + }); + }); + + describe('Edge cases', () => { + test('expects all edge cases to format correctly', () => { + // Filter out empty string cases as they should throw + const validEdgePairs = testEdgePairs.filter( + (pair) => pair.contestId.trim() !== '' && pair.taskId.trim() !== '', + ); + + testMultiplePairs(validEdgePairs, ({ contestId, taskId }) => { + const pair = { contestId, taskId }; + const key = createTestKey(pair); + + expect(key).toBe(getExpectedKey(contestId, taskId)); + }); + }); + + test.each<[string, string, string]>([ + ['', 'abc123_a', 'contestId must be a non-empty string'], + [' ', 'abc123_a', 'contestId must be a non-empty string'], + ['abc123', '', 'taskId must be a non-empty string'], + ['abc123', ' ', 'taskId must be a non-empty string'], + ])( + 'expects error when contest_id="%s" and task_id="%s"', + (contestId, taskId, expectedError) => { + expect(() => createContestTaskPairKey(contestId, taskId)).toThrow(expectedError); + }, + ); + + test('expects to preserve order of contest_id and task_id', () => { + const pair1 = testNormalPairs[0]; + const pair2 = { contestId: pair1.taskId, taskId: pair1.contestId }; + + expectKeysDifferent(pair1, pair2); + }); + + test('expects to include the colon separator', () => { + const pair = testNormalPairs[0]; + const key = createTestKey(pair); + + expect(key).toContain(':'); + expect(key.split(':')).toHaveLength(2); + }); + }); + + describe('Anomaly cases', () => { + test('expects anomaly cases with special characters to format correctly', () => { + // Filter out pipe character cases for basic testing + const specialCharCases = testAnomalyPairs.slice(4); + + testMultiplePairs(specialCharCases, ({ contestId, taskId }) => { + const pair = { contestId, taskId }; + const key = createTestKey(pair); + + expect(key).toBe(getExpectedKey(contestId, taskId)); + }); + }); + + test.each<[TestPair, string, string]>([ + [{ contestId: 'abc|123', taskId: 'abc123_a' }, 'abc|123:abc123_a', 'pipe in contest_id'], + [{ contestId: 'abc123', taskId: 'abc|123_a' }, 'abc123:abc|123_a', 'pipe in task_id'], + [{ contestId: 'abc|123', taskId: 'abc|123_a' }, 'abc|123:abc|123_a', 'pipes in both IDs'], + [ + { contestId: 'abc||123', taskId: 'task||id' }, + 'abc||123:task||id', + 'multiple consecutive pipes', + ], + ])('expects pipe characters to be preserved (%s)', (pair, expected) => { + const key = createTestKey(pair); + expect(key).toBe(expected); + }); + + test('expects to handle very long contest_id without issues', () => { + const pair = { contestId: 'a'.repeat(1000), taskId: 'task_a' }; + const key = createTestKey(pair); + + expect(key).toBe(getExpectedKey(pair.contestId, pair.taskId)); + }); + + test('expects to handle very long task_id without issues', () => { + const pair = { contestId: 'contest', taskId: 'b'.repeat(1000) }; + const key = createTestKey(pair); + + expect(key).toBe(getExpectedKey(pair.contestId, pair.taskId)); + }); + }); + + describe('Key validation', () => { + test('expects key to be parseable back into components', () => { + testMultiplePairs(testNormalPairs, ({ contestId, taskId }) => { + const pair = { contestId, taskId }; + const key = createTestKey(pair); + const [parsedContestId, parsedTaskId] = key.split(':'); + + expect(parsedContestId).toBe(contestId); + expect(parsedTaskId).toBe(taskId); + }); + }); + + test('expects key with colon separator to be splittable into two parts', () => { + const pair = testNormalPairs[0]; + const key = createTestKey(pair); + const parts = key.split(':'); + + expect(parts).toHaveLength(2); + expect(parts[0]).toBe(pair.contestId); + expect(parts[1]).toBe(pair.taskId); + }); + + test('expects all keys to follow the same format pattern', () => { + const keys = testNormalPairs.map((pair) => createTestKey(pair)); + + keys.forEach((key) => { + expect(key).toMatch(/^.+:.+$/); + }); + }); + + test('expects multiple keys to be unique', () => { + const keys = testNormalPairs.map((pair) => createTestKey(pair)); + const uniqueKeys = new Set(keys); + + expect(uniqueKeys.size).toBe(keys.length); + }); + + test('expects keys from different pairs to be different', () => { + const selectedPairs = testNormalPairs.slice(0, 3); + const keys = selectedPairs.map((pair) => createTestKey(pair)); + + for (let i = 0; i < keys.length; i++) { + for (let j = i + 1; j < keys.length; j++) { + expect(keys[i]).not.toBe(keys[j]); + } + } + }); + }); +}); From ae6fa670e390ab7721f926cca53fbc398e03fc89 Mon Sep 17 00:00:00 2001 From: Kato Hiroki Date: Sat, 25 Oct 2025 12:31:02 +0000 Subject: [PATCH 3/3] refactor: Add and remove tests (#2748) --- .../refactor-getMergedTasksMap/lesson.md | 45 ++++++++++++- src/test/lib/utils/contest_task_pair.test.ts | 63 +++++++------------ 2 files changed, 67 insertions(+), 41 deletions(-) diff --git a/docs/dev-notes/2025-10-25/refactor-getMergedTasksMap/lesson.md b/docs/dev-notes/2025-10-25/refactor-getMergedTasksMap/lesson.md index 9db45ec6c..20cb9bc8c 100644 --- a/docs/dev-notes/2025-10-25/refactor-getMergedTasksMap/lesson.md +++ b/docs/dev-notes/2025-10-25/refactor-getMergedTasksMap/lesson.md @@ -135,9 +135,50 @@ const pairs = { | テストデータ集約 | 保守性向上 | | beforeEach で初期化 | テスト間の独立性確保 | +### 6. **コード レビュー フィードバック対応** + +#### 指摘事項 + +| 項目 | 内容 | 対応 | +| ------------------ | ------------------------------------ | ------------------ | +| 弱い Assertion | `toContain()` では不正確 | `toBe()` に変更 | +| 冗長テスト | O(n²) の全ペア比較テストは不要 | O(n) Set検証に統一 | +| beforeEach削減 | イミュータブルデータの不要なコピー | 削除 | +| 特殊文字カバレッジ | **デリミタ文字（コロン）が未テスト** | 3ケース追加 | + +#### 学んだことと根拠 + +- **アサーション強度**: `toContain()` は部分一致で誤検知の可能性 → `toBe()` で完全一致を保証 +- **テスト効率**: 冗長な検証は実装と同じ複雑さ → 代表的パターンだけ実装 +- **パーサビリティ脆弱性**: デリミタ文字（`:`）が ID に含まれると `split(':')` で分割失敗 → **デリミタ自体のテストケースが必須** + +#### 対応結果 + +- ✅ Assertion を 4 個改善（`toContain()` → `toBe()` 統一） +- ✅ 冗長テスト 1 個削除（O(n²) → O(n)) +- ✅ コロン文字テスト 3 個追加（`contestId` のみ、`taskId` のみ、両方） +- ✅ **テスト総数: 26 → 29 個**（全成功 ✅） + +#### 重要な発見 + +**コロン文字は関数内で保存されるが、デリミタと同じため使用時に注意が必要** + +```typescript +// 実装例：コロンを含む ID +const key = createContestTaskPairKey('abc:123', 'task_a'); +// 結果: "abc:123:task_a" (コロン3個) + +// ⚠️ split(':') での分割に注意 +const [contestId, ...taskIdParts] = key.split(':'); +// contestId = 'abc', taskIdParts = ['123', 'task_a'] ❌ 失敗！ +``` + +**教訓**: デリミタ文字も含めてテストし、実装側で検証ルールを明確にすべき。 + ## テスト統計 -- **総テスト数**: 27 個（全成功 ✅） -- **パラメタライズテスト**: 2 グループ（合計 8 ケース） +- **総テスト数**: 29 個（全成功 ✅） +- **パラメタライズテスト**: 2 グループ（合計 11 ケース） - **ヘルパー関数**: 5 個 - **テストデータセット**: 3 グループ（normal, edge, anomaly） +- **特殊文字カバレッジ**: パイプ 4 ケース + コロン 3 ケース + その他 8 ケース diff --git a/src/test/lib/utils/contest_task_pair.test.ts b/src/test/lib/utils/contest_task_pair.test.ts index ce86f918a..8b9f83150 100644 --- a/src/test/lib/utils/contest_task_pair.test.ts +++ b/src/test/lib/utils/contest_task_pair.test.ts @@ -1,4 +1,4 @@ -import { describe, test, expect, beforeEach } from 'vitest'; +import { describe, test, expect } from 'vitest'; import { createContestTaskPairKey } from '$lib/utils/contest_task_pair'; @@ -9,7 +9,7 @@ describe('createContestTaskPairKey', () => { taskId: string; } - type TestPairs = TestPair[]; + type TestPairs = readonly TestPair[]; // Helper to create test pairs const pairs = { @@ -44,6 +44,9 @@ describe('createContestTaskPairKey', () => { { contestId: 'abc|123', taskId: 'abc|123_a' }, { contestId: 'abc||123', taskId: 'task||id' }, { contestId: 'abc.*+?^${}()', taskId: 'task[a-z]' }, + { contestId: 'abc:123', taskId: 'task_a' }, + { contestId: 'abc123', taskId: 'task:a' }, + { contestId: 'abc:123', taskId: 'task:a' }, { contestId: 'abc日本語123', taskId: 'task日本語a' }, { contestId: 'abc😀', taskId: 'task😀' }, { contestId: 'abc\n123', taskId: 'task\na' }, @@ -51,16 +54,6 @@ describe('createContestTaskPairKey', () => { ] as const, }; - let testNormalPairs: TestPairs; - let testEdgePairs: TestPairs; - let testAnomalyPairs: TestPairs; - - beforeEach(() => { - testNormalPairs = [...pairs.normal]; - testEdgePairs = [...pairs.edge]; - testAnomalyPairs = [...pairs.anomaly]; - }); - // Helper functions: // To generate expected key const getExpectedKey = (contestId: string, taskId: string) => `${contestId}:${taskId}`; @@ -98,7 +91,7 @@ describe('createContestTaskPairKey', () => { describe('Base cases', () => { test('expects to create a key with valid contest_id and task_id', () => { - const pair = testNormalPairs[0]; + const pair = pairs.normal[0]; const key = createTestKey(pair); expect(key).toBe(getExpectedKey(pair.contestId, pair.taskId)); @@ -106,27 +99,27 @@ describe('createContestTaskPairKey', () => { }); test('expects to create different keys for different contest_ids', () => { - const pair = testNormalPairs[0]; + const pair = pairs.normal[0]; const modifiedPair = { ...pair, contestId: 'abc124' }; expectKeysDifferent(pair, modifiedPair); }); test('expects to create different keys for different task_ids', () => { - const pair = testNormalPairs[0]; + const pair = pairs.normal[0]; const modifiedPair = { ...pair, taskId: 'abc100_b' }; expectKeysDifferent(pair, modifiedPair); }); test('expects to create consistent keys for the same inputs', () => { - const pair = testNormalPairs[0]; + const pair = pairs.normal[0]; expectKeysToBeConsistent(pair); }); test('expects to work with various contest types', () => { - testMultiplePairs(testNormalPairs, ({ contestId, taskId }) => { + testMultiplePairs(pairs.normal, ({ contestId, taskId }) => { const pair = { contestId, taskId }; const key = createTestKey(pair); @@ -142,10 +135,10 @@ describe('createContestTaskPairKey', () => { }); test('expects to work with numeric task identifiers', () => { - const pair = testNormalPairs[5]; // typical90_a + const pair = pairs.normal[5]; // typical90_a const key = createTestKey(pair); - expect(key).toContain(pair.taskId); + expect(key).toBe(getExpectedKey(pair.contestId, pair.taskId)); }); test('expects to work with long contest and task IDs', () => { @@ -162,7 +155,7 @@ describe('createContestTaskPairKey', () => { describe('Edge cases', () => { test('expects all edge cases to format correctly', () => { // Filter out empty string cases as they should throw - const validEdgePairs = testEdgePairs.filter( + const validEdgePairs = pairs.edge.filter( (pair) => pair.contestId.trim() !== '' && pair.taskId.trim() !== '', ); @@ -187,14 +180,14 @@ describe('createContestTaskPairKey', () => { ); test('expects to preserve order of contest_id and task_id', () => { - const pair1 = testNormalPairs[0]; + const pair1 = pairs.normal[0]; const pair2 = { contestId: pair1.taskId, taskId: pair1.contestId }; expectKeysDifferent(pair1, pair2); }); test('expects to include the colon separator', () => { - const pair = testNormalPairs[0]; + const pair = pairs.normal[0]; const key = createTestKey(pair); expect(key).toContain(':'); @@ -205,7 +198,7 @@ describe('createContestTaskPairKey', () => { describe('Anomaly cases', () => { test('expects anomaly cases with special characters to format correctly', () => { // Filter out pipe character cases for basic testing - const specialCharCases = testAnomalyPairs.slice(4); + const specialCharCases = pairs.anomaly.slice(4); testMultiplePairs(specialCharCases, ({ contestId, taskId }) => { const pair = { contestId, taskId }; @@ -224,7 +217,10 @@ describe('createContestTaskPairKey', () => { 'abc||123:task||id', 'multiple consecutive pipes', ], - ])('expects pipe characters to be preserved (%s)', (pair, expected) => { + [{ contestId: 'abc:123', taskId: 'task_a' }, 'abc:123:task_a', 'colon in contest_id'], + [{ contestId: 'abc123', taskId: 'task:a' }, 'abc123:task:a', 'colon in task_id'], + [{ contestId: 'abc:123', taskId: 'task:a' }, 'abc:123:task:a', 'colons in both IDs'], + ])('expects special characters to be preserved (%s)', (pair, expected) => { const key = createTestKey(pair); expect(key).toBe(expected); }); @@ -246,7 +242,7 @@ describe('createContestTaskPairKey', () => { describe('Key validation', () => { test('expects key to be parseable back into components', () => { - testMultiplePairs(testNormalPairs, ({ contestId, taskId }) => { + testMultiplePairs(pairs.normal, ({ contestId, taskId }) => { const pair = { contestId, taskId }; const key = createTestKey(pair); const [parsedContestId, parsedTaskId] = key.split(':'); @@ -257,7 +253,7 @@ describe('createContestTaskPairKey', () => { }); test('expects key with colon separator to be splittable into two parts', () => { - const pair = testNormalPairs[0]; + const pair = pairs.normal[0]; const key = createTestKey(pair); const parts = key.split(':'); @@ -267,7 +263,7 @@ describe('createContestTaskPairKey', () => { }); test('expects all keys to follow the same format pattern', () => { - const keys = testNormalPairs.map((pair) => createTestKey(pair)); + const keys = pairs.normal.map((pair) => createTestKey(pair)); keys.forEach((key) => { expect(key).toMatch(/^.+:.+$/); @@ -275,21 +271,10 @@ describe('createContestTaskPairKey', () => { }); test('expects multiple keys to be unique', () => { - const keys = testNormalPairs.map((pair) => createTestKey(pair)); + const keys = pairs.normal.map((pair) => createTestKey(pair)); const uniqueKeys = new Set(keys); expect(uniqueKeys.size).toBe(keys.length); }); - - test('expects keys from different pairs to be different', () => { - const selectedPairs = testNormalPairs.slice(0, 3); - const keys = selectedPairs.map((pair) => createTestKey(pair)); - - for (let i = 0; i < keys.length; i++) { - for (let j = i + 1; j < keys.length; j++) { - expect(keys[i]).not.toBe(keys[j]); - } - } - }); }); });