-
Notifications
You must be signed in to change notification settings - Fork 0
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
회원 탈퇴 구현 #106
회원 탈퇴 구현 #106
Conversation
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.
리뷰 부탁드립니다! 이전에 단위 테스트 관련해서
- 지금 있는 통합테스트 기반으로 컴포넌트 분리
- 네이밍 일반화
- 인터페이스 추출
- 인터페이스를 구현하는 대역 클래스 생성
- 통합테스트 유지한 상태로 단위테스트 작성
위와 같은 순서로 개선할 것을 제안드렸어요! 그 부분을 인증 로직에 한번 적용해봤습니다
- jwtService의 여러 책임을 컴포넌트로 분리 (Decoder와 WithdrawalHandler)
- jwtService 인터페이스 추출
- jwtService 인터페이스를 구현하는 대역(해당 PR에서는 Mocking을 이용)
- userService delete의 단위 테스트를 위해서 모킹된 jwtService를 사용
더 자세한 단위 테스트, 예를 들어 WithdrawalHandler나 JwtDecoder에 대해서 예외 처리를 잘하는지 검증하기 위해서는 위와 같은 순서로 인터페이스를 추출해, 모킹하거나 직접 대역을 만들어서 사용 가능할 것 같아요!
|
||
@Component | ||
@RequiredArgsConstructor | ||
public class JwtDecoder { |
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.
기존 jwtService 내부 decode에 대한 책임을 분리해서 Decoder를 만들었습니다!
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.
jwtService에서 굳이 Decoder를 수행하는 서비스를 분리한 이유가 궁금합니다! 역할을 분리할 목적이었다면, withDraw 메소드도 분리할 수 있을 것 같은데, 이유가 뭔가용??
@Component | ||
@RequiredArgsConstructor | ||
public class JwtService { | ||
public interface JwtService { |
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.
기존 jwtService의 인터페이스를 추출했습니다!
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.
인터페이스와 이를 구현하는 클래스로 분리해야하는 이유는 뭔가요??
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.
아마도 Mockito를 사용하면 인터페이스 추출안해도 상관없을 것 같은데요.. 제가 생각한 목표는 테스트와 구조 개선을 동시에 잡는 것이 중요하다고 생각했어요!
의존성 관점이나 직접 테스트 대역을 만드는 것(mockito 없이)을 생각했을 때 인터페이스를 추출하는 것이 합리적이라 생각해요! 해당 PR의 개발 단계를 SOLID의 관점에서 말씀드리면 아래처럼 이해할 수 있을 것 같아요!
- jwtService 분리 (SRP)
- 인터페이스를 추출(DIP)
- jwtService를 구현하는 대역 사용(OCP)
|
||
@Component | ||
@RequiredArgsConstructor | ||
public class WithdrawalHandler { |
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.
회원 탈퇴 핸들러를 구현했습니다! 네이밍은 적절한지 다시한번 고민해볼 필요가 있을 것 같아요!
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.
WithDraw보다는 DeleteAccount가 '탈퇴'의 의미에선 직관적이라고 생각합니다!
} | ||
|
||
@Test | ||
public void delete_사용자_삭제시_파이어베이스에_사용자_삭제_요청() throws Exception { |
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.
Mockito를 이용해 외부 API 호출에 대한 spy 대역을 만들기 위해서는 실제 외부 API를 가지고 있는 jwtAuth 클래스를 모킹하는 것이 적절하나, 우선 현재 userService - jwtService간의 상호 작용을 확인하는 것으로 충분하다고 생각했어요!
현재 코드는 jwtService를 모킹해 spy 대역으로 사용합니다. 파라미터를 캡쳐해서 적절히 상호작용하는지 확인합니다!
구현 내용
구현 요약
관련 이슈
#99
구현 내용