refactor: 테스트 실행 속도 개선 및 중복 코드 제거 #557
Conversation
| protected RequestSpecification given(final Player player) { | ||
| final Member member = player.getMember(); | ||
| final AuthToken generate = authTokenGenerator.generate(member, member.getId(), AuthType.KAKAO); | ||
| final String accessToken = generate.getAccessToken(); | ||
| return RestAssured | ||
| .given().log().all() | ||
| .header("Authorization", "Bearer " + accessToken); | ||
| } | ||
|
|
||
| protected RequestSpecification given(final Member member) { | ||
| final AuthToken generate = authTokenGenerator.generate(member, member.getId(), AuthType.KAKAO); | ||
| final String accessToken = generate.getAccessToken(); | ||
| return RestAssured | ||
| .given().log().all() | ||
| .header("Authorization", "Bearer " + accessToken); | ||
| } |
There was a problem hiding this comment.
P2:
일단, 중복된 과정을 빼려는 시도는 매우 좋고 흥미롭네요!!
그리고 코드가 클린해진 것 같아서 좋습니다.
(쿠션)
하지만,
부모가 되는 추상클래스에 캡슐화(?)할 필요가 있는 부분일까요?
제 생각엔 두가지가 조금 걸리는데요.
- 이 메서드를 호출하는 자식 테스트에서 이것이 RestAssured 테스트임을 인지하기 힘들다.
- RestAssured로 체이닝을 시작해야 해당 테스트가 RestAssured라고 확실히 인지되지 않을까요?
- given()이라는 메서드명은 RestAssured에서 사용하는 요청을 구성하는 영역인데 해당 메서드명으로 Player나 Member를 받는 것이 직관적일까요?
- Auth 헤더에 특정 토큰을 넣는 것은 Controller 통합테스트의 중요한 테스트 영역이다.
- 인증 헤더를 넣냐 마냐는 되게 E2E 테스트에서 되게 중요한 부분인 것 같아요.
- 근데, 그 부분이 캡슐화 되어있으니까, 직관적으로 테스트를 읽는 사람이 해당 테스트에 인증정보가 있는지 없는지 파악하기 어려워 보입니다.
사실 두 문제 다 해당 메서드의 메서드명을 적절히 바꾸면 해결할 수 있는 문제인 것 같은데요.
제 생각에는 RestAssured의 요청을 만드는 영역을 전처리로 빼놓는 것은 테스트의 의도를 조금 헷갈리게 한다고 생각하는데, 제 의견 어떻게 생각하시나요?
차라리 인가 헤더 문자열을 만드는 메서드를 정의해놓는 것은 어떨까요?
There was a problem hiding this comment.
그렇게도 생각될 수 있겠네요. 확실히 auth 로직이 존재하는지 안하는지는 꽤 중요한 영역이라고 생각합니다. 이에 따라서 타는 interceptor 로직이 바뀌니까요!
다 어느정도 동의하는 생각이라서, 인가 헤더 문자열을 만드는 메서드를 정의하도록 하겠습니다.
There was a problem hiding this comment.
P4:
저도
Auth 헤더에 특정 토큰을 넣는 것은 Controller 통합테스트의 중요한 테스트 영역이다.
라는 루카의 말에 동의합니다.
사실 메서드를 캡슐화함으로써 가독성이 좋지는 않은 것 같아요.
하지만 변경 사항이 많은 부분이라 코코닥의 코드에서 메서드명만 바꿔도 괜찮을 것 같습니다
| @SuppressWarnings("NonAsciiCharacters") | ||
| @DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class) |
There was a problem hiding this comment.
P5:
해당 부분은 Domain에 대한 테스트이므로 POJO에 대한 테스트여서 따로 컨텍스트 캐싱을 할 필요는 없지만, 해당 부분의 중복 로직을 부모클래스로 뺼 수 있지 않을까요?
테스트들을 둘러보니까 어떤 도메인 테스트에 프로필 설정이 있는 것들이 있어서, 그런것도 지울겸... (도메인 테스트에는 프로필 설정은 따로 필요없는 것 같기는해요)
There was a problem hiding this comment.
인정합니다. 다만 도메인 테스트에 대해서는 클래스를 상속시키기보다, 커스텀 어노테이션을 만들어서 해결해도 좋을 것 같아요!
중복 코드를 한번 제거해보겠습니다.
There was a problem hiding this comment.
근데 생각해보니까 커스텀 어노테이션까지는 필요없어서, @ActiveProfiles 같은 POJO 테스트에서 필요없는 어노테이션을 제거했습니다!
| @SpringBootTest(webEnvironment = WebEnvironment.RANDOM_PORT) | ||
| public abstract class ControllerTest extends AbstractTest { | ||
|
|
||
| @Autowired | ||
| protected AuthTokenGenerator authTokenGenerator; | ||
|
|
||
| @Autowired | ||
| protected JwtProvider jwtProvider; | ||
|
|
||
| @LocalServerPort | ||
| private int port; | ||
|
|
||
| @BeforeEach | ||
| void setUp() { | ||
| RestAssured.port = port; | ||
| } |
There was a problem hiding this comment.
이런 다른 설정으로 인해서 ServiceTest와 ControllerTest가 다른 컨텍스트로 인식돼서 컨텍스트가 각각 한번씩 뜨는거군요!!
좋은데요?
| import org.springframework.boot.test.web.server.LocalServerPort; | ||
|
|
||
| @SpringBootTest(webEnvironment = WebEnvironment.RANDOM_PORT) | ||
| public abstract class ControllerTest extends AbstractTest { |
There was a problem hiding this comment.
abstract 클래스로 설정한 이유가 있나요? 추상 메서드가 보이지 않아서요
| @LocalServerPort | ||
| private int port; | ||
|
|
||
| @BeforeEach | ||
| void setUp() { | ||
| RestAssured.port = port; | ||
| } |
There was a problem hiding this comment.
SpringBootTest 어노테이션부터 해당 필드, 메서드까지 abstract 클래스에 두지 않은 이유가 있나요?
나머지 sql 어노테이션 등은 abstract 클래스에 있어서요
| protected RequestSpecification given(final Player player) { | ||
| final Member member = player.getMember(); | ||
| final AuthToken generate = authTokenGenerator.generate(member, member.getId(), AuthType.KAKAO); | ||
| final String accessToken = generate.getAccessToken(); | ||
| return RestAssured | ||
| .given().log().all() | ||
| .header("Authorization", "Bearer " + accessToken); | ||
| } | ||
|
|
||
| protected RequestSpecification given(final Member member) { | ||
| final AuthToken generate = authTokenGenerator.generate(member, member.getId(), AuthType.KAKAO); | ||
| final String accessToken = generate.getAccessToken(); | ||
| return RestAssured | ||
| .given().log().all() | ||
| .header("Authorization", "Bearer " + accessToken); | ||
| } |
There was a problem hiding this comment.
P4:
저도
Auth 헤더에 특정 토큰을 넣는 것은 Controller 통합테스트의 중요한 테스트 영역이다.
라는 루카의 말에 동의합니다.
사실 메서드를 캡슐화함으로써 가독성이 좋지는 않은 것 같아요.
하지만 변경 사항이 많은 부분이라 코코닥의 코드에서 메서드명만 바꿔도 괜찮을 것 같습니다
| import org.junit.jupiter.api.Test; | ||
| import org.springframework.beans.factory.annotation.Autowired; | ||
| import org.springframework.boot.test.context.SpringBootTest; | ||
| import org.springframework.boot.test.mock.mockito.MockBean; | ||
| import org.springframework.test.context.jdbc.Sql; | ||
|
|
||
| @SuppressWarnings("NonAsciiCharacters") |
There was a problem hiding this comment.
P5:
해당 부분은 추상화? 하지 않은 이유가 뭔가요
| @Autowired | ||
| private HintService hintService; |
There was a problem hiding this comment.
P5:
Service클래스들은 추상화 하지 않은 이유가 있나요?
| @SpringBootTest(webEnvironment = WebEnvironment.RANDOM_PORT) | ||
| public abstract class ControllerTest extends AbstractTest { | ||
|
|
||
| @Autowired | ||
| protected AuthTokenGenerator authTokenGenerator; | ||
|
|
||
| @Autowired | ||
| protected JwtProvider jwtProvider; | ||
|
|
||
| @LocalServerPort | ||
| private int port; | ||
|
|
||
| @BeforeEach | ||
| void setUp() { | ||
| RestAssured.port = port; | ||
| } |
There was a problem hiding this comment.
코코닥 해당 문제를 발견하고 고친 거 👍🏻👍🏻
딱히 고칠만한 부분이 잘 보이지 않네요
+) 하나 궁금한 점은
왜 처음 테스트 코드에서 6개의 ApplicationContext 를 띄우는 과정이 실행된건가요?
컨텍스트 캐싱이 되는 조건이
같은 bean의 조합을 필요로 하고
이전 테스트에서 ApplicationContext가 오염되지 않은 경우
인데,
@MockBean이 있는 테스트의 경우 bean의 조합이 달라지기 때문에 해당 문제가 발생하는 것 같은데,
그런 클래스는 2개 밖에 존재하지 않아서요.
2+1+1 = 4번의ApplicationContext가 띄워지는 게 맞지 않나요?
📌 관련 이슈
🛠️ 작업 내용
🎯 리뷰 포인트
테스트 실행 속도 개선
@SpringBootTest어노테이션을 활용하여 테스트를 진행 시에, 스프링에서는 내부적으로Test Context Caching을 사용해서 매 테스트마다 새로운ApplicationContext를 만들지 않도록 합니다.다만 이 컨텍스트 캐싱이 무조건 되는 것은 아니고, 특정 조건들을 만족해야 같은 컨텍스트라 인식하여
ApplicationContext를 재활용하게 됩니다. (캐싱이 되는 조건은 이 블로그 글과 스프링 공식 문서를 참고했습니다. 궁금하시다면 여기를 참고하면 좋을 것 같아요 ㅎㅎ)현재 저희 dev 브랜치 테스트 코드에서는 총 6번의 ApplicationContext 를 띄우는 과정을 거칩니다.
이를 최적화하기 위해 같은 성격의 테스트들을 abstract class 로 추출하여 묶었고, 각각의 테스트들이 이를 상속하게 하여 컨텍스트 캐싱을 적극 활용할 수 있도록 수정했습니다.
이 PR의 결과에서는 총 6번 → 2번의 ApplicationContext 를 띄우게끔 최적화되었고, 아래와 같이 테스트 속도가 개선된 모습을 보실 수 있습니다. (IDE에서 테스트 코드 실행 속도는 비슷하게 나오지만, 이는 ApplicationContext 로딩 시간이 포함되지 않은 시간이라고 합니다!)
↑ 총 6번의 ApplicationContext 로드 ↑
↑ 총 2번의 ApplicationContext 로드 ↑
또한, 테스트 리소스를 잡아먹는 코드들을 제거하여 테스트 실행 속도를 최대한 끌어올렸습니다. 결과는 아래와 같습니다.
최종적으로, 제 로컬 기준 테스트 실행 시간이 13.5초 → 5.7초로 개선되었습니다.
테스트 중복 코드 제거
총 3개의 abstract class 로 테스트 유형을 정의했고, 앞으로 테스트 코드 짜실때 얘네들만 상속시켜주시면 Builder 나 Repository 같은거 따로
@Autowired사용 안해도 됩니당.AbstractTest
최상위 테스트 클래스입니다. 여기에 공통적으로 사용되는 테스트 Builder 나, Repository, MockBean 들을 정의해놨습니다!
ControllerTest
인수 테스트시에 사용되는 테스트 클래스입니다.
기존에 사용되던 토큰 인가 중복 코드가 너무 지저분해서, 이곳에 토큰 인가 관련 중복 제거용 메서드를 정의해두었습니다.
이렇게 생겼는데, 앞으로 RestAssured.given() 대신 이 메서드를 사용하시면 됩니다! 이 메서드 파라미터로, 인가에 사용될 Player 나 Member 객체를 넣어주게 되면 알아서 Bearer 인증 헤더가 포함되게 해놨습니다.
ServiceTest
내용은 아직 없고, 그냥 테스트 컨텍스트 캐싱용으로 사용하기 위해 만들었습니다.
코드 변경량이 많긴한데 다 같은 패턴의 작업들이라서, 보실만한 지점은 기존의 컨트롤러나 서비스 테스트가 어떻게 바뀌었는지, 토큰 인가 로직 어떻게 바뀌었는지 정도 봐주시면 될 것 같아요 👨🚀
⏳ 작업 시간
추정 시간: 4시간
실제 시간: 6시간
테스트 컨텍스트 캐싱 공부좀 한다고 좀 걸렸습니다..