feat: add vapor-unit-test skills#620
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new Changesvapor-unit-test AI Skill
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.claude/skills/vapor-unit-test/references/examples/util.test.ts (1)
45-48: ⚡ Quick winTighten the invalid-input assertion to avoid false positives.
Line 48 uses a generic
.toThrow(), which can pass on unrelated exceptions. Assert an expected message/pattern so the test validates the intended guard path.Suggested patch
- ).toThrow(); + ).toThrow(/string|function/i);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/skills/vapor-unit-test/references/examples/util.test.ts around lines 45 - 48, The test for resolveClassName at line 45-48 uses a generic `.toThrow()` assertion without specifying an expected error message or pattern, which can pass if any exception is thrown rather than validating the specific guard condition being tested. Add an expected error message string or regex pattern as an argument to the `.toThrow()` call to validate that the error thrown is specifically for the invalid resolver input check (when the resolver is neither a string nor a function), ensuring the test validates the intended error path rather than accepting unrelated exceptions..claude/skills/vapor-unit-test/references/examples/breadcrumb.test.tsx (1)
44-50: ⚡ Quick winMake the
prop: currentblock actually validate prop-driven behavior.The current test repeats the same positive assertion as the ARIA block, but doesn’t exercise the prop boundary (e.g.,
currentabsent vs present). This weakens the value of the “prop: current” section.Suggested patch
- describe('prop: current', () => { - it('sets aria-current="page" on the active item', () => { - const rendered = render(<BreadcrumbTest />); - const currentItem = rendered.getByRole('link', { name: 'Products' }); - - expect(currentItem).toHaveAttribute('aria-current', 'page'); - }); - }); + describe('prop: current', () => { + it('sets `aria-current="page"` when current is true', () => { + const rendered = render(<BreadcrumbTest current />); + const currentItem = rendered.getByRole('link', { name: 'Products' }); + expect(currentItem).toHaveAttribute('aria-current', 'page'); + }); + }); @@ -const BreadcrumbTest = (props: Breadcrumb.Root.Props) => ( +const BreadcrumbTest = ({ current = false, ...props }: Breadcrumb.Root.Props & { current?: boolean }) => ( <Breadcrumb.Root {...props}> <Breadcrumb.Item href="home">Home</Breadcrumb.Item> <Breadcrumb.Separator /> - <Breadcrumb.Item href="products" current> + <Breadcrumb.Item href="products" current={current}> Products </Breadcrumb.Item> </Breadcrumb.Root> );Also applies to: 54-60
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/skills/vapor-unit-test/references/examples/breadcrumb.test.tsx around lines 44 - 50, The test within the describe block labeled "prop: current" is repeating the same positive assertion as the ARIA block instead of validating prop-driven behavior. Refactor this test to exercise the boundary of the current prop by testing what happens when the current prop is present versus absent. This should demonstrate how the component behaves differently based on whether the current prop is passed, rather than just asserting the same positive case that is already covered elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/vapor-unit-test/references/examples/breadcrumb.test.tsx:
- Around line 25-34: In the keyboard navigation test (the 'should support
keyboard navigation via Tab and Enter' test), the render call is not following
the documented convention. Refactor the render statement to assign the result to
a const variable named rendered (i.e., const rendered = render(...)) instead of
calling render directly without assignment. This ensures the example follows the
canonical pattern documented in the guide.
In @.claude/skills/vapor-unit-test/SKILL.md:
- Around line 64-71: The fenced code block displaying the file tree structure
lacks a language identifier on the opening fence, which violates the
markdownlint MD040 rule. Add a language tag to the opening fence by changing ```
to ```text to indicate the content is plain text, which will satisfy the
markdown linting requirement and prevent pipeline failures.
---
Nitpick comments:
In @.claude/skills/vapor-unit-test/references/examples/breadcrumb.test.tsx:
- Around line 44-50: The test within the describe block labeled "prop: current"
is repeating the same positive assertion as the ARIA block instead of validating
prop-driven behavior. Refactor this test to exercise the boundary of the current
prop by testing what happens when the current prop is present versus absent.
This should demonstrate how the component behaves differently based on whether
the current prop is passed, rather than just asserting the same positive case
that is already covered elsewhere.
In @.claude/skills/vapor-unit-test/references/examples/util.test.ts:
- Around line 45-48: The test for resolveClassName at line 45-48 uses a generic
`.toThrow()` assertion without specifying an expected error message or pattern,
which can pass if any exception is thrown rather than validating the specific
guard condition being tested. Add an expected error message string or regex
pattern as an argument to the `.toThrow()` call to validate that the error
thrown is specifically for the invalid resolver input check (when the resolver
is neither a string nor a function), ensuring the test validates the intended
error path rather than accepting unrelated exceptions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2315e470-ea70-4af7-bb87-7f59f8e722a9
📒 Files selected for processing (7)
.claude/skills/vapor-unit-test/SKILL.md.claude/skills/vapor-unit-test/references/component-tests.md.claude/skills/vapor-unit-test/references/examples/breadcrumb.test.tsx.claude/skills/vapor-unit-test/references/examples/use-hook.test.ts.claude/skills/vapor-unit-test/references/examples/util.test.ts.claude/skills/vapor-unit-test/references/general-tests.md.claude/skills/vapor-unit-test/references/hook-tests.md
| it('should support keyboard navigation via Tab and Enter', async () => { | ||
| const onClick = vi.fn(); | ||
| render( | ||
| <Breadcrumb.Root> | ||
| <Breadcrumb.Item href="home">Home</Breadcrumb.Item> | ||
| <Breadcrumb.Item href="away" onClick={onClick}> | ||
| Away | ||
| </Breadcrumb.Item> | ||
| </Breadcrumb.Root>, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Align the example with the rendered convention defined in the guide.
Line 27 bypasses the documented const rendered = render(...) pattern. Since this file is a canonical example, it should strictly follow the convention it teaches.
Suggested patch
- render(
+ const rendered = render(
<Breadcrumb.Root>
<Breadcrumb.Item href="home">Home</Breadcrumb.Item>
<Breadcrumb.Item href="away" onClick={onClick}>
Away
</Breadcrumb.Item>
</Breadcrumb.Root>,
);📝 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.
| it('should support keyboard navigation via Tab and Enter', async () => { | |
| const onClick = vi.fn(); | |
| render( | |
| <Breadcrumb.Root> | |
| <Breadcrumb.Item href="home">Home</Breadcrumb.Item> | |
| <Breadcrumb.Item href="away" onClick={onClick}> | |
| Away | |
| </Breadcrumb.Item> | |
| </Breadcrumb.Root>, | |
| ); | |
| it('should support keyboard navigation via Tab and Enter', async () => { | |
| const onClick = vi.fn(); | |
| const rendered = render( | |
| <Breadcrumb.Root> | |
| <Breadcrumb.Item href="home">Home</Breadcrumb.Item> | |
| <Breadcrumb.Item href="away" onClick={onClick}> | |
| Away | |
| </Breadcrumb.Item> | |
| </Breadcrumb.Root>, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/vapor-unit-test/references/examples/breadcrumb.test.tsx
around lines 25 - 34, In the keyboard navigation test (the 'should support
keyboard navigation via Tab and Enter' test), the render call is not following
the documented convention. Refactor the render statement to assign the result to
a const variable named rendered (i.e., const rendered = render(...)) instead of
calling render directly without assignment. This ensures the example follows the
canonical pattern documented in the guide.
| ``` | ||
| <area>/ | ||
| ├── target.ts | ||
| ├── target.test.ts | ||
| └── __testfixtures__/ | ||
| ├── case-a.input.tsx | ||
| └── case-a.output.tsx | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to the fenced example block.
Line 64 opens a fenced code block without a language, which triggers markdownlint MD040 and can fail doc-lint pipelines depending on config.
Suggested patch
-```
+```text
<area>/
├── target.ts
├── target.test.ts
└── __testfixtures__/
├── case-a.input.tsx
└── case-a.output.tsx</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 64-64: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/vapor-unit-test/SKILL.md around lines 64 - 71, The fenced
code block displaying the file tree structure lacks a language identifier on the
opening fence, which violates the markdownlint MD040 rule. Add a language tag to
the opening fence by changing ``` to ```text to indicate the content is plain
text, which will satisfy the markdown linting requirement and prevent pipeline
failures.
Source: Linters/SAST tools
MaxLee-dev
left a comment
MaxLee-dev
left a comment
There was a problem hiding this comment.
I understand that the content you've provided serves as a convention for test cases. For this type of content, Anthropic officially recommends adding it under rules.
Since skills are only triggered based on the conditions defined in their descriptions, how about moving it to [rules], which are triggered based on file paths? Please let me know what you think!
| }); | ||
| ``` | ||
|
|
||
| `cleanup`은 RTL이 자동으로 호출하므로 **기본적으로 추가하지 않습니다.** 한 `it()` 내부에서 여러 번 `render`를 호출하는 등 특수한 경우에만 명시적으로 `afterEach(cleanup)` 또는 `cleanup()`을 직접 호출하세요. |
There was a problem hiding this comment.
In this case, rather than manually calling cleanup within a single test, I think it would be more appropriate to separate the test cases.
Therefore, instead of guiding the use of cleanup as an example for special cases, how about updating the convention to recommend using rerender or separating the test cases?
Additionally, for edge cases where manual DOM cleanup is actually required, unmount is generally used much more often than cleanup. Since we are writing component-level tests, there will be scenarios where we need to test specific behaviors upon unmounting (e.g., event listener removal for a Sheet or Dialog). For these situations, specifically calling unmount() on the target component is much more desirable than using cleanup() to wipe out the entire global DOM!
There was a problem hiding this comment.
cleanup은 내부적으로 unmount()를 포함하고 있기도 하고, 많이 쓰는 패턴으로 알고 있긴 합니다..! 그리고 afterEach(cleanup)이 의미가 있었던 이유는 매번 동일한 테스트베드를 호출하는 번거로움을 줄이기 위해 아래와 같이 작성하고 있기 때문이었습니다!
describe('xxx', () => {
let rendered: RenderResult;
beforeEach(() => {
rendered = render(<TestBed />);
})
it('can test without render testbed', () => {
const element = rendered.getByRole('textbox');
// ...
})
})이 때 사전에 생성된 테스트베드 대신 새로운 케이스를 띄워야 하는 경우 DOM을 지우고 새롭게 render하기 위해 cleanup을 호출해야 했어요.
다만 제가 놓친 부분이 있어서 정리해서 제안드리자면 다음과 같습니다!
- vitest 설정 파일에
globals: true를 지정해주면 RTL 같은 라이브러리에 의해 DOM이 auto-cleanup 된다고 하네요! 그래서afterEach(cleanup)은 더이상 필요없을 것 같아요. - 그리고 제안해주신 특수 케이스에 대한 사용을 제거하기 위해서는
rendered = beforeEach()와 같은 사용법을 지양해야 하는데, 조금 번거롭긴 하겠지만 그래도 각 테스트케이스에서 사용할 테스트베드를 직접 설정해준다는 의미로 해당 가이드는 제거해두도록 하겠습니다!
|
|
||
| ## describe 그룹 | ||
|
|
||
| 테스트가 **2개 이상의 카테고리**로 나뉠 때만 nested describe로 묶습니다. 카테고리가 1개거나 테스트가 몇 개 안 되면 nested 없이 최상위 `describe`에 평탄하게 둡니다 — 불필요한 중첩은 가독성을 해칩니다. |
There was a problem hiding this comment.
The phrasing in "카테고리가 1개거나 테스트가 몇 개 안 되면 nested 없이" feels a bit ambiguous.
How about updating it to "카테고리가 1개거나, 테스트 케이스의 케이스가 적어 카테고리를 분류할 수 없는 경우 nested 없이" for better clarity?
2 participants
Description of Changes
Summary by CodeRabbit
useIntervalhook (fake timers + cleanup), andstateful-propsutilities (merging/resolution behavior).Summary
Adds
vapor-unit-testskill — convention guide for writing Vitest + React Testing Libraryunit tests across the vapor-ui monorepo. Triggered by phrases like "테스트 추가해줘", "write
unit tests", "커버리지 보강", "test 좀 짜줘" — no explicit skill invocation required.
Structure
Progressive disclosure: skill body stays short, dispatches to a single reference per target.
SKILL.mdcarries the routing table + rules that apply regardless of target (fileplacement, fixture layout, imports, naming, mocks, coverage). Each reference is loaded only
when its target matches — keeps Claude's context lean.
Decisions
materially different assertion vocabulary (RTL queries vs
renderHook/actvsinput→output), and bundling them into one doc would force Claude to read irrelevant guidance
on every invocation.
and let Claude pattern-match on real shapes.
__tests__/for cross-file flows — keeps single-file targetsdiscoverable; isolates multi-file integration scenarios where co-location would clutter the
source folder.
__testfixtures__/— single glob name (jscodeshift convention)used across all domains so Vitest/tsconfig can exclude them in one place; prevents
partial-source fixtures from breaking build/typecheck.
delegation rule (one-line forward to external lib = skip) so the guide doesn't push toward
testing third-party code.
red→green→refactor would conflict with how the skill is actually invoked.
size/variant/color/shapebelong to visual regression;declaring this up front prevents redundant unit tests.
Screenshots
Checklist
Before submitting the PR, please make sure you have checked all of the following items.