Test: use fixture

[goldentrash]

• 멤버 등록에 필수 속성을 추가했습니다. (Role, Department etc...)
• Controller Test에 관리자 권한이 필요한 경우 미리 반영했습니다.
  • Security 속성 반영은 개발 끝나면 일괄 적용하는게 나을듯 합니다
• Test Fixture 작성, 적용
  • domain/과 external/의 테스트는 Test Data가 명시적으로 드러나는게 좋다 생각해서 Fixture 적용 안 했습니다
  • controller/와 application/은 대부분 해당 method의 동작 및 반환에 Test Data의 상세 내용이 중요하지는 않다고 생각했습니다.
• Controller Test를 단위 테스트로 전환
  • Spring Security Form Login은 Security 테스트하는거라 생각해서 통합 테스트 유지했습니다.
  • 테스트 총 러닝 타임은 생각보다 안 줄더군요

[ekgns33]

오.. 고생하셨습니다.
내용이 많은데 공통적으로 고민이 되는 것들만 적어볼게요.

1. 지금까지 서비스 레이어도 테스트코드를 작성했었는데 컨트롤러테스트를 하면 서비스레이어의 테스트코드가 필요한지 고민입니다. 도메인단과 통합테스트만으로 충분한 것 같기도하네요.

2. 오류를 뱉는 테스트들에 대해서 불필요한 값들이 들어간 테스트가 보이기도 합니다. Not Null과 같은 제한때문에 집어넣은 객체들에 대해서는 어쩔 수 없다고 생각이 드는데 테스트할 대상만 빌더를 통해 생성해야할 것 같아요.

ex) 이미 존재하는 멤버 확인 << 멤버 email만 필요.

3. Fixture를 이렇게 사용해본적은 없는 지라 저도 잘 모르겠습니다ㅠㅠ 😢 . 그런데 서비스 레이어의 코드를 걷어낸다고 생각하고 컨트롤러와 도메인만 두고 생각해볼게요. 중복되는 객체를 정리하는 것은 필요해 보이지만 컨트롤러테스트에서 getEventFixture1()과 같이 객체를 사용하는 데에 저는 부정적으로 바라봅니다.

제가 생각한 이유는 이러해요.

1. 중복되는 값이라면 테스트 클래스상단에 정의한다.
2. 테스트코드를 읽을때 봐야할 레이어가 하나 더 생긴다. 파일을 들어가서 이 게터가 어떤 객체를 반환하는지 봐야하기 때문이죠. (결국 피로하다...라는 문제입니다.)

[goldentrash]

요약

• Fixture Factory가 테스트 목적과 관계가 있는 속성만을 입력받도록 Factory를 분리하면 괜찮지 않을까요?
• /application 테스트를 걷어내고 통합 테스트로 전환하더라도 RestDoc을 위한 단위 테스트는 별도로 작성했으면 합니다.

리뷰 답변

우선, 상당히 큰 PR이었는데 정성 들여 검토해 주셔서 감사합니다. 역시 테스트는 어렵네요. 짚어주신 내용들 모두 공감하고 있습니다.

저 역시 테스트는 WET하게, Verbose하게 작성해야 한다고 생각합니다. 언급해 주신 것처럼 테스트가 실패할 경우 원인 분석이 빨라야 한다고 생각해요. 테스트 실패의 원인이 테스트 Case 내부가 아닌 외부(Fixture)에 있다면 원인 파악에 어려움이 따르겠죠. (이만저만 피로한게 아닐겁니다) 뿐만 아니라, 외부 Fixture를 사용하다간 자칫 테스트 Case 간 응집이 강해져 테스트 하나의 변경이 주변 테스트로 전파될 위험도 분명히 존재합니다.

그렇기에 제가 이런 PR을 작성하게 될 것이라고는 생각하지 못했는데, 작성하면서 저 스스로도 신기하고 이게 정말 맞는지 고민이 많이 되더군요. 그럼에도 불구하고 테스트 Fixture를 분리해야겠다 마음먹은 이유가 있었습니다.

Fixture가 필요하다고 생각하는 이유

1. 불필요한 정보 과다로 오히려 테스트 의도가 묻힘
2. 테스트 작성 과정에서 충분히 다양한 Data를 떠올리기 어려움 (특정 Case 누락 발생)

(각 사안 예시는 아래에 별도로 작성해 두겠습니다)

Fixture를 사용하기로 했다면 이제 2가지 문제가 따라오게 되지요.

1. Fixture를 어떻게 구성하고, 불러올(사용할) 것인가?
2. Fixture를 어디에 사용할 것인가?

Fixture를 어떻게 구성할 것인가? -- External Static Factory

1번 고민에 대해서는 별도 Factory 클래스로 분리하는 방향을 생각했습니다. 테스트 클래스 자체로 private factory나 private instance를 작성하는 방향을 언급해 주셨는데, 개인적으로는 이미 Fixture를 사용하는 시점에서 피로도의 차이는 크지 않다고 생각합니다. 테스트 클래스 상단이든, Before/After Setup 블럭이든 어차피 테스트 블럭을 벗어나야 Fixture 내용을 확인할 수 있는 것은 동일하니까요.

그러한 이유로 External Static Factory를 작성했는데, kotlin이나 js와 달리 java는 method에 default parameter를 지원하지 않기에 Fixture Factory가 모든 인자를 받아오게끔 하거나, 필요한 인자마다 factory를 분리해야 했습니다.

첫 번째의 경우에는 기존과 다를 것이 없고, 두 번째 경우에는 메서드 분기가 너무 과해지지 않을까 생각했습니다. 그래서 고민 끝에 "메서드 이름에 해당 Fixture의 의도가 분명히 드러나면 충분하지 않을까...?"란 결론에 도달했습니다만

예, 제 판단 미스였던 것으로 보입니다. 언급하신 것처럼 getEventFixture1으로는 충분한 정보가 전달되지 않습니다.

오류를 뱉는 테스트들에 대해서 불필요한 값들이 들어간 테스트가 보이기도 합니다. Not Null과 같은 제한때문에 집어넣은 객체들에 대해서는 어쩔 수 없다고 생각이 드는데 테스트할 대상만 빌더를 통해 생성해야할 것 같아요.

결국 검증하고자 하는 인자만으로 Fixture를 생성하는 Factory를 만드는 것이 가장 좋은 방법이 아닐까 생각합니다. 예시로 이미 작성한 getEmailTaskFixtureWillSendAfterXSeconds가 있겠네요.

Fixture를 어디까지 사용해야 하는가?

원래는 RestDoc 테스트에만 적용할 생각이었습니다. RestDoc Test는 어떠한 기능에 대한 Functionality를 테스트하기보다는, 요청과 응답 간의 Payload Scheme을 확인하는 문서로서의 기능이 더 커 보입니다. 때문에 객체의 값 속성 하나하나는 크게 중요하지 않다고 생각했어요.

그래서 실제로 /domain과 /external에 위치한 단위 테스트에는 Fixture를 적용하지 않았습니다. 다만 고민은 /application에 위치한 테스트였습니다. 처음 계획할 때는 Fixture 적용 대상이 아니었지만, 이미 Fixture Instance (like mock1)를 적용한 상태였기에, 일괄 적용했습니다.

예, 이 부분 역시 아쉬운 판단으로 보입니다. RestDoc Test와 다르게 Application 테스트는 실제 비즈니스 로직의 동작을 검증합니다. 때문에, 입력과 출력값의 속성이 매우 중요합니다. 현 Fixture Factory로는 해당 속성이 충분히 가시적으로 드러나지 않습니다. 다만 이 문제는 위에서 언급했던, 필요한 속성별로 factory를 분리한다면 어떨까 싶습니다. 다시 한번 getEmailTaskFixtureWillSendAfterXSeconds를 주목해 주세요.

RestDoc 테스트는 조금 독특한거 같습니다...?

지금까지 서비스 레이어도 테스트코드를 작성했었는데 컨트롤러테스트를 하면 서비스레이어의 테스트코드가 필요한지 고민입니다. 도메인단과 통합테스트만으로 충분한 것 같기도하네요.

우선 해당 내용에 동의합니다. 다만, RestDoc을 사용한다면, 문서를 위한 테스트는 통합 테스트가 아닌 단위 테스트였으면 합니다.

문서는 API 당 하나의 Case만 있으면 되지 않나 싶습니다. 다양한 응답 예제가 필요하다면 테스트 Case를 늘리기보다는 하나의 응답에 다양한 객체가 포함되도록 하면 되지 않을까요? 가령 이벤트 목록 조회에 출석이 있는 이벤트와 출석이 없는 이벤트가 모두 포함되도록요. 물론 BadRequest와 같이 요청이 달라지는 경우에 대해서는 다른 Test Case가 필요하겠지요.

즉, "서비스 레이어 테스트를 걷어내고, 통합테스트와 도메인 단위테스트로 전환"하는데 동의합니다. 다만 이 경우 RestDoc을 통합테스트를 통해 생성하는 대신, RestDoc 작성만을 위한 별도 클라이언트 단위 테스트도 작성하는게 좋다고 생각합니다.

[goldentrash]

Fixture를 Service Test에도 적용하기로 결정한 이유

원래 Service Layer에는 Fixture를 사용하지 않으려 했었는데, 이미 적용 중인 부분이 있어 일괄 적용했습니다.

[goldentrash]

Fixture 사용 이유 - 필요한 Case 누락이 쉬움

기존 Test Data에는 썸네일이 누락된 경우가 포함되지 않았습니다. 결과적으로 Optional 한 payload가 늘 있어야만 하는 것처럼 문서화되었어요.

실제로 (물론 기획과의 소통이 더 원활했더라면 발생하지 않았겠지만) 프런트 팀이 썸네일이 무조건 있어야 한다고 생각하고 이벤트 세션 등록에 반드시 하나 이상의 이미지를 등록하도록 강제하는 경우도 있었습니다.

[goldentrash]

Fixture 분리가 필요하다 생각했던 시점

불필요한 정보 과다로 오히려 테스트하고자 하는 내용이 눈에 들어오지 않음

[goldentrash]

Fixture 분리를 마음먹은 시점 2

[goldentrash]

제 의견에 대한 생각이 궁금하네요 @ekgns33
만약 긍정적으로 생각하신다면, Factory가 테스트 목적이 되는 인자만 받도록 분기하는 방향으로 커밋을 올리겠습니다.
서비스 레이어 테스트의 통합테스트 전환은 별도 PR로 관리해야 한다고 생각해서 이 PR에 추가 커밋을 하지는 않겠습니다.

[ekgns33]

역시 이런 큰 고민이 있었군요 😢
읽고 저도 고민을 해봤습니다! 제가 생각한 바로는

1. RestDocsTest와 컨트롤러 테스트를 분리하자. (매우동의)

이름에 걸맞은 문서화를 위한 테스트와 우리가 테스트하고자하는 유즈케이스를 테스트하는 코드는 분리하는게 맞아보이네요

2. Fixture 클래스가 모든 값을 들고있기보다는 팩토리메서드를 두고 필요한 값을 주입받도록하자.

예, 이 부분 역시 아쉬운 판단으로 보입니다. RestDoc Test와 다르게 Application 테스트는 실제 비즈니스 로직의 동작을 검증합니다. 때문에, 입력과 출력값의 속성이 매우 중요합니다. 현 Fixture Factory로는 해당 속성이 충분히 가시적으로 드러나지 않습니다. 다만 이 문제는 위에서 언급했던, 필요한 속성별로 factory를 분리한다면 어떨까 싶습니다. 다시 한번 getEmailTaskFixtureWillSendAfterXSeconds를 주목해 주세요.

허나 상황마다 팩토리메서드를 다만들자는 의미는 아닙니다.
유즈케이스마다 모든 fixture를 구현하게되면 변경에 너무 취약한 코드가 될 것 같아요.

• 개인적인 선호인 것 같지만 저는 서비스레이어 내의 매핑객체가 아니라 RequestBody를 의미하는 Request객체라면 필드를 명시해서 보여주는게 더 알아보기 쉬운 것 같아요.

뭔가 테스트의 범위 내에서 이렇게 명시하는게 전 보기가 편하더라구요. Json이 머리에 생각되는...

EventFixture.createFixtureWithTime(1000)

EventTask.builder()
  ...
  .delay(1000)
  .build();

이건 제 개인취향입니다 ㅎㅅㅎ 😆

그러한 이유로 External Static Factory를 작성했는데, kotlin이나 js와 달리 java는 method에 default parameter를 지원하지 않기에 Fixture Factory가 모든 인자를 받아오게끔 하거나, 필요한 인자마다 factory를 분리해야 했습니다.

코틀린에서 defualt parmater가 있었다면 저희는 걱정을 할 것이 줄어들 수도 있었겠죠! 허나 그렇지 않기에 빌더패턴과 비슷한 형태로 직접 만들어야하지 않을까 생각이됩니다. (근데 이러면 빌더를 사용하는 것과 뭐가 다를까요.)

[goldentrash]

허나 상황마다 팩토리메서드를 다만들자는 의미는 아닙니다.
유즈케이스마다 모든 fixture를 구현하게되면 변경에 너무 취약한 코드가 될 것 같아요.
개인적인 선호인 것 같지만 저는 서비스레이어 내의 매핑객체가 아니라 RequestBody를 의미하는 Request객체라면 필드를 명시해서 보여주는게 더 알아보기 쉬운 것 같아요.

그렇다면 아래와 같은 방향은 어떨까요 @ekgns33 ?

@Getter
public class MemberFixture {

  Member fixture;

  @Builder
  public MemberFixture(Long id, String memberId, String password, String name, String email, String department, MemberRole role){
    this.fixture = Member.builder()
      .id(getDefault(id, 0L))
      .memberId(getDefault(memberId, "202400000"))
      .password(getDefault(password, "$2a$10$d7DjseDroHsRGVGR1zDUL.q7uwAQ2aH4nHM1JiQ1OFV.D0qUTl7w."))
      .name(getDefault(name, "member"))
      .email(getDefault(email, "[email protected]"))
      .department(getDefault(department, "Computer Science"))
      .role(getDefault(role, MemberRole.MEMBER).toString())
      .batch(getDefault(BATCH, "24-25"))
      .build();
  }

  private static <T>T getDefault(T value, T defaultValue){
    return value == null ? defaultValue : value;
  }
}

// given
Member memberToRegister = MemberFixture.builder().id(1L).memberId("202400001").build().getFixture();

Builder의 형태를 유지시키면서도 필요한 속성만 설정하고 나머지는 기본값을 사용합니다. 이렇게 할 경우 실 Domain 객체에서 기본값을 사용할 수 없는 속성(memberId, password etc)도 테스트를 위해 기본값을 적용할 수 있습니다.

[sonarcloud]

Quality Gate passed

Issues
8 New issues
0 Accepted issues

Measures
0 Security Hotspots
82.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud