Step2. 지하철 구간 추가 리팩토링

[nara1030]

안녕하세요
두 번째 리뷰 요청 드립니다.
늦은 마음에 급급하여 따라 치는데 그친 거 같지만, 질문이 몇 가지 있어 아래 남기도록 하겠습니다.
감사합니다.

---

1. 인수 테스트 이후 기능 구현은 TDD로 진행하세요.

요구사항에 다음과 같이 적혀있는데, 제가 강의를 이해를 잘못해서 그런지 내부가 구현이 안 되어 있는데 인수 테스트가 가능한가 싶더라구요. 인수 조건, 전체적인 플로우만 정의해 놓은 채로 기능 구현을 하란 의미인지요?

2. 상행 구간 추가 이슈

다음 링크에서 addSectionToStartOfLine메소드는 새로운 역을 상행 종점으로 등록하는 테스트입니다.

이를 위해 다음 링크의 addSection 메소드를 보시면 다음 로직을 추가하였습니다.

if (checkIfPositionToBeAddedIsFirst(section)) {
    sections.add(section);
    return;
}

헌데 계속 다음과 같은 에러가 발생하는데요.

java.lang.AssertionError: 
Expecting actual:
  [1L, 2L, 1L]
to contain exactly (and in same order):
  [3L, 1L, 2L]
but some elements were not found:
  [3L]
and others were not expected:
  [1L]

이삼일을 이 에러를 보고 있었는데 잘 풀리지가 않아 질문드립니다..

3. 기존 코드의 구간 추가 메소드 add 내부에 rearrangeSectionWithUpStation와 rearrangeSectionWithDownStation 메소드가 있던데요. 제가 이해를 잘못해서인건지, 두 개 중 하나만 있으면 되지 않나 싶더라구요. 어차피 같은 기능을 수행하는게 아닌지...

4. 구간 추가 실패 케이스와 관련하여 Line의 addSection 메소드를 테스트하는 테스트 메소드 failToAddSectionIfAllStationsExist를 다음과 같이 작성하였습니다.

// 테스트 코드
assertThatThrownBy(() -> 지하철_노선에_지하철_구간_생성_요청(신분당선, createSectionCreateParams(강남역, 양재역)))
	.isInstanceOf(IllegalArgumentException.class);

// 도메인 코드
public void addSection(Section section) {
	if (sections.isEmpty()) {
		sections.add(section);
		return;
	}
	checkDuplicateSection(section);
	sections.add(section);
}

private void checkDuplicateSection(Section section) {
	sections.stream()
			.filter(it -> it.hasDuplicateSection(section.getUpStation(), section.getDownStation()))
			.findFirst()
			.ifPresent(it -> {
				throw new IllegalArgumentException();
			});
}

하지만 분명히 addSection 메소드에서 예외를 던져줌에도 테스트가 실패(예외가 발생함이 expected되나 null, 즉 발생하지 않았다는 에러 메세지..)하여 다음과 같이 수정하여 테스트하였습니다.

// when
ExtractableResponse<Response> response = 지하철_노선에_지하철_구간_생성_요청(신분당선, createSectionCreateParams(강남역, 양재역));

// then
assertThat(response.statusCode()).isEqualTo(HttpStatus.BAD_REQUEST.value());

애초에 Line 도메인 단위 테스트를 위해 작성했던 failToRemoveSection 메소드의 경우에도 assertThatThrownBy를 사용하여 테스트를 작성하였던 터라 의아했는데요. LineSteps 클래스에 정의한 지하철_노선에_지하철_구간_생성_요청 메소드의 반환 타입인 ExtractableResponse와 관련이 있는건가 하고 추측만 하고 넘어갔습니다. 혹시 제가 잘못 이해한 부분이 무엇일까요?

[junwoochoi]

안녕하세요 미션 진행해주신 부분 확인했고, 몇가지 리뷰 달았으니 확인 부탁드립니다 :)

---

1. 인수 테스트는 요구사항을 기반으로 인수 조건을 만들고, 해당 조건이 통과하는 테스트를 만드시면 됩니다.
   예를 들면 특정 상황에 200 OK를 내려오게 일단 controller단에서 만들어놓거나, 샘플 응답데이터를 만들어 넣어놓고 TDD를 통해 세부 구현을 차근차근 구현하실 수 있습니다.
2. 해당 부분은 현재 sections의 순서를 list의 index순서 그대로 사용하고 계셔서 나타난 문제입니다. upStation과 downStation을 기반으로 각각의 역들을 이어서 순서를 맞춰서 getStations를 구현하시면 해결될 걸로 보이네요 😄
3. 코멘트에 따로 달았습니다!
4. failToAddSectionIfAllStationsExist는 인수테스트로 RestAssured를 활용하여 서버에 요청을 날리고 있습니다. 따라서 현재 테스트메소드에서는 실제 해당 메소드를 실행하는 것이 아니라 실제 요청을 만들어서 날리고 있기때문에 직접적으로 exception을 받는 것이 아니라, 해당 exception이 발생하여 globalExceptionHandler에서 바꿔준 400 BadRequest응답을 받게됩니다.
   Line도메인 테스트의 failToRemoveSection에서는 실제로 line객체를 만들어서 해당 객체의 remove함수를 직접 실행하는 방식으로 테스트 하였기 때문에 예외가 발생했고, 인수테스트의 경우 http API를RestAssured 를 통해서 테스트하고 있기 때문에 익셉션이 던져진게 아닌 서버로 부터의 에러응답을 받습니다.

[junwoochoi]

rearrangeSectionWithDownStation은 아래와 같은 경우 때문에 있었던 듯 하네요 :)

(정자 - 기흥), (기흥 - 수원) 

위 상태에서 (정자 - 미금)으로 요청이 온다면 rearrangeSectionWithUpstation이 정상동작하겠지만, (미금-기흥)으로 온다면 정상 동작되지 않아서 그런 것 같네요.
요구사항의 기존 구간 A-C에 신규 구간 A-B를 추가하는 경우 A역을 기준으로 추가를 어떻게 해석하느냐에 따라 달를 것 같아요.
현재는 항상 앞에것을 기준으로 사이역을 추가한다고 가정하면 rearrangeSectionWithUpstation만 있어도 될듯하네요~

[junwoochoi]

조건문이 뭘 뜻하는 지 명확하게 변수나 메소드로 추출해보면 좋겠네요 :)

[junwoochoi]

JPA 의 OneToMany는 따로 @orderby를 설정하지 않으면 순서가 정해지지 않습니다.
지금 upStation과 downStation을 기준으로 순서대로 찾으려면 id나 created_at으로는 알 수 없으니, 구간들을 이어서 맨 앞과 맨 뒤 역을 찾으셔야합니다 :)

[junwoochoi]

then 부분에 같은 로직이 반복되는데 리팩토링해보면 좋을 것 같습니다 :)

[junwoochoi]

각각의 예외케이스도 모두 잘 테스트해주셨네요 👍