[Review] ATDD 1주차 3차 PR

송용주 리뷰어 님께 감사의 말을 전하고 싶다!!

 

1. 질문

@BeforeEach
void init() {
    upStationId = 지하철역_생성("강남역").jsonPath().getLong("id");
    downStationId = 지하철역_생성("건대입구역").jsonPath().getLong("id");
}

@Test
@DisplayName("정상적으로 역을 생성하여 기존 노선 끝에 구간을 추가한다")
public void addSection() {
    // given
    Long lineId = 지하철_노선_생성("2호선", "bg-blue-600", upStationId, downStationId, 10).jsonPath().getLong("id");
    Long stationId = 지하철역_생성("성수역").jsonPath().getLong("id");

    // when
    int statusCode = 지하철_구간_등록(lineId, downStationId, stationId, 1);

    // then
    ExtractableResponse<Response> response = 지하철_단일_노선_조회(lineId);
    List<String> stationList = response.jsonPath().getList("stations.name", String.class);
    assertAll(
            () -> assertThat(statusCode).isEqualTo(HttpStatus.OK.value()),
            () -> assertThat(stationList.get(2)).isEqualTo("성수역")
    );
}

지하철_노선_생성()을 보면 upStationId, downStationId를 매번 인자로 전달받고 있습니다.

지난번 step2 에서는 이를 해결하기 위해 TestFixture를 자신의 내부에 가지고 있는 "2호선_생성()"과 같은 메서드를 만들 수 있었지만, 따라서 중복된 upStationId, downStationId 인자 전달을 막을 수 있었죠!

 

하지만 이번에는 그러한 방식이 힘들다 생각된 이유가 있는데,
이번 테스트 대부분의 when절에서 upStationId, downStationId 둘 중 하나가 지속적으로 필요한 상황이었기 때문입니다.

"2호선_생성()" 메서드에서 upStationId, downStationId를 반환하기에도 힘들 것 같아...
더 좋은 방법을 찾지 못한 상황입니다 ㅎㅎ ....

 

리뷰어님의 의견이 궁금합니다!!

 

답변: 

아 이게... 내가 설명을 조금 잘못한 것 같다... upStationId, downStationId는 클래스 변수로 되어있어 지하철 노선 생성에는 관계가 없는 것이긴 한데...

 

내 의도는 해당 노선 생성 메서드 들을 별도의 Class로 분리하여 모아둘 경우에 대한 이야기였다.

예를 들어 A라는 class의 중복 메서드 들을, B로 이동시켰을 때, B에서 A의 변수를 어떻게 사용할지 였는데....

읔 ㅠ,ㅠ....

 

2. 리뷰 정리

2-1) Boolean 값을 반환하는 메서드 네이밍

리뷰어 말이 적합한 것 같다.

나는 거의? bool값을 반환하는 메서드들을 is로 시작하는 네이밍으로 사용해왔다.

 

하지만 포함을 하고 있는지? 물어보는 메서드는 has라는 접두어로 시작하는 점이 매우 좋은 것 같다!

 

2-2) 이미 있는 메서드 활용하기

size()가 1보다 작아야 한다는 조건에 부합하는지 확인하는 메서드인데, 사실 sections의 isEmpty()를 호출하면 됐을 문제였다.

이미 구현되어 있는 메서드를 잘 활용하는 것 또한 중요하다!

 

2-3) 불면 List로 반환하기

new ArrayList <>()를 통해 반환하게 만들어, 외부에서 넘겨주는 List와 내부적으로 사용하는 인스턴스 변수가 참조하는 값이 다르도록 변경하였다!

 

2-4) 에러 메시지를 한 곳에서 보관하는 이유는?

나의 생각은 다음과 같다.

 

직접 사용할 custom 에러 메시지들이 여러 곳에 퍼져있으면 관리하기 어려울 거라 생각하였습니다.
사실 단순 관리의 편의성이 좋다고 생각하여 한 곳에 모아서 사용 중입니다!!

이에 대한 리뷰어님의 의견이 궁금합니다!
이러한 사용 방법이 좋은 방식일까요?

 

답변:

리뷰어의 의견처럼 Custom Exception마다 별도의 ErrorCode를 만들면 좋을 것 같다!! 다음 과제에서 적극 수용할 예정이다.

 

예를 들어

LineException 에는 해당 전용 LineErrorCode를, StationException 에는 해당 전용 StationErrorCode를 만들어서 사용하면 좋을것 같다.

 

2-5) 예외 메시지 검증하기

음 사실 이 리뷰에 약간은 회의적인 편이다.

현재 ControllerAdvice는 void 형을 반환하는데, 예외 메시지를 알려주기 위해 String을 반환하도록 변경해야 할까?

CQRS를 항상 생각하는 편인데, 상태 검증을 위해서 구간을 등록하는 Command성 메서드가 상태를 반환해야할까?

음...

 

만약 반환해야 한다면,

단순히 예외 메시지를 반환하자고 ResponseEntity <String>으로 바꿔 메시지 본문만 반환하면 될까?

아니면 ErrorResponse와 같은 객체를 만들어 반환해야 할까?

 

답변:

음 의도한 예외 상황인지는 Controller 테스트에서 검증해야 하지 않을까? 물론 인수테스트가 어느정도 Controller 테스트를 대신하기는 한다만...

 

시나리오 기반의 동작을 확인하는 인수 테스트에서 검증해야 할지는 아직도 의문이다?

일단 다음 구현 때는 리뷰어의 의견을 수용하여 예외 메시지를 반환하도록 변경해볼 예정이다.

 

2-6) 인수 테스트의 리팩터링

사실상 가장 어려운 리뷰 었다...

 

일단 내선에서 가능한 리팩터링을 진행하기는 했는데... 부족한 부분이 많았던 것 같다.

특히 리팩터링 도중 다음과 같은 의문이 들었다.

 

답변:

그렇다! 꼭 모든 초기화를 @BeforeEach에 몰아서 코드의 행 수를 줄여야 한다는 강박에 있을 필요는 없는 것 같다.

인수 테스트의 핵심은 시나리오이다!

 

말씀해주신 것처럼 상수나, 변수로 선언만 해두고, 상황에 맞게 Given에서 초기화하면 의미가 더 잘 읽히게 될 것 같다.