feat: test skill 추가#647
Conversation
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 제안하는 리뷰어
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
claude.md (1)
367-367:⚠️ Potential issue | 🟡 Minor
test.md참조가 오래된 경로를 가리키고 있을 수 있습니다.Line 367의
test.md참조가 현재 PR에서 추가된.claude/skills/test/SKILL.md와 일치하지 않습니다. 테스트 가이드가 skill 파일로 이동했다면 이 참조도 업데이트하는 것이 좋겠습니다.제안하는 수정
-- **테스트 가이드**: `test.md` 파일 참고 +- **테스트 가이드**: `.claude/skills/test/SKILL.md` 파일 참고 (`/test` skill로 자동 로드)
🤖 Fix all issues with AI agents
In @.claude/skills/test/SKILL.md:
- Around line 32-36: Add a language identifier to the Markdown code block that
shows the directory tree so markdownlint MD040 is resolved: update the block
containing "[Entity]FixtureBuilder.java" and "[Entity]Fixture.java" to start
with ```text (or ```plaintext) instead of plain ```, keeping the rest of the
block unchanged.
- Around line 104-123: The example incorrectly accesses a private final field
chatRoomFixtureBuilder on ChatRoomFixture; change the example to access the
builder via a public accessor or inject the builder into the test instead—e.g.,
call a getter on ChatRoomFixture (e.g.,
ChatRoomFixture.getChatRoomFixtureBuilder()) or add an `@Autowired`
ChatRoomFixtureBuilder field in the test and use that builder to call
chatRoom().isGroup(true).mentoringId(100L).create(); ensure you reference the
ChatRoomFixture class and its chatRoom(...) convenience method and the builder's
chatRoom() method when making the change.
- Around line 237-246: The exception assertion example is incorrect: replace the
use of assertThatCode(() -> method()) with assertThatThrownBy(() -> method()) so
the test actually asserts that an exception is thrown; update the sample to call
assertThatThrownBy with the same thrown-exception expectations
(isInstanceOf(CustomException.class).hasMessage("error message")) and leave the
assertAll multi-assert example unchanged.
- 응답 대기시 알람발송 - 컨벤션 어겼을 때 훅 작동
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.claude/hooks/notify.sh:
- Around line 1-2: The notify.sh script in .claude/hooks is dead code because
settings.json's Notification hook uses an inline osascript command; either
update settings.json to call the hook script (e.g., point the Notification
command to ".claude/hooks/notify.sh") so the script with timestamp is used, or
delete .claude/hooks/notify.sh and keep the inline osascript in
settings.json—make the change consistently (modify settings.json's Notification
entry or remove the file) and verify the notification still works; target
symbols: .claude/hooks/notify.sh and the Notification entry in settings.json.
In @.claude/settings.json:
- Around line 6-16: The Notification hook contains an unsupported "matcher"
field and uses an inline osascript command; remove the unsupported "matcher" key
from the "Notification" object and replace the inline command "osascript -e
'display notification ...'" with a call to the existing notify.sh script (e.g.,
invoke notify.sh with the same title/message so it uses its timestamp and sound
behavior). Update the "hooks" entry under "Notification" to use type "command"
and command "./notify.sh" (or the project-specific notify.sh invocation) so
Notification uses the standardized notify.sh implementation instead of the
inline osascript.
🧹 Nitpick comments (3)
.claude/hooks/post-edit-check.py (3)
9-10: 조건 순서가 살짝 아쉬워요.
not file_path.endswith(".java") or not file_path에서not file_path체크는 사실상 도달하지 않습니다 (빈 문자열이면 첫 번째 조건에서 이미True). 가독성을 위해 순서를 바꾸는 것을 추천드립니다.✨ 제안
-if not file_path.endswith(".java") or not file_path: +if not file_path or not file_path.endswith(".java"):
12-17:Exception전체를 무시하는 것은 디버깅 시 불편할 수 있어요.
- 정적 분석(Ruff BLE001)에서도 지적하고 있습니다.
- 훅 스크립트 특성상 조용히 종료하는 것이 합리적이긴 하지만, 최소한
(FileNotFoundError, PermissionError, OSError)정도로 좁혀주면 예상치 못한 에러를 놓치지 않을 수 있습니다.✨ 제안
-except Exception: +except (FileNotFoundError, PermissionError, OSError): sys.exit(0)
30-43: Entity 필드 감지 정규식이 일부 흔한 패턴을 놓칠 수 있어요.
- 초기화가 있는 필드:
private String name = "default";—= value부분이 패턴에 없어서 매칭되지 않습니다.- 중첩 제네릭:
private Map<String, List<String>> map;—<[^>]+>는 내부>에서 끊깁니다.- 배열 타입:
private String[] names;—\w+뒤에[]가 허용되지 않습니다.실제로 이런 필드들은 경고 없이 통과하게 되므로, 체크의 효과가 제한될 수 있습니다.
✨ 더 넓은 범위를 커버하는 패턴 제안
- field_pattern = re.compile(r"^\s+private\s+\w+(?:<[^>]+>)?\s+\w+;") + field_pattern = re.compile(r"^\s+private\s+\S+\s+\w+\s*(?:=.*)?;")이렇게 하면 타입 부분을
\S+로 느슨하게 잡아 제네릭/배열을 포함하고,(?:=.*)?로 초기화 구문도 허용합니다. 단, 너무 느슨하면 false positive가 늘어날 수 있으니 팀과 논의해 주세요.
관련 이슈
작업 내용
claude.md에 있던 테스트 관련 컨벤션을 별도의 skill로 뺐습니다.
테스트를 모든 세션에서 작업하지 않는데 매번 claude가 이 테스트 컨벤션을 읽는 게 비효율적인 거 같습니다!
이제 테스트 작업 시에 /test로 skill을 활성화시키면 될 거 같습니다. 사실 별도로 안해도 테스트 관련 작업 시키면 디스크립션을 이해하고 알아서 활성화 시키는 것도 확인했습니다.
추가로 세레나 MCP도 저는 활성화 시켜서 위에 사진 보면 세레나 MCP가 작동하는 걸 확인할 수 있습니다~
=====추가 수정 =====
앗 작업하다보니 욕심이 생겨서 hook도 추가했습니다.
저희 컨벤션 실수하는 경우가 많은데 만약 클로드코드 쓰신다면 hook이 작동해서 개행이나 와일드카드 사용, 혹은 @Colum누락시 알려주도록 추가했고 클로드 코드 작업 기다리다보면 다른 짓(?) 하는 경우가 많은데 사용자 답변 기다릴 경우에 알람 주도록 추가했어요~
(참고로 item에서만 테스트해서 다른 툴에서 되는진 모르겠습니다.)
특이 사항
리뷰 요구사항 (선택)