test: 아바타 업로드 Feature 테스트와 UploadStorage seam - #102
Conversation
UploadedFile::move() 가 is_uploaded_file()·move_uploaded_file() 에 직접 의존해 CLI 테스트로 태울 수 없었다. 저장 동작 한 줄만 UploadStorage 로 빼고 테스트에서 바꿔 끼워, 나머지 업로드 로직을 전부 Feature 테스트로 덮는다. - 검증 실패 분기·랜덤 파일명·옛 파일 삭제 순서·avatar 컬럼 갱신까지 커버 - 가짜 저장기도 copy() 로 실제 파일을 만든다. 이름만 돌려주면 파일 존재를 전제로 한 뒷단 로직이 통과하는 척만 하게 된다 - 덮지 못하는 것은 move_uploaded_file() 호출 한 줄(프레임워크 코드) Closes #95 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough프로필 아바타 저장을 Changes아바타 업로드 저장소
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant Profile
participant UploadStorage
participant UploadDirectory
Browser->>Profile: 아바타 업로드 요청
Profile->>UploadStorage: UploadedFile 저장 요청
UploadStorage->>UploadDirectory: 임의 이름으로 파일 이동
UploadDirectory-->>UploadStorage: 저장 파일명 반환
UploadStorage-->>Profile: 저장 파일명 반환
Profile-->>Browser: 프로필 응답
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/Feature/ProfileAvatarUploadTest.php`:
- Around line 87-89: Update makeTempPng() and
testRejectsNonImageDisguisedAsPng() to use tempnam()’s returned path directly,
without appending .png; keep the desired PNG filename supplied separately
through attach()’s name argument.
- Around line 46-47: Update the test teardown to call
Services::resetSingle('uploadStorage') after the FakeUploadStorage mock is
injected, ensuring the shared uploadStorage instance is cleared between tests.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e8ac6b6d-b8b3-4111-87d7-c9425f4fdbbd
📒 Files selected for processing (5)
app/Config/Services.phpapp/Controllers/Profile.phpapp/Libraries/UploadStorage.phptests/Feature/ProfileAvatarUploadTest.phptests/_support/Libraries/FakeUploadStorage.php
CodeRabbit 지적 2건. 둘 다 실증하고 고쳤다. 1) tempnam() 반환값에 .png 를 덧붙여 원본 임시 파일이 추적되지 않은 채 남았다. 반환값을 그대로 쓴다(업로드 파일명은 attach() 의 $name 이 정한다). 2) 주입한 가짜 저장기가 다른 테스트 클래스까지 살아남았다. 프로브로 확인: ZzLeakProbe 가 FakeUploadStorage 를 받았다. 원인은 프레임워크의 비대칭이다. injectMock() 은 $instances 에 준 이름 그대로, $mocks 에는 소문자로 넣는데 resetSingle() 은 소문자 키만 지운다. 그래서 $instances['uploadStorage'] 는 공개 API 로 지울 수 없고, service() 는 파라미터가 없을 때 Services::get() 으로 그 $instances 를 그대로 읽는다. 소문자 이름으로 주입해 두 키를 맞췄다. 이 사고는 조용하다 — 다른 테스트가 업로드를 안 하면 아무도 안 깨진다. 그래서 방지망 테스트를 남겼고, camelCase 로 되돌리면 그 테스트만 깨진다. 275/275. 뮤테이션 4건 재확인, 임시 파일 잔여 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
리뷰 2건 모두 반영했습니다( 1) 2) 가짜 저장기 누수 — 이것도 실재했습니다. 임시 프로브 테스트를 뒤에 붙여 실증했습니다. 다만 제안하신
그래서 camelCase 로 주입하면 공개 API 로는 치울 방법이 없습니다. 소문자 이름으로 주입해 두 키를 맞췄습니다. 이 사고는 조용합니다 — 다른 테스트가 업로드를 안 하면 아무도 안 깨집니다. 그래서 275/275 통과. 기존 뮤테이션 4건도 재확인했습니다. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
이슈 #95 입니다.
Profile::saveUploadedAvatar()경로를 자동 테스트로 덮었습니다.이슈 본문의 전제가 일부 틀렸습니다
임시 프로브 테스트로 실제 확인했습니다(확인 후 삭제).
$_FILES주입은 공식 API로 됩니다 —service('superglobals')->setFilesArray().FileCollection::populateFiles()가 이 서비스를 거치므로 정상적인UploadedFile이 만들어집니다.is_image·mime_in·max_size는getError()·getSize()·mime 만 보고isValid()를 쓰지 않습니다.isValid()에 의존하는 건uploaded[]규칙뿐인데 이 프로젝트는 안 씁니다.$file->move()한 곳뿐입니다.즉 테스트 불가 영역은 업로드 경로 전체가 아니라 프레임워크 코드 한 줄이었습니다.
접근
그 한 줄만
App\Libraries\UploadStorage뒤로 보내고, 테스트에서Services::injectMock()으로 바꿔 낍니다.검토했다가 버린 대안 두 가지:
UploadedFile서브클래스 주입 —FileCollection::createFileObject()가new UploadedFile(...)을 하드코딩해FileCollection·IncomingRequest까지 갈아끼워야 합니다. 프레임워크 내부 결합이라 업그레이드에 취약합니다.uopz확장으로 내장 함수 대체 — 프로덕션 코드는 안 건드리지만 로컬·CI 양쪽에 PHP 확장이 필요해집니다. 강좌용 저장소에 진입 장벽을 만들 이유가 없습니다.적용 범위는 아바타만으로 했습니다. 글 대표 이미지(
Posts.php:305)도 같은 모양이지만, 테스트 없는 리팩터링을 남기지 않으려고 그쪽 테스트와 함께 후속 이슈에서 바꾸겠습니다.가짜 저장기가 실제 파일을 만듭니다
FakeUploadStorage는move_uploaded_file()만copy()로 바꾸고, 파일명은 진짜getRandomName()을 씁니다. 이름만 돌려주는 가짜였다면 "새 파일이 저장된 뒤 옛 파일을 지운다"는 순서나deleteAvatarFile()의is_file()분기가 통과하는 척만 하게 됩니다.테스트 4건
avatar컬럼 = 저장 파일명, 파일이 실제로 생성됨image/png라고 위장세 번째는
mime_in이 클라이언트가 신고한 타입이 아니라 실제 감지된 mime(getMimeType())을 본다는 점을 검증합니다. 네 번째는getSize()가$_FILES['size']를 우선 쓰므로 큰 파일을 만들지 않았습니다 — 실제 업로드에서도 PHP 가 전송 크기를 그 자리에 넣습니다.확인
274/274 통과(기존 270 + 신규 4). 뮤테이션으로 각 테스트가 자기 결함에만 반응하는 것을 확인했습니다.
is_image+mime_in제거max_size제거전체 스위트에서 처음엔 5건이 깨졌습니다.
setFilesArray()가$_FILES전역까지 덮어써서(Superglobals.php:408) 주입값이 뒤 테스트로 샜고, 그쪽이 이미 지워진 임시 파일을 업로드로 인식했습니다. tearDown 에서 전역을 비우고 서비스를 리셋해 막았습니다.덮지 못하는 것
move_uploaded_file()실제 호출 한 줄입니다. PR #70 의 curl 종단 검증으로 확인했고 앞으로도 수동 영역입니다. 업로드가 전부 테스트된다고 오해하지 않도록 여기에 적어 둡니다.Closes #95
Summary by CodeRabbit