-
Notifications
You must be signed in to change notification settings - Fork 0
[Test/#288] Browser의 테스트 코드를 작성한다. #298
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
YunDaeHyeon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📄 PR Code Review Report
🔍 Summary
제출된 Pull Request는 Browser 클래스에 대한 새로운 단위 테스트와 테스트 헬퍼 Actor들을 포함하고 있습니다. 전체적으로 Swift Testing 프레임워크를 효과적으로 사용하여 비동기 이벤트 스트림을 안정적으로 테스트하려는 노력이 돋보입니다. EventCollector 및 HeartbeatEventCollector Actor의 구현은 비동기 코드 테스트에 대한 좋은 접근 방식을 보여주며, deinit에서 Task를 취소하는 등 구조화된 동시성 패턴을 잘 적용했습니다.
하지만 Xcode 스킴 파일에 잠재적인 휴먼 에러가 있으며, MultipeerConnectivity 델리게이트 메서드의 스레딩 처리 방식과 명령 데이터 직렬화 방식에 대한 실제 구현 시나리오에서 발생할 수 있는 잠재적 위험 요소가 확인되었습니다.
⚠️ Key Issues
-
Xcode 테스트 스킴의 잘못된
LastUpgradeVersion: 새로 추가된 테스트 스킴 파일에LastUpgradeVersion = "2610"으로 설정되어 있습니다. 현재 Xcode 버전은 15.x대이며, 2610은 현실적으로 불가능한 버전입니다. 이는 명백한 오타 또는 플레이스홀더 오류이며, 다른 개발자가 프로젝트를 열었을 때 예기치 않은 동작이나 스킴 마이그레이션 문제를 일으킬 수 있습니다. 올바른 최신 Xcode 버전으로 수정해야 합니다. -
MultipeerConnectivity델리게이트 스레딩 불확실성:MultipeerConnectivity프레임워크의 델리게이트 메서드(예:browser(_:foundPeer:withDiscoveryInfo:),session(_:didReceive:fromPeer:))는 시스템에 의해 임의의 백그라운드 스레드에서 호출될 수 있습니다. 현재 테스트는BrowserTests가@MainActor로 격리되어 있어 테스트 내부에서 델리게이트 메서드를 메인 액터에서 호출하지만, 실제Browser클래스 구현이@MainActor로 격리되어 있지 않거나, 해당 델리게이트 메서드 내부에서 UI 또는 메인 액터 관련 상태를 업데이트할 때 명시적으로 메인 스레드로 디스패치하지 않으면 런타임에 메인 스레드 위반으로 인한 크래시나 예기치 않은 동작이 발생할 수 있습니다.Browser클래스의 실제 구현에서 델리게이트 콜백 처리 시 스레드 안전성 확보 여부를 면밀히 확인해야 합니다. -
명령 데이터 직렬화 방식의 취약성: 특정 테스트(
heartBeat_명령_수신시_HeartBeater가_beat를_호출한다등)에서Data("heartBeat".utf8)와 같이 문자열 리터럴을Data로 변환하여 명령을 주고받는 방식은 잠재적인 취약점을 가집니다. 이는 인코딩 문제, 오타, 또는 향후 명령 구조 변경 시 호환성 문제를 일으키기 쉽고, 타입 안전성이 낮습니다. 더 견고하고 타입 안전한 통신을 위해Codable프로토콜(예:JSONEncoder/JSONDecoder)을 사용하여 명령 객체를 직렬화/역직렬화하는 방식을 고려하는 것이 좋습니다.
🛠 Improvement Suggestions
#expect(true)테스트 구문 개선:세션이_없을때_sendCommand는_실패해도_크래시하지_않는다와 같이 크래시 방지를 테스트하는 경우#expect(true)는 테스트의 의도를 명확하게 보여주지 못할 수 있습니다.Testing프레임워크에 예외 발생 여부를 확인할 수 있는 더 명시적인 API가 있다면 (예:XCTAssertNoThrow와 유사한 기능), 이를 활용하여 테스트의 의도를 더욱 명확히 표현할 수 있습니다.
✅ Positive Observations
- Swift Testing 프레임워크의 효과적인 활용: 최신
Testing프레임워크를 사용하여 비동기 코드와 이벤트 스트림을 테스트하는 방식이 매우 깔끔하고 현대적입니다. - 안정적인 비동기 테스트 헬퍼 구현:
EventCollector및HeartbeatEventCollectorActor는 비동기 이벤트 스트림을 안정적으로 수집하고 타임아웃을 지원하는 훌륭한 테스트 헬퍼입니다.deinit에서Task를 취소하고[weak self]를 사용하여 강한 순환 참조를 방지하는 등 구조화된 동시성 모범 사례를 잘 적용했습니다. - 엣지 케이스 테스트 노력: 특정 조건에서 이벤트가 발생하지 않음을 테스트하거나, 세션이
nil일 때 명령 전송이 크래시하지 않음을 테스트하는 등 엣지 케이스를 고려한 테스트가 돋보입니다. Issue.record활용: 테스트 실패 시Issue.record를 사용하여 더 자세한 디버깅 정보를 제공하는 것은 매우 유용한 접근 방식입니다.
sangYuLv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
어떻게 테스트 하나 싶었는데 이렇게 테스트 헬퍼를 만드는 방법이 있군요...!
수고하셨습니다 👍
| MCNearbyServiceBrowser(peer: testPeerID, serviceType: "mirroringbooth"), | ||
| foundPeer: testPeerID, | ||
| withDiscoveryInfo: discoveryInfo | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
몽이가 몽이를 찾은 상황이네요
| guard case .captureCommand = events.first else { | ||
| Issue.record("Expected .captureCommand event, got: \(String(describing: events.first))") | ||
| return | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기서는 어떤 이벤트가 발생했는지 #expect로 확인 안 하나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
레코드로 단순히 실패만 뜨도록 했는데 지금보니 성공했을 때 명시적으로 출력해주는 요소가 없었네요.. guard문 하단에 #expect(true, "captureCommand 이벤트 수신") 이런식으로 추가해서 성공 케이스도 표시하겠습니다!
감사합니다 🙇
opficdev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
GRJeon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다!
- 명령 수신 (executeCommand)는 현재 private이므로 리팩터링 후 테스트 예정
22aee42 to
c9cd211
Compare
🔗 연관된 이슈
📌 요약
Browser 리팩터링 전 동등성 검증을 위한 테스트 코드를 작성했습니다.
🔍 상세
Browser Test Suite (Swift Testing 사용)
총 11개의 테스트 시나리오로 Browser의 핵심 기능을 검증합니다.
기기 검색
deviceFound이벤트 발생deviceLost이벤트 발생deviceType없는 기기 필터링카메라 스트림
capturePhoto()호출 시captureCommand이벤트 발생명령 전송 안전성
sendCommand크래시 방지sendRemoteCommand크래시 방지sendStreamData크래시 방지명령 수신
heartBeat명령 수신 시 정상 처리Heartbeat
mirroringHeartbeat타임아웃 시 이벤트 발생테스트 헬퍼
EventCollector: Browsing/CameraStream 이벤트 수집HeartbeatEventCollector: Heartbeat 이벤트 수집AsyncStream 이벤트를 배열로 수집하여 검증할 수 있도록 구현했습니다.
👀 리뷰 노트
Browser 분리 작업이 내용이 상당히 커 PR이 커질 것 같아 우선적으로 테스트 코드만 분리해서 PR 먼저 올렸습니다! 👀