Skip to content

[4주차] RateLimit 적용 및 테스트 커버리지 개선#85

Merged
BAEKJungHo merged 12 commits intohanghae-skillup:devops3199from
devops3199:main
Feb 7, 2025
Merged

[4주차] RateLimit 적용 및 테스트 커버리지 개선#85
BAEKJungHo merged 12 commits intohanghae-skillup:devops3199from
devops3199:main

Conversation

@devops3199
Copy link

[4주차] RateLimit 적용 및 테스트 커버리지 개선

작업 내용

이번 PR에서 진행된 주요 변경 사항을 기술해 주세요.

  • 영화 조회 RateLimit 구현
  • 예약 API RateLimit 구현
  • 테스트 코드 작성 및 커버리지 개선

이번 주차에서 고민되었던 지점이나, 어려웠던 점을 알려 주세요.

과제를 해결하며 특히 어려웠던 점이나 고민되었던 지점이 있다면 남겨주세요.

  • 어떻게 테스트 코드를 어떻게 작성해야할지 모르겠습니다. 관련 도서나 강의 추천 부탁드립니다 💯
    • 서비스 로직, querydsl 동적 쿼리, 통합 테스트 경우 로직이 있어 어떤 부분을 테스트할지 명확했지만, 객체만 있는 domain 모듈이나 단순히 데이터를 가져오고 저장하는 repository 클래스는 테스트 코드가 정말 필요할까 생각했습니다.

리뷰 포인트

리뷰어가 특히 의견을 주었으면 하는 부분이 있다면 작성해 주세요.

기타 질문

추가로 질문하고 싶은 내용이 있다면 남겨주세요.

@BAEKJungHo
Copy link
Contributor

@devops3199 안녕하세요 찬엽님 ~! 가장 먼저 4주차를 구현해주셨네요 ! 4주간 고생 많으셨습니다 👍🏿
리뷰 진행하겠습니다 :)

private SeatPersistenceAdapter seatPersistenceAdapter;

@InjectMocks
private CreateBookingService createBookingService;
Copy link
Contributor

@BAEKJungHo BAEKJungHo Feb 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

테스트 대상을 SUT(System Under Test) 이라고 부르는데, 실무에서는 테스트 대상을 코드 내에서 빠르게 식별하기 위해서 sut 이라는 네이밍도 자주 사용합니다.

.parameter(LocalDate.class)
.parameter(Set.class))
.set("seats", Set.of(TheaterSeat.B1, TheaterSeat.C1, TheaterSeat.D1))
.sample();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set.of(TheaterSeat.B1, TheaterSeat.C1, TheaterSeat.D1) 부분을 불연속적인 좌석 을 의미하는 변수로 추출하여 전달하면 유지보수성 및 가독성이 더 좋아질 것 같습니다.


var result = searchMovieService.searchMovies(searchCommand);

assertEquals(10, result.size());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

10이라는 매직넘버를 상수로 관리하거나 지역 변수로 관리하면 중복을 없앨 수 있을 것 같습니다.

}
}, Collections.singletonList(key));

if (Boolean.FALSE.equals(allowed)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

깔끔하게 잘 구현해주셨네요 ! 💯


@Test
public void Rate_Limit_테스트() {
var bookingRequest = new CreateBookingRequest(1L, 2L, 5L, 1L, LocalDate.of(2025, 3, 1), List.of("C1", "C2"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

테스트 메서드 이름을 더 명확하게 작성하면 좋을 것 같아요 !

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public void Rate_Limit_유저_영화_시간표_상영관_per_1요청_5분_테스트()

위처럼 어떤 rate limit 테스트인지 명시적으로 수정했습니다!

@SpringBootTest(classes = EmbeddedRedisConfig.class)
@TestPropertySource(properties = "spring.config.location = classpath:application-test.yml")
@Sql(scripts = "/booking-data.sql", executionPhase = Sql.ExecutionPhase.BEFORE_TEST_CLASS)
public class BookingControllerTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

독립적인 환경을 잘 구성해서 진행해주셨네요 👍🏿

blockedIps.put(clientIp, LocalDateTime.now());
throw new RateLimitException();
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

깔끔하게 잘 구현해주셨습니다 👍🏿

@BAEKJungHo
Copy link
Contributor

@devops3199 님 너무 잘 구현해주셨습니다 :) 코드 깔끔하네요 💯

좋았던 점

  • 코드 작성을 너무 깔끔하게 잘 해주셨습니다 👍🏿
  • 독립적인 환경을 만들고 통합 테스트 코드를 작성해주셔서 좋았습니다.
  • 커버리지 결과를 잘 첨부해주셨습니다 !
  • RateLimit 활용을 잘 해주셨습니다 !

아쉬웠던 점

  • 4주차 리뷰와는 별개지만, 로직을 조금 살펴봤습니다. checkSeatsInSequence 이러한 메서드들은 애플리케이션 로직이 아닌 도메인 로직으로써 TheaterSeat 내에서 검증 로직을 관리하는 것이 더 응집도를 높일 수 있을 것 같습니다.
  • 위와 관련해서, 도메인 로직들이 잘 응집되어있으면 단위 테스트를 작성하기 쉽고, 문서화로서 역할을 하기 좋다는 건데요, 통합 테스트는 잘 작성해주셨지만, 도메인 로직에 대한 단위 테스트가 없다는 점이 아쉽습니다. 실무에서는 단위 테스트의 개수가 더 많아야 합니다 ! 통합 테스트는 커버리지를 많이 높이는데 훌륭하며, 복잡한 시나리오를 커버하기 위해서 작성하는 경우가 많습니다.

리뷰 포인트 및 추가 질문에 대한 답변

관련 도서나 강의 추천 부탁드립니다 💯

  • Unit Testing Principles, Practices, and Patterns 책 추천드립니다.
  • 강의는 제가 잘 안들어서 잘 모르겠네요 !
  • 실무에서는 사실 AI 를 활용해서 테스트 코드를 작성하기 때문에, 엄청난 테크닉이 필요하다고 생각되진 않습니다. 다만, AI 를 활용하는 경우 Production Code 에 대해서 설계를 바꿀 필요는 없는지에 대한 피드백을 받긴 어렵습니다.

서비스 로직, querydsl 동적 쿼리, 통합 테스트 경우 로직이 있어 어떤 부분을 테스트할지 명확했지만, 객체만 있는 domain 모듈이나 단순히 데이터를 가져오고 저장하는 repository 클래스는 테스트 코드가 정말 필요할까 생각했습니다.

사실 repository 테스트는 통합테스트가 있다면 굳이 필요할까라는 생각을 저도 갖고 있긴 합니다. 복잡한 쿼리 위주의 애플리케이션(e.g 통계 등)이라면 의미가 있을 수 있겠습니다. 그 외의 경우, 실무에서 일정 수치의 커버리지를 만족 시켜야 한다고 하면 작성을 해야겠죠. 혹은 해당 부분은 측정되지 않도록 제외시키는 등의 작업이 필요합니다.

도메인 로직에 대한 테스트는 중요합니다.

@devops3199
Copy link
Author

devops3199 commented Feb 4, 2025

@BAEKJungHo
안녕하세요 코치님, 피드백 감사드리고 리뷰기반으로 코드 수정했습니다. domain 모듈도 테스트 코드 작성했습니다.
말씀대로 유즈케이스 로직 일부(예 - checkSeatsInSequence)를 도메인으로 내리니까 별도 테스트가 가능해진 장점이 있네요!
4주간 정말 고생 많으셨습니다. 해당 수업 덕분에 많이 배우고 갑니다 👍


@SpringBootTest(classes = EmbeddedRedisConfig.class)
@TestPropertySource(properties = "spring.config.location = classpath:application-test.yml")
public class SeatTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

단위 테스트 잘 작성해주셨네요 👍🏿 단위 테스트의 경우에는 @SpringBootTest 가 필요 없을 것 같습니다 !


@Test
public void 연속된_열_체크_테스트() {
var discontinuousSeats = Set.of(TheaterSeat.B1, TheaterSeat.C1, TheaterSeat.D1);
Copy link
Contributor

@BAEKJungHo BAEKJungHo Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

테스트 메서드를 한글로 해주셔서 가독성이 높아졌습니다. 다만 조금 더 구체적으로 작성하면 좋을 것 같아요.

  • e.g 비연속적인 열이 주어지는 경우 예외가 발생한다

@BAEKJungHo
Copy link
Contributor

@devops3199 감사합니다 ㅎㅎ 4주간 고생 많으셨습니다 !! 💯

@BAEKJungHo BAEKJungHo merged commit 882923a into hanghae-skillup:devops3199 Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants