test: 가입시 인증 로직 테스트 코드 작성

[mrlee7]

작업 내용

• 문자 인증을 받는 부분의 인증 기능 테스트 작성 추가하였습니다.
  • 인증키를 받고, 해당 키로 인증 성공하는 것까지의 시나리오만을 인수테스트 진행하였습니다.
  • 단위 테스트 > 통합 테스트 > 인수 테스트 순으로 테스트 개수를 가지도록 하였습니다.
    • 마틴 파울러의 피라미드 를 따랐는데요, 인수테스트나 통합테스트가 무거워질수록 빌드타임에 영향을 끼칠듯 하여 서비스 레이어 테스트를 풍부하게 가는 기조로 작성했습니다.

AS-IS

TO-BE

주의 사항

1. private 메소드를 테스트 하는 것에 대한 고민
   private method를 테스트 하는 것을 고민해보았습니다.

@DisplayName("인증키 생성시마다 랜덤한 문자열이 생성된다.")
@Test
void generateCertificationKey()
        throws NoSuchMethodException, InvocationTargetException, IllegalAccessException {

    // given & when
    Method method = AuthService.class.getDeclaredMethod("generateCertificationKey");
    method.setAccessible(true);

    String actual1 = (String) method.invoke(authService);
    String actual2 = (String) method.invoke(authService);

    // then
    assertThat(actual1).isNotEqualTo(actual2);
}

private 메소드를 사용하는 public 메소드에서 충분한 검증이 이뤄져야 한다는 생각으로 귀결되어 private method 테스트는 진행하지 않았는데요, public이 아닌 메소드들의 테스트에 대해 어떤식으로 테스트를 할지 의견 부탁드리겠습니다 :)

2. 라인 커버리지 100%?
   토스의 테스트 커버리지 영상을 보면, 100% 로 커버리지 목표를 잡은 이유가 상당히 reasonable해보입니다.
   다만, 라인 커버리지에서 벗어나 엣지 케이스를 어떤식으로 정립하고 테스트 코드를 작성할 것인가에 대해서는 아직 고민이 많습니다.. 이 부분도 정책으로 세울 수 있다면 같이 이야기해보아도 좋을 것 같아요 :) ( 아직도 테스트 코드 작성에 어려움이 많네요.. ㅎㅎ )

3. 인증부랑 결합이 되어있는 SMS 테스트 고민
   이번에 인증쪽을 테스트 하면서 SMS도 Event(Pub-Sub) 구조로 결합도를 떨어뜨리면서 같이 테스트를 진행하려 하였습니다.
   -> 이슈
   Auth 도메인이랑 Order 도메인에 SMS가 결합되어있는데, Topic별로 관리하는 것에 어려움이 있어 삽질을 좀 깊게 하고 있습니다.
   도메인 별로 Pub-Sub Topic을 관리하는 방법에 대한 고민이 필요하여, 후순위로 미뤄두겠습니다. ( 좋은 의견이 있다면 부탁드립니다 🙏 )

4. 테스트 공통 부분
   ControllerTest나 AcceptanceTest 작성시 공통으로 사용되는 부분이 많아보여요 !
   추상 클래스로 빼는 등의 리팩토링을 추후 같이해보면 어떨까요 ! ;)

Reference

• https://stackoverflow.com/questions/105007/should-i-test-private-methods-or-only-public-ones#105021
• https://fishbowl.pastiche.org/2003/03/28/testing_private_methods_dont_do_it

PR 체크리스트

• 테스트는 모두 통과했나요?
• 빌드는 성공했나요?
• 코드 포맷팅을 진행했나요?

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[joelonsw]

테스트 잘 써주셔서 딱히 건드릴게 없네요! 좋습니다! 💪 질문해주신거 같이 얘기해보죠

1. private 메서드는 테스트 할 필요 없다고 생각합니다. 아래 레퍼런스로 남겨주신 스택오버플로우에서 처럼 private 메서드를 테스트 할라고 억지로 지지고 볶는건 캡슐화를 깨뜨리는 행위로 볼 수 있어요. 물론 참고로 써주신 리플렉션을 활용하여 코드를 실행할 수는 있겠지만, 안하는게 좋을 것 같아요. 감춘것은 감췄을 때 의미를 가지니까요.

   • 만~약에 'private 메서드를 테스트 해봐야할 것 같은데?' 라는 생각이 든다면 프로덕션 코드를 쪼꼼 수정해야하는 신호가 아닌가 생각해볼 것 같긴하네요. 해당 메서드를 다른 클래스로 따로 빼서 public 접근자를 갖도록 리팩터링 해보는 방향도 생각해볼 것 같아요.
   • 예시로 주신 AuthService의 generateCertificationKey()를 볼까요?

   private String generateCertificationKey() {
       return RandomStringUtils.randomNumeric(6);
   }

   • 아 이거 테스트하고 싶어! 해서 리플렉션을 써서 테스트를 해볼 수도 있어요.
   • 하지만 그전에 이걸 왜 private으로 해뒀는지 생각해봅시다.
   • 인증에 쓰일 길이 6짜리 문자열을 만드는 기능을 굳이 public으로 해둘 필요가 없어서겠죠?
   • 우리 한 번 객체지향적으로 생각해봅시다. AuthService의 책임은 무엇일까요? Connectable 서비스의 인증 절차를 똑바로 잘 수행하는게 AuthService의 책임일거에요.
   • 그렇다면, 랜덤하게 길이가 6짜리 String을 만드는게 과연 AuthService의 책임이라고 할 수 있을까요? 전 아니라고 보긴합니다. AuthService의 책임은 인증을 위한 문자열을 이를 레디스에 저장하여 나중에 인증을 할 때 체크하는 책임/역할을 가지긴 하지만, 랜덤한 문자열을 만드는 역할은 가지고 있을 필요가 없어요.
   • 제가 저번에 connectable.global.common.util 패키지에 랜덤 스트링을 생성하는 RandomStringUtil 클래스를 활용해보는걸 살짝 제안드렸는데요. 그때도 아마 문자열 생성의 책임은 Service단에 있을 필요가 없다고 판단해서 였을 겁니다. 또한 해당 클래스를 사용하면 문자열 생성되는건 public으로 열어둘테니 테스트하기도 용이하겠죠.
   • 물론! 책임이라는 단어는 굉장히 주관적이기 때문에 얘기하는 사람마다 다를수 있어요. 인증에 쓰일 문자열 만드는 것도 온전히 AuthService의 책임이다~ 라고 하면 할말은 없는거죠. 하지만 Private 메서드 테스트에 대한 애기가 나와서 제 생각 한 번 공유드립니다.

2. 라인 커버리지 100%

   • 이런 장점을 얘기하신 것 같네요.
     • 자신있게 누를수 있는 배포버튼
     • 거침없는 리팩터링
     • 불필요한 프로덕션 코드 사라짐
     • 프로덕션 코드에 대한 이해도 상승
     • 점점 쉬워지는 테스트 작성
   • 전 개인적으로 해당 이점은 커버리지 80%만 되어도 충분하다고 보긴합니다.
   • DTO 이런거 다 테스트 해야 100프로가 될텐데,, 전 조꼼 소모적이지 않나 생각해보긴합니다.
   • 물론 고고익선인건 동의합니다!

3. Event Pub-Sub 빡세죠. 나중에 필요하면 같이 페어 프로그래밍 해보죠!

4. 좋습니다. 추후에 공통으로 빼시죠!

[joelonsw]

컨트롤러 테스트 좋네요...! 응답을 간결하게 확인하기 좋아보입니다 굿