[1단계 방탈출 결제 / 배포] 몰리(김지민) 미션 제출합니다. #46

jminkkk · 2024-05-30T02:32:02Z

안녕하세요 조앤 🌊
몰리입니다 😀
마지막 미션 코드 리뷰를 받게 되어 영광입니다 🙌

이전의 웹 개발 경험

스프링을 활용하여 3번 정도의 팀 프로젝트 및 배포를 진행해본 적이 있습니다.
스프링 학습을 기능 구현에 집중하여 사용법을 주로 학습했었어서, 레벨2 때는 전보다 깊게 스프링을 학습하고자 노력하고 있습니다.🔥

참고하실 부분

구현 사항

결제 entity는 2단계 요구사항으로 생각하여 추가하지 않고, 우선 외부 API 연동만 구현했습니다.
외부 API 연동은 처음 진행해보았는데, 외부에 요청을 위임하고 결과를 돌려받는다고 느껴서 Oauth와 비슷하다고 느껴졌던 것 같으면서도 또 약간은 다른 느낌이였던 것 같아요.
따라서 이 부분에 대해 변경에 대해 유연하게 구현된 것이 맞는지, 또는 현재 구현 사항에서 요구사항이 변경되었을 경우 문제점 등에 대해 피드백 받고 싶습니다 🙇

패키지 구조

현재 패키지 구조는 도메인형으로 작성되어 있습니다.
코드 리뷰하실 때 미리 참고하시면 더 수월하실 것 같아요 😁

DTO 네이밍

패키지 구조와 관련해 도메인형 구조에서는, 행위가 먼저 나오는 것이 빠르게 인식할 수 있을 것이라고 생각하여 아래와 같은 컨벤션을 따라 작성하였습니다.

<행위> <자원> <Response/Request>

추가 질문 사항은 코멘트를 통해 작성하겠습니다.
리뷰 잘 부탁드립니다 🙂

Co-authored-by: tkdgur0906 <[email protected]>

jminkkk · 2024-05-30T04:06:16Z

src/main/resources/application.properties

+payment.secretKey=test_gsk_docs_OaPz8L5KdmQXkzRz3y47BMw6
+


현재는 테스트용 secretKey를 사용하지만, 추후 실제 환경을 운영하게 되어 발급을 받아 사용하게 된다면, 민감한 정보라고 생각되어 properties로 관리하고자 했습니다..!

jminkkk · 2024-05-30T04:07:34Z

src/test/java/roomescape/payment/FakePaymentClient.java

+public class FakePaymentClient implements PaymentClient {
+
+    @Override
+    public Payment confirm(final ConfirmPaymentRequest confirmPaymentRequest) {
+        return new Payment(confirmPaymentRequest.paymentKey(), confirmPaymentRequest.orderId(),
+                confirmPaymentRequest.amount());
+    }
+}


RestClient와 테스트

현재 모든 통합 테스트와 서비스단 테스트를 목을 사용하지 않고 실제 구현으로 사용하고 있습니다.
따라서 결제 관련 구현도 목을 사용하지 않고 Fake객체를 구현하여 주입하는 것이 자연스러울 것 같아 이처럼 구현했습니다.

Fake 객체를 구현하여 주입하는 것이 자연스러울 것 같은 이유는 뭐예요?

제 설명이 조금 불친절했네요 🥲

일단 저는 레벨1 까지만 하더라도 Mock 사용을 선호했었는데요. 요구사항이 추가될수록 Mock 때문에 테스트 관리가 더 불편하다고 느꼈던 것 같아요.
사용하는 메서드를 하나하나 stub을 해줘야 하는데 요구사항 변경이 일어났을 때, stub을 까먹고 안한다든가 또는 거짓 음성에 취약하다는 점도 영향이 컸어요 🤔
따라서 제가 작성한 모든 통합, 서비스 계층 테스트에서 가능하면 최대한 실제 구현 또는 Fake 객체를 사용하고자 했던 것 같아요.

결제 승인을 추가하면서, 요런 주관(?)과 함께 관련 테스트들에서도 외부 API 부분을 어떻게 제어하여 테스트를 할 수 있을까를 고민했었는데요.
실제 구현을 사용하여 테스트 하기에는 외부 API는 개발자가 제어할 수 없는 부분이라 적절하지 않고, 남은 Mock과 Fake 에 대한 선택 중에는 다른 부분들도 실제 구현처럼 동작하도록 했으니 Mock이 아닌 Fake 객체를 사용하는 것이 더 일관된 처리라고 생각했던 것 같아요.

조앤에게 얘기하고 싶었던 부분도 "이 이유에서 Fake를 사용했다" 였던 것 같은데 여러모로 생략된 부분이 많았네요 😁

Mock과 Fake의 장단점이 존재하는 것 같아요. Fake는 어쨌든 직접 구현하여 쓰는 거라 관리가 필요하게 되는 반면, Mock은 테스트에서만 임시로 생성할 수 있다고 생각해요. Mock에서 행위 지정도 가능하고요ㅎㅎ (말씀해주신 장/단점도 있을 거고용)
Spy라는 것도 있는데 나중에 시간되실 때 학습해보셔도 좋을 것 같아요~

jminkkk · 2024-05-30T04:19:43Z

src/test/java/roomescape/reservation/ReservationIntegrationTest.java

@@ -40,22 +43,30 @@
 import roomescape.waiting.repository.WaitingRepository;

 @IntegrationTest
+@Import(FakePaymentClient.class)


실제 외부 API 테스트

현재 TossPaymentClient에 대한 테스트가 작성되어 있지 않습니다.

RestClient 사용 시 MockRestServiceServer를 통해서 테스트를 할 수 있다고 알고 있는데, 그렇게 되면 실제 외부 API 요청을 보내지 않고 stub처럼 처리하기 때문에 실제 연결 테스트가 아닌 것으로 알고 있습니다...!

그렇다면 이렇게 실제 연결을 하지 않았을 때, 외부 API에 대해 어떤 테스트가 필요할지에 대한 고민을 했었습니다. 그러나 현재 단계에서 제가 어떤 것을 테스트 하고 싶은지를 느끼지 못해서 따로 작성하지 못했습니다.

결과적으로 기존의 Reservation에 대한 통합, 서비스단 테스트가 정상적으로 동작할 수 있도록만 관련 코드를 변경하였습니다..!
혹시 추가하거나 고민해보았으면 하는 테스트 상황이 있을지 조앤의 의견을 듣고 싶습니다 🙌

payment에서 실패가 발생했을 때 예약 생성도 실패하는 것에 대한 테스트도 있으면 좋을 것 같아요!

꼭 필요한 테스트네요 🙌
추가하겠습니다!

seovalue

안녕하세요, 몰리! 반가워요~
미션 깔끔하게 잘 수행해주셨네요ㅎㅎ
소소한 코멘트 몇가지 남겼는데 확인 후 리뷰 요청 주세요!

seovalue · 2024-05-30T12:47:38Z

docs/README.md

-
-## 2단계
- [x] 내 예약 조회 API 구현
+# 요구사항 분석


seovalue · 2024-05-30T12:48:09Z

src/main/java/roomescape/common/ClientConfiguration.java

+import roomescape.payment.client.PaymentProperties;
+
+@Configuration
+public class ClientConfiguration {


소소한 피드백ㅎㅎ
여기도 RestClient를 명시하는건 어때요? RestClientConfiguration

앗 더 명확해질 것 같아요!
반영하겠습니다 🙌

seovalue · 2024-05-30T12:49:00Z

src/main/java/roomescape/common/ClientConfiguration.java

+        String authorizations = BASIC_PREFIX + new String(encodedBytes);
+
+        return RestClient.builder()
+                .baseUrl("https://api.tosspayments.com/v1/payments")


time-out도 한번 설정해볼까요?
외부 api 호출 시 타임아웃은 왜 중요할까요?

타임아웃 설정을 놓쳤네요 😅

제가 알기로는, 외부 Client 와 연결이 되지 않는다거나(connection timeout), 연결은 됐지만 응답이 오는 시간이 너무 오래 걸려(read timeout) 사용자가 무한정 기다리게 되는 상황을 막고자 timeout 설정이 필요한 것으로 알고 있어요 !

seovalue · 2024-05-30T12:49:20Z

src/main/java/roomescape/common/ClientConfiguration.java

+    }
+
+    @Bean
+    public RestClient restClient() {


RestClient를 선택해주신 이유가 궁금해요~

일단 외부 client를 고려할 때, 빠른 미션 적용을 위해 학습 테스트에서 제시된 RestTemplate, RestClient 둘 중에 하나를 고민을 했었는데요 !
RestClient가 체이닝으로 더 가독성이 좋고 깔끔하게 코드를 작성할 수 있는 것 같아서 선택했습니다 !

seovalue · 2024-05-30T12:49:36Z

src/main/java/roomescape/common/GlobalExceptionHandler.java

+    }
+
+    @ExceptionHandler
+    public ResponseEntity<ProblemDetail> catchHttpClientErrorException(HttpServerErrorException ex) {


Suggested change

public ResponseEntity<ProblemDetail> catchHttpClientErrorException(HttpServerErrorException ex) {

public ResponseEntity<ProblemDetail> catchHttpServerErrorException(HttpServerErrorException ex) {

앗 미쳐 확인하지 못했네요 😅
꼼꼼히 봐주셔서 감사합니다 🙌 🙇🏻‍♀️

seovalue · 2024-05-30T12:50:12Z

src/main/java/roomescape/payment/client/TossPaymentClient.java

+import roomescape.payment.model.Payment;
+
+@Component
+public class TossPaymentClient implements PaymentClient {


seovalue · 2024-05-30T12:52:56Z

src/main/java/roomescape/payment/service/PaymentService.java

+@Service
+public class PaymentService {
+
+    private final PaymentClient paymentClient;


다른 서비스에서 Client를 직접 호출하는 게 아니라 Service로 한번 더 감싸주셨군요?

현재는 "결제 승인"이라는 기능에 대해 단순 외부 client 연동만 하면 된다는 미션 요구사항이 제공되었지만,
외부 client에 요청을 한다는 것 또한 결제라는 기능을 위한 비즈니스 요구사항이라고 생각하여 Service 계층을 만들어 해당 역할을 하는 객체를 호출했습니다 !
또 결제에 대한 추가적인 요구사항이 변경될 경우에도 해당 계층 덕분에 더 유연할 것이라고도 생각했습니다 😁

seovalue · 2024-05-30T12:55:50Z

src/main/java/roomescape/common/PaymentClientErrorResponse.java

@@ -0,0 +1,5 @@
+package roomescape.common;
+
+public record PaymentClientErrorResponse(String code,


webclient에서 던지는 HttpClientErrorException을 PaymentClientErrorResponse화 하는 것에 대해서,
나중엔 payment 외에 다른 client를 연동하게 될 수도 있지 않을까요?

동의합니다 !
여러 Client를 사용하게 된다면 HttpClientErrorException가 TossPayment에만 발생한다는 보장이 없네요!
따라서 해당 에러 객체의 규격이 PaymentClientErrorResponse과 동일하다고 할 수도 없구요!

모든 HttpClientErrorException를 PaymentClientErrorResponse화 하지 않고 TossClient의 예외 상황에 한해 감싸서 터트리는 것이 맞을 것 같아요 수정하겠습니다 🙇

제가 다시 읽어봤을 때 좀 모호하게 질문드렸던 것 같은데 찰떡같이 알아들으셨네요ㅎㅎ 💯

seovalue · 2024-05-30T12:56:28Z

src/test/java/roomescape/payment/FakePaymentClient.java

+public class FakePaymentClient implements PaymentClient {
+
+    @Override
+    public Payment confirm(final ConfirmPaymentRequest confirmPaymentRequest) {
+        return new Payment(confirmPaymentRequest.paymentKey(), confirmPaymentRequest.orderId(),
+                confirmPaymentRequest.amount());
+    }
+}


Fake 객체를 구현하여 주입하는 것이 자연스러울 것 같은 이유는 뭐예요?

seovalue · 2024-05-30T12:57:44Z

src/test/java/roomescape/reservation/ReservationIntegrationTest.java

@@ -40,22 +43,30 @@
 import roomescape.waiting.repository.WaitingRepository;

 @IntegrationTest
+@Import(FakePaymentClient.class)


payment에서 실패가 발생했을 때 예약 생성도 실패하는 것에 대한 테스트도 있으면 좋을 것 같아요!

… 명을 올바르게 변경

…디폴트 메시지 전달하는지 테스트 작성

… 예약 생성 실패 테스트 추가

jminkkk

안녕하세요 조앤!
지난 리뷰 감사합니다! 덕분에 놓친 부분들에 대해서 고민해볼 수 있었어요 🍀
리뷰 반영을 하면서 궁금했던 질문들이나 고민했던 부분들을 추가 코멘트를 달았어요 (마지막 미션과 리뷰라고 생각되니, 너무 아쉽고 소중해요 😭)

이번 리뷰 반영까지의 커밋 내역은 요 링크를 통해 확인해주시면 될 것 같아요!
이번 리뷰도 잘 부탁드립니다! 🙌🙇🏻‍♀️

+) toss 등 패키지 구조를 조금 변경했는데, 관련 파일 추적 때문에 리뷰 하시기 힘들어지실 수도 있을 것 같아서 미리 죄송해요 🥲

jminkkk · 2024-05-31T01:43:02Z

src/main/java/roomescape/common/ClientConfiguration.java

+import roomescape.payment.client.PaymentProperties;
+
+@Configuration
+public class ClientConfiguration {


앗 더 명확해질 것 같아요!
반영하겠습니다 🙌

jminkkk · 2024-05-31T08:37:09Z

src/main/java/roomescape/common/GlobalExceptionHandler.java

+    }
+
+    @ExceptionHandler
+    public ResponseEntity<ProblemDetail> catchHttpClientErrorException(HttpServerErrorException ex) {


앗 미쳐 확인하지 못했네요 😅
꼼꼼히 봐주셔서 감사합니다 🙌 🙇🏻‍♀️

jminkkk · 2024-06-01T06:41:40Z

src/main/java/roomescape/common/PaymentClientErrorResponse.java

@@ -0,0 +1,5 @@
+package roomescape.common;
+
+public record PaymentClientErrorResponse(String code,


동의합니다 !
여러 Client를 사용하게 된다면 HttpClientErrorException가 TossPayment에만 발생한다는 보장이 없네요!
따라서 해당 에러 객체의 규격이 PaymentClientErrorResponse과 동일하다고 할 수도 없구요!

모든 HttpClientErrorException를 PaymentClientErrorResponse화 하지 않고 TossClient의 예외 상황에 한해 감싸서 터트리는 것이 맞을 것 같아요 수정하겠습니다 🙇

jminkkk · 2024-06-01T06:43:44Z

src/main/java/roomescape/payment/client/PaymentClient.java

+import roomescape.payment.dto.request.ConfirmPaymentRequest;
+import roomescape.payment.model.Payment;
+
+public interface PaymentClient {


꼭 toss 말고 다른 결제 client를 사용하게 될 수도 있어서 추상화가 필요한 부분이라고 생각했어요 ㅎㅎ

jminkkk · 2024-06-01T07:50:29Z

src/main/java/roomescape/payment/client/PaymentProperties.java

+
+import org.springframework.boot.context.properties.ConfigurationProperties;
+
+@ConfigurationProperties(value = "payment")


Spring boot가 자동으로 외부 설정 파일(properties 또는 yml)의 속성값을 인식하여 클래스의 필드값으로 바인딩해주는 어노테이션입니다 !

기존에 사용했던 @value보다 쉽게 값들을 주입받을 수 있고 클래스로 더 편하게 관리할 수 있다고 생각하여 사용했습니다 😁

jminkkk · 2024-06-01T12:58:48Z

src/main/java/roomescape/common/ClientConfiguration.java

+        String authorizations = BASIC_PREFIX + new String(encodedBytes);
+
+        return RestClient.builder()
+                .baseUrl("https://api.tosspayments.com/v1/payments")


타임아웃 설정을 놓쳤네요 😅

제가 알기로는, 외부 Client 와 연결이 되지 않는다거나(connection timeout), 연결은 됐지만 응답이 오는 시간이 너무 오래 걸려(read timeout) 사용자가 무한정 기다리게 되는 상황을 막고자 timeout 설정이 필요한 것으로 알고 있어요 !

jminkkk · 2024-06-01T13:23:22Z

src/main/java/roomescape/payment/client/PaymentRestClientConfiguration.java

+        simpleClientHttpRequestFactory.setConnectTimeout(3);
+        simpleClientHttpRequestFactory.setReadTimeout(30);


타임 아웃 설정 시간 기준

토스페이먼츠 API의 Read Timeout은 30초에서 60초로 설정하면 돼요. 타임아웃(Timeout) | 토스페이먼츠 개발자센터

ReadTimeout

해당 타임타웃은 토스 문서에 제공된 권장 시간인 30 ~ 60초 중 가장 빠른 시간으로 설정했습니다 !

ConnectTimeout

해당 타임아웃은 사실 어느 정도가 적절한 수준인지 감이 안왔었는데요. 수업 시간 내용을 토대로 3초로 설정했습니다 😅

연결에서 3초 이상이 걸린다는 것은 비정상적인 상황이라고 하더라구요 ! 때문에 차라리 빠른 예외를 터뜨려서 사용자가 재시도 등을 시도할 수 있게 하는 것이 적절하다고 생각하였습니다.

굿굿! 좋네요 💯

jminkkk · 2024-06-01T13:49:10Z

src/main/java/roomescape/payment/client/toss/TossErrorCodeNotForUser.java

+    INVALID_API_KEY,
+    NOT_FOUND_TERMINAL_ID,
+    INVALID_AUTHORIZE_AUTH,
+    INVALID_UNREGISTERED_SUBMALL,
+    NOT_REGISTERED_BUSINESS,
+    UNAPPROVED_ORDER_ID,
+    UNAUTHORIZED_KEY,
+    FORBIDDEN_REQUEST,
+    INCORRECT_BASIC_AUTH_FORMAT,
+    ;


조앤 이 부분도 수업 시간을 통해 다시 생각해보게 되면서 추가한 부분인데요 !

1차 리뷰 요청 당시에는 토스 측의 에러 메시지를 전부 그대로 매핑하여 에러를 터뜨렸었어요.

"토스 측의 모든 에러 메시지가 사용자 또는 클라이언트가 알게 할 필요가 있을까?"

그런데 위 질문을 통해 고민해본 결과 토스 client 요청 중 발생할 수 있는 에러 중에서 사용자에게 알릴 코드와 아닌 코드를 분리해서 처리하는 것이 적절하다고 생각했어요.
따라서 토스 문서 (에러 코드 - 결제 승인)에서, 제가 판단하기에 클라이언트나 사용자 측에서 조치를 할 수 없는 에러인 경우라면, 해당 메시지를 전달하는 것이 의미가 없다고 생각하여 다른 메시지로 바꾸어 전달하고자 했어요.
그러기 위해서 변경이 필요한 토스 에러 code를 enum으로 관리하도록 했는데요.

enum으로 변경해야 할 외부의 에러 코드를 관리하는 것에 대해 조앤은 어떻게 생각하시는지 궁금합니다 🤔 ~~(뭔가 설명할 수 없는 찝찝함이 있어요 😭)~~

또, 실무에서는 보통 이런 에러 메시지를 변경할 것인가에 대한 문제 또한 기획자와 상의하나요? 아니면 개발자의 역량일까요?

개발자의 역량이라면, 앞서 제가 말했듯 "제가 생각하기에 클라이언트나 사용자 측에서 조치를 할 수 없는 에러라면, 해당 메시지를 전달하는 것이 의미가 없다"라는 변경 기준이 적절한 기준이 맞을까요?

예를 들어 토스쪽에서 해당 에러코드를 breaking change 내는 경우, 사용하는 쪽에서 1:1로 매핑하고 있다면 일일이 다 바꿔줘야하는 불편함이 있을 것 같아요. 또한 그걸 모르고 계속 유지보수한다면 예상치 못한 상황이 발생할 수도 있고요ㅎㅎ
근데 지금은 결제 요구사항이기도 하고 디펜던시가 굉장히 큰 상황에서 에러 코드를 직접 관리해도 괜찮을 것 같다는 생각이 들어요. 대신 메시지는 커스텀해서 내릴 수 있을 것 같아요.

다 다를 것 같긴 한데 제 경험으로는 요구사항을 바탕으로 발생가능한 예외 상황에 대해 다같이 논의해요! 저는 좀 스타트업쪽이라 그런데 큰회사는 요구사항이 찍혀내려오는 경우도 있다고 들었어요.

저도 좀 비슷하게 생각하긴 해요. 에러라는게 사실 피드백인데, 다음에 어떤 행위를 해야 개선할 수 있는지 가이드가 되지 않는다면 의미 없다고 생각하긴 해요. 하지만 사용성의 측면에서 유저에게 지금이 어떤 상황인지 정도를 알리는 용도로서 적절한 메시지로 변환해서 내릴 순 있을 것 같아요.

jminkkk · 2024-06-01T14:41:03Z

src/test/java/roomescape/reservation/service/ReservationServiceTest.java

+        Member member = memberRepository.save(MemberFixture.getOne());
+
+        CreateMyReservationRequest createMyReservationRequest = new CreateMyReservationRequest(
+                LocalDate.of(2024, 10, 10), reservationTime.getId(), theme.getId(), "failPayment", "orderId", 100_000L);


조앤, "결제 오류 시 예약은 저장되지 않는다" 테스트를 추가하면서 이 부분에서도 고민이 큰데요 😭
현재 reservationService는 PaymentClient를 Stub하지 않고 Fake로 사용하고 있어요

"결제 오류 시 예약은 저장되지 않는다"를 테스트하기 위해서는 우선 결제 오류가 선행되어야 하는데, 이때 PaymentClient 부분 또는 PaymentService에서 에러를 발생하도록 해야 한다고 생각했어요. 그 에러를 어떻게 발생시킬지는 다시 Fake와 Mock 중에 고려했어요

Mock을 사용하게 되면 메서드 내에서 바로 payment 에러가 발생한다는 것을 알 수 있지만, 이 테스트가 아닌 다른 예약 생성 테스트들에서도 stub을 해줘야 하는 등 영향을 끼치기 때문에 적절하지 못하다고 생각했어요. (기존 Mock에 대한 제 생각도 있구요)

그렇다면 Fake에서 어떻게 에러를 터뜨릴 수 있을지 고민하다가 paymentKey 값으로 "failPayment"가 오면 FakePaymentClient에서 무조건 에러를 터뜨리도록 변경하였어요.
흠 그런데 Mock보다는 낫지만 CreateMyReservationRequest의 paymentKey 값이 미치는 영향에 대해, Fake 구현체를 확인하지 못하면 전혀 파악할 수 없어서 즉, 하나의 테스트에서 한눈에 모든 상황을 파악할 수 없어서 우아하지 못하다고 생각해요.

이런 경우에는 보통 조앤은 어떻게 처리하시나요? 현재보다 더 우아한 방법이 있을까요? 😭

저는 보통 Fake 대신 Mock을 사용하긴 해요!
그 이유는 Fake는 말씀주신 것처럼 구현체에 따라서 동작이 달라질 수 있어서, 예상치 못한 예외가 발생할 수도 있고 호출부에서 구현을 알아야하기도 해서요ㅎㅎ

만약 Fake를 유지해야한다면, Fake와 Stubbing을 결합해서 특정 boolean 값에 따라 fail하는 FakePaymentClient를 사용하는 등으로 할 수도 있을 것 같아요. (일괄 실패하는 Client) 대신 지금 구조에선 Fake..Client를 Bean으로 등록해서 사용하고 있어서 빈 대신 직접 생성해서 호출하는 형태로 가야할 것 같아요.

public class FakePaymentClient implements PaymentClient { private boolean shouldFail; public void setShouldFail(boolean shouldFail) { this.shouldFail = shouldFail; } @Override public PaymentResponse processPayment(PaymentRequest request) { if (shouldFail) { throw new PaymentException("Payment failed"); } // 기존 결제 로직 } }

jminkkk · 2024-06-01T14:57:30Z

src/test/java/roomescape/payment/client/toss/TossRestClientTest.java

+import roomescape.util.IntegrationTest;
+
+@IntegrationTest
+class TossRestClientTest {


실제 외부 API 요청 테스트 2

그렇다면 이렇게 실제 연결을 하지 않았을 때, 외부 API에 대해 어떤 테스트가 필요할지에 대한 고민을 했었습니다. 그러나 현재 단계에서 제가 어떤 것을 테스트 하고 싶은지를 느끼지 못해서 따로 작성하지 못했습니다.

1차 리뷰 요청 당시에 위와 같은 말씀을 드렸는데요.
당시와 약간 다른 포인트지만, 앞에서 토스 측의 특정 에러인 경우 에러 메시지를 변경하여 터뜨리는 부분이 있었어요.
이에 대해 잘 변경이 되는지 테스트가 필요할 것 같아서 추가했습니다 !

"테스트에서 실제 요청을 보내도 되는가"라는 고민이 있었는데요 🤔
기존에 실제 요청을 고려하지 않았던 테스트는 연결에서 문제가 발생할 때 미치는 영향이 크다고 생각했기 때문이었어요.

에러 발생 시 이미 여러 의존성이 걸쳐 있어서 연결 때문에 발생한 에러인지 파악하기 어려울 것 같고

또한 실제 요청으로 인해 거짓 양성을 일으킬 가능성이 있다

하지만 이 파일 경우에는 전체가 직접 요청만을 하고 있기 때문에 위에서 고려했던 문제들에 대해서 괜찮을 것이라고 생각하여 일단 실제로 요청을 보내도록 작성했습니다...!

문제되는 점이 있거나 아쉬운 부분이 있다면 가감없이 피드백 주시면 감사하겠습니다 🙇

테스트에서 실제 요청을 보내도 되는가와, 지금 테스트하는 내용에 대해 실제 요청을 하는 것 두가지로 나눠서 생각해볼게요.

첫번째에 대해서
저는 개인적으로 테스트는 발생 가능한 문제에 대해 예상한 대로 동작하는 지 검증하는 것이라고 생각하는데요.
그 과정에서 컨트롤 가능한 도 하나의 조건이 되는 것 같아요.

즉, 지금과 같은 상황에서 Toss의 응답은 자체적으로 관리할 수 있는 부분이 아니기 때문에 테스트에서 직접 호출 하는 것이 큰 의미가 있다고 생각하진 않아요.

두번째는 지금 구현 자체가 Toss의 응답에 의존되어있어요. (위에서 언급했던 Enum) 그래서 그 enum에 해당하는 값이 포함된 경우 적절한 예외로 내리느냐를 테스트해야해요.
이 경우도, 마찬가지로 Toss에서 이러한 응답을 준다고 가정했을 때, 내가 매핑할 Enum에 적절히 매핑되는지까지가 테스트 범위이라고 생각해요. 그래서 그 이전 단계인 Toss에서 이러한 응답을 준다 부분은 꼭 직접 호출할 필요는 없다고 생각해요.

seovalue · 2024-06-01T23:01:44Z

src/test/java/roomescape/payment/client/toss/TossRestClientTest.java

+    }
+
+    @Test
+    @DisplayName("토스 결제 승인 실패: 유저에게 알리지 않을 사유라면 디폴트 메시지 전달")


이 테스트 실패해요~

seovalue · 2024-06-01T23:03:22Z

src/main/java/roomescape/common/PaymentClientErrorResponse.java

@@ -0,0 +1,5 @@
+package roomescape.common;
+
+public record PaymentClientErrorResponse(String code,


제가 다시 읽어봤을 때 좀 모호하게 질문드렸던 것 같은데 찰떡같이 알아들으셨네요ㅎㅎ 💯

seovalue

안녕하세요, 몰리! 리팩터링 잘 수행해주셨네요ㅎㅎ
기존에 남겨주셨던 질문이나 의견이 좋아서 거기에 대한 답변 위주로 남겨봤어요.
수정해주신 부분에 대해서는 크게 코멘트할 부분은 없는 것 같아요~
답변은 다음 단계 진행하시기 전에 한번 확인해주세요!
그리고 제출 전 테스트도 한번 실행해주세요~

seovalue · 2024-06-02T00:03:36Z

src/test/java/roomescape/payment/client/toss/TossRestClientTest.java

+import roomescape.util.IntegrationTest;
+
+@IntegrationTest
+class TossRestClientTest {


테스트에서 실제 요청을 보내도 되는가와, 지금 테스트하는 내용에 대해 실제 요청을 하는 것 두가지로 나눠서 생각해볼게요.

첫번째에 대해서
저는 개인적으로 테스트는 발생 가능한 문제에 대해 예상한 대로 동작하는 지 검증하는 것이라고 생각하는데요.
그 과정에서 컨트롤 가능한 도 하나의 조건이 되는 것 같아요.

즉, 지금과 같은 상황에서 Toss의 응답은 자체적으로 관리할 수 있는 부분이 아니기 때문에 테스트에서 직접 호출 하는 것이 큰 의미가 있다고 생각하진 않아요.

두번째는 지금 구현 자체가 Toss의 응답에 의존되어있어요. (위에서 언급했던 Enum) 그래서 그 enum에 해당하는 값이 포함된 경우 적절한 예외로 내리느냐를 테스트해야해요.
이 경우도, 마찬가지로 Toss에서 이러한 응답을 준다고 가정했을 때, 내가 매핑할 Enum에 적절히 매핑되는지까지가 테스트 범위이라고 생각해요. 그래서 그 이전 단계인 Toss에서 이러한 응답을 준다 부분은 꼭 직접 호출할 필요는 없다고 생각해요.

jminkkk and others added 15 commits May 29, 2024 21:13

chore(/src): 기존 코드 마이그레이션

37f366d

docs(data.sql): 요구사항 분석 작성

60459e1

Co-authored-by: tkdgur0906 <[email protected]>

chore(/resources): 프론트 코드 추가 및 수정

02cfe1d

Co-authored-by: tkdgur0906 <[email protected]>

feat(PaymentProperties): 외부 API 결제 키 추가

cf2aaeb

Co-authored-by: tkdgur0906 <[email protected]>

feat(ClientConfiguration): 외부 결제 Client 설정 추가

f14d05e

Co-authored-by: tkdgur0906 <[email protected]>

feat(PaymentClient): 외부 API에 결제 요청을 보내는 PaymentClient 생성

6526ae9

Co-authored-by: tkdgur0906 <[email protected]>

feat(ReservationService, PaymentService): 예약 로직에서 결제 승인하는 기능 추가

89e452c

Co-authored-by: tkdgur0906 <[email protected]>

feat(PaymentClient): 결제 API 호출 시 예외처리

3750a51

Co-authored-by: tkdgur0906 <[email protected]>

refactor(PaymentClient): 결제 관련 외부 Client 객체 추상화

40b81f8

Co-authored-by: tkdgur0906 <[email protected]>

refactor(ReservationServiceTest): 예약 시 FakePaymentClient 호출하도록 수정

70cb8a0

Co-authored-by: tkdgur0906 <[email protected]>

refactor(ReservationIntegrationTest): 안쓰는 import 제거

4664d31

Co-authored-by: tkdgur0906 <[email protected]>

refactor(GlobalExceptionHandler): 500 대 HttpServerErrorException 에러 처리

c5f4a34

Co-authored-by: tkdgur0906 <[email protected]>

refactor(ClientConfiguration): Authorization 헤더 상수로 변경

8d4e573

Co-authored-by: tkdgur0906 <[email protected]>

refactor(ClientConfiguration): 인증 헤더의 시크릿 키에서 상수 추출

0cb75e4

Co-authored-by: tkdgur0906 <[email protected]>

refactor(ReservationService): admin, 일반 유저 예약 생성 중복 로직 제거

43d3457

jminkkk commented May 30, 2024

View reviewed changes

seovalue requested changes May 30, 2024

View reviewed changes

jminkkk added 11 commits May 31, 2024 10:44

refactor(RestClientConfiguration): ClientConfiguration를 더 명확하게 이름 변경

cf8f2e9

refactor(GlobalExceptionHandler): HttpServerErrorException를 핸들링하는 메서드…

fc576c4

… 명을 올바르게 변경

refactor(TossClientErrorResponse): Toss에서 제공하는 에러 객체 이름 변경

846f338

refactor(PaymentProperties): 결제 요청 시 필요한 비밀번호 값을 properties로 위치 이동

f7c1236

refactor(/payment, /toss): 결제 client 관련 파일들의 위치 수정

e0f50e2

refactor(TossPaymentErrorHandler): Toss 결제 처리 중 발생한 에러를 핸들링하는 객체 구현 및 등록

06bbf14

refactor(TossErrorCodeNotForUser): 유저에게 보여주지 않을 토스 에러 코드 이넘으로 관리

e8ca752

test(TossRestClientTest): 토스 결제 승인 실패 시, 토스 에러 코드가 사용자에게 알리지 않을 사유라면 …

72a0361

…디폴트 메시지 전달하는지 테스트 작성

test(FakePaymentClient): 결제 승인 요청 시 무조건 예외를 발생하는 keyword 추가 및 결제 오류 시…

09a27a8

… 예약 생성 실패 테스트 추가

test(PaymentRestClientConfiguration): 타임아웃 설정

a8095d6

test(GlobalExceptionHandler): SocketTimeoutException에 대한 핸들링 추가

aab08d3

jminkkk commented Jun 1, 2024

View reviewed changes

seovalue reviewed Jun 1, 2024

View reviewed changes

seovalue approved these changes Jun 2, 2024

View reviewed changes

seovalue merged commit f0cac3e into woowacourse:jminkkk Jun 2, 2024

	public ResponseEntity<ProblemDetail> catchHttpClientErrorException(HttpServerErrorException ex) {
	public ResponseEntity<ProblemDetail> catchHttpServerErrorException(HttpServerErrorException ex) {

		@@ -0,0 +1,5 @@
		package roomescape.common;

		public record PaymentClientErrorResponse(String code,


		import org.springframework.boot.context.properties.ConfigurationProperties;

		@ConfigurationProperties(value = "payment")

		simpleClientHttpRequestFactory.setConnectTimeout(3);
		simpleClientHttpRequestFactory.setReadTimeout(30);

		payment.secretKey=test_gsk_docs_OaPz8L5KdmQXkzRz3y47BMw6

[1단계 방탈출 결제 / 배포] 몰리(김지민) 미션 제출합니다. #46

[1단계 방탈출 결제 / 배포] 몰리(김지민) 미션 제출합니다. #46

Conversation

jminkkk commented May 30, 2024 • edited Loading

이전의 웹 개발 경험

참고하실 부분

구현 사항

패키지 구조

DTO 네이밍

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RestClient와 테스트

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

실제 외부 API 테스트

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seovalue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jminkkk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

타임 아웃 설정 시간 기준

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

실제 외부 API 요청 테스트 2

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seovalue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jminkkk commented May 30, 2024 •

edited

Loading