🧪 [testing improvement] Add tests for validateSlug#882
Conversation
Release: staging -> main (2026-05-21 07:43)
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? |
📝 WalkthroughWalkthroughThe PR exports the ChangesSlug Validation Refactoring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
このリポジトリでは staging 先行フローを採用しています。PR のターゲットを |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![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 exports the validateSlug function and introduces unit tests to validate slug requirements, including length and character constraints. Feedback suggests enhancing the function by using constants for length limits, handling empty strings, and refining the regex. Additionally, the reviewer recommended adding boundary tests for length limits while keeping the test suite concise by removing redundant assertions.
| export function validateSlug(slug: unknown): string | null { | ||
| if (typeof slug !== "string") return "slug is required"; | ||
| const s = slug.trim().toLowerCase(); | ||
| if (s.length < 3 || s.length > 40) return "slug must be 3–40 characters"; |
There was a problem hiding this comment.
The validateSlug function should return "slug is required" for empty or whitespace-only strings. Additionally, use pre-defined constants for the length limits (3 and 40) instead of hardcoding them to improve maintainability. The SLUG_RE regex contains a redundant branch for single characters that can be removed, and the error message for invalid characters should clarify that slugs cannot start or end with a hyphen.
| export function validateSlug(slug: unknown): string | null { | |
| if (typeof slug !== "string") return "slug is required"; | |
| const s = slug.trim().toLowerCase(); | |
| if (s.length < 3 || s.length > 40) return "slug must be 3–40 characters"; | |
| export function validateSlug(slug: unknown): string | null { | |
| if (typeof slug !== "string") return "slug is required"; | |
| const s = slug.trim().toLowerCase(); | |
| if (s.length === 0) return "slug is required"; | |
| if (s.length < MIN_SLUG_LENGTH || s.length > MAX_SLUG_LENGTH) return "slug must be " + MIN_SLUG_LENGTH + "-" + MAX_SLUG_LENGTH + " characters"; |
References
- Use pre-defined constants for validation logic instead of hardcoding values to improve maintainability.
| it("returns null for valid slugs", () => { | ||
| expect(validateSlug("valid-slug")).toBeNull(); | ||
| expect(validateSlug("valid")).toBeNull(); | ||
| expect(validateSlug("v123")).toBeNull(); | ||
| expect(validateSlug("123-abc")).toBeNull(); | ||
| // Should trim and lowercase before validating | ||
| expect(validateSlug(" VALID-SLUG ")).toBeNull(); | ||
| expect(validateSlug("Valid-Slug")).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
Add boundary value tests for the slug length (exactly 3 and 40 characters). Comprehensive coverage for other normal success cases should be deferred to subsequent PRs to keep the test suite minimal.
it("returns null for boundary length slugs", () => {
expect(validateSlug("abc")).toBeNull();
expect(validateSlug("a".repeat(40))).toBeNull();
});References
- Do not maintain redundant assertions in tests; keep them minimal.
- Defer comprehensive test coverage for all normal/success cases to subsequent PRs, provided that boundary tests have been added.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| describe("orgs routes", () => { | ||
|
|
||
| describe("validateSlug", () => { |
There was a problem hiding this comment.
beforeEach の位置がずれています。新しい validateSlug describe ブロックの追加により、beforeEach がネストされた describe の後に配置されました。Vitest/Jest では動作に影響しませんが、コードを読む際に可読性が下がります。beforeEach を describe("orgs routes") 直下の先頭に移動することを推奨します。
| describe("orgs routes", () => { | |
| describe("validateSlug", () => { | |
| describe("orgs routes", () => { | |
| beforeEach(() => { | |
| vi.restoreAllMocks(); | |
| vi.resetModules(); | |
| mockDb = createMockDb(); | |
| }); | |
| describe("validateSlug", () => { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/api/src/routes/__tests__/orgs.test.ts
Line: 28-30
Comment:
`beforeEach` の位置がずれています。新しい `validateSlug` describe ブロックの追加により、`beforeEach` がネストされた describe の後に配置されました。Vitest/Jest では動作に影響しませんが、コードを読む際に可読性が下がります。`beforeEach` を `describe("orgs routes")` 直下の先頭に移動することを推奨します。
```suggestion
describe("orgs routes", () => {
beforeEach(() => {
vi.restoreAllMocks();
vi.resetModules();
mockDb = createMockDb();
});
describe("validateSlug", () => {
```
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("returns error if length < 3", () => { | ||
| expect(validateSlug("ab")).toBe("slug must be 3–40 characters"); | ||
| expect(validateSlug("a")).toBe("slug must be 3–40 characters"); | ||
| expect(validateSlug(" ab ")).toBe("slug must be 3–40 characters"); // Testing trim | ||
| }); | ||
|
|
||
| it("returns error if length > 40", () => { | ||
| const longSlug = "a".repeat(41); | ||
| expect(validateSlug(longSlug)).toBe("slug must be 3–40 characters"); | ||
| }); |
There was a problem hiding this comment.
長さチェックのちょうど 3 文字・40 文字のテストが含まれていません。< や > を <= や >= に誤変更した場合にオフバイワンバグを検出できないリスクがあります。validateSlug("abc") や "a".repeat(40) のケースを追加することを推奨します。
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/api/src/routes/__tests__/orgs.test.ts
Line: 38-47
Comment:
**境界値テストが不足しています**
長さチェックのちょうど 3 文字・40 文字のテストが含まれていません。`<` や `>` を `<=` や `>=` に誤変更した場合にオフバイワンバグを検出できないリスクがあります。`validateSlug("abc")` や `"a".repeat(40)` のケースを追加することを推奨します。
How can I resolve this? If you propose a fix, please make it concise.1 participant
🎯 What: The testing gap in
validateSlugfunction insideapps/api/src/routes/orgs.tswas addressed. The function was modified to be exported so it can be explicitly unit-tested.📊 Coverage: A new
describeblock was added inapps/api/src/routes/__tests__/orgs.test.tsproviding 100% path coverage forvalidateSlug. Scenarios covered include:✨ Result: The improvement in test coverage prevents future regressions by explicitly pinning down the behavior of
validateSlug. All unit tests passed without negatively affecting any part of the project.PR created automatically by Jules for task 10952334015309556050 started by @is0692vs
Summary by CodeRabbit
Tests
Refactor
Greptile Summary
validateSlug関数をエクスポートし、専用のユニットテストを追加する変更です。既存のロジックには一切手を加えず、テスト可能性の向上のみを目的としています。apps/api/src/routes/orgs.ts—validateSlug関数にexportを追加（1行のみ変更）apps/api/src/routes/__tests__/orgs.test.ts— 非文字列入力・長さチェック・無効文字・連続ハイフン・正常値など全パスをカバーする describe ブロックを追加Confidence Score: 4/5
既存の実装ロジックに変更はなく、テストの追加のみのため、マージしても安全です。
テスト自体は全コードパスをカバーしており論理的に正しいですが、長さチェックの境界値（ちょうど 3 文字・40 文字）が未テストのため、将来オフバイワンが混入した場合に検出できないリスクが残ります。
テストファイル
apps/api/src/routes/__tests__/orgs.test.tsで境界値テストとbeforeEachの配置を確認してください。Important Files Changed
validateSlugのユニットテストを追加。全コードパスをカバーしているが、境界値テストが不足しておりbeforeEachの配置に可読性の改善余地あり。validateSlug関数にexportキーワードを追加したのみ。関数本体に変更なし。Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[validateSlug入力] --> B{typeof slug === 'string'?} B -- No --> E1["'slug is required'"] B -- Yes --> C[trim + toLowerCase] C --> D{length < 3 OR length > 40?} D -- Yes --> E2["'slug must be 3–40 characters'"] D -- No --> F{SLUG_RE.test失敗?} F -- Yes --> E3["'slug must contain only...'"] F -- No --> G{includes '--'?} G -- Yes --> E4["'slug must not contain consecutive hyphens'"] G -- No --> H[null 返却]Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "test: improve coverage for validateSlug ..." | Re-trigger Greptile