Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#4] Controller 및 테스트 코드 작성 #6

Merged
merged 14 commits into from
Nov 27, 2024
Merged

[#4] Controller 및 테스트 코드 작성 #6

merged 14 commits into from
Nov 27, 2024

Conversation

sssukho
Copy link
Collaborator

@sssukho sssukho commented Nov 19, 2024

Shop Controller 일부(Shop 등록, Shop 리스트 조회)와 그에 해당하는 테스트 코드 작성하였습니다.

아래와 같이 테스트 해야 할 항목들을 정해서 테스트 코드를 작성해 나갔습니다.

  1. controller 로 들어오는 input 이 원하는 java object 로 잘 serialization 되는지 확인
  2. input validation check 가 잘 동작하는지 확인
  3. controller 에서 나가는 http 응답 본문에 유효한 값들이 포함되어 있는지 확인
  4. exception handler 가 의도한 바와 같이 잘 동작하는지 확인

Shop Controller 뿐만 아니라 다른 테스트 코드들도 계속해서 작성해 나갈 예정입니다.😅

이외에 추가로 궁금한 부분이 있습니다.

  1. 차후 가까운 거리별 샵 조회 기능을 위해 샵의 주소나 위치 정보를 가지고 있어야 할 것 같은데, bp-dto 모듈 아래에 있는 ShopRegistrationRequest.Address 의 멤버 변수들이면 충분할까요? => 오픈 API - 주소기반산업지원서비스를 참고해서 일단 필요할 것 같은 데이터들을 구성해보았습니다.)
  2. Test 메서드명은 공식적인 Convention 이 있을까요? 아니면 개인적인 기준을 세워 작성하면 되는걸까요?
  3. 주석은 보통 어떻게 작성하시는지 좋은 레퍼런스 추천해주실 수 있으실까요?
  4. 멘토님께서는 Java 코드 formatter를 어떤걸 쓰고 계시나요? GoogleStyle formatter 를 쓰고 있는데 개인적으로 안이쁘게 포멧팅이 되는 것 같아서요 😂

@sssukho sssukho added the enhancement New feature or request label Nov 19, 2024
@sssukho sssukho self-assigned this Nov 19, 2024
@sssukho sssukho linked an issue Nov 19, 2024 that may be closed by this pull request
build.gradle Outdated
@@ -41,4 +41,8 @@ subprojects {
test {
useJUnitPlatform()
}

bootJar {
Copy link
Collaborator

Choose a reason for hiding this comment

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

내일 이 얘기를 할 때가 된 것 같군요!! :-)

@f-lab-lyan
Copy link
Collaborator

f-lab-lyan commented Nov 21, 2024

api 모둘과 dto 모듈을 나눈 이유에 대해서


implementation 'org.apache.commons:commons-lang3:3.14.0'
Copy link
Collaborator

Choose a reason for hiding this comment

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

요건 뭐 .. 특별히 안된다고 보기는 어렵지만, 스프링에도 비슷한 기능을 하는 클래스들이 있지 않을까요? ㅎㅎ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

spring-core 에도 비슷한 역할을 클래스가 있기는 한데, apache의 라이브러리를 계속 사용해왔던지라 사용하게 되었습니다.

혹시 관련해서 비추천 하는 이유가 있으실까요?

@sssukho
Copy link
Collaborator Author

sssukho commented Nov 24, 2024

7주차 리뷰에서 말씀해주신 내용은 크게 다음과 같습니다.

  1. 모듈 구성에 명확한 의도가 드러나지 않는다.
  2. 컨트롤러 테스트 코드에서 어떤 점을 테스트하려고 했는지 불분명하다.

관련해서 다음과 같은 방향으로 수정했습니다.

1. 모듈 구성에 명확한 의도가 드러나지 않는다.

각 모듈에 대한 목적은 다음과 같습니다.

  • bp-app-api: 하위 모듈들(bp-domain-rdb, bp-kafka-xxx, bp-utils)을 조립하여 api 서버 역할
  • bp-app-batch: 배치 작업이나 스케줄러 등을 실행하는 배치 서버 역할
  • bp-domain-rdb: 모든 도메인들의 entity가 위치하고, db와 통신만 담당
  • bp-kafka-event-consumer, bp-kafka-event-publisher: 카프카 publisher, consumer 역할
  • bp-utils: util 성 로직 처리 역할

삭제한 모듈의 목적과 그 이유는 다음과 같습니다.

  • bp-dto: dto 클래스들만 모여있는 모듈이었으나 기본 4개 도메인에 대한 dto(샵, 예약, 회원, 리뷰) + 차후 추가될 dto 들이 몇 가지가 있어 초기 단계에서 불필요하게 크게 나눌 필요가 없다고 생각하여 삭제하였습니다.
  • bp-core-web: 초기 웹 설정 등을 담당할 목적으로 두었으나 아직 그렇게까지 프로젝트 사이즈가 크지 않아 삭제하였습니다.

2. 컨트롤러 테스트 코드에서 어떤 점을 테스트하려고 했는지 불분명하다.

최초 테스트 코드 작성시 input 과 output 에 대한 테스트 즉, API 스펙에 따른 요청/응답 메시지의 형태가 정확히 구현이 되었는지에 대한 테스트를 하는 코드를 작성했으나 의도가 불분명해진 것 같습니다.

따라서 다음 클래스들을 통해서 그 의도를 명확하고자 하였으며, 그 외 테스트 코드들은 부가적인(주로 validation) 테스트를 진행한 코드라고 보시면 될 것 같습니다.

  • ShopIntegrationTest: Shop 관련 API 들에 대한 통합 테스트들이 위치해있으며, 추후 구현해야 할 API 들도 추가할 예정입니다.
  • ReviewIntegrationTest: Review 관련 API 들에 대한 통합 테스트들이 위치해있으며, 추후 구현해야 할 API 들도 추가할 예정입니다.


ErrorResponseMessage errorResponseMessage = createErrorResponseMessage(
MSG_FORMAT_PARAMETER_INVALID, exception.getParameterName(), ErrorCode.BR001);
log.error("", exception);
Copy link
Collaborator

Choose a reason for hiding this comment

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

메세지에 뭘 넣을 수 있을지에 대해서도 추후에 고민해보시면 좋을 것 같네요.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

기본적인 메시지는 ErrorCode 라는 enum 에 정의를 해두었습니다만, 말씀주신대로 어떤걸 더 넣을 수 있을지는 추후에 고민해보도록 하겠습니다!


public class CommonTestFixture {

public static ObjectMapper OBJECT_MAPPER = new ObjectMapper().registerModule(new JavaTimeModule());
Copy link
Collaborator

Choose a reason for hiding this comment

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

이건 JavaTimeModule 때문에 넣으신건가요? 그렇다면, 이건 Test범위에서만 필요한 것인가요? 아니면, 모든 비지니스에서 필요한건아닌가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

모든 비즈니스에서 필요할 것 같긴한데, 아직 실제로 사용되지는 않아서 테스트 코드에서만 사용할 수 있도록 위와 같이 작성하였습니다. 추후 비즈니스에서 실제로 사용할 때 테스트 코드도 비즈니스에서 사용하는 object mapper bean 으로 가져오도록 수정할 예정입니다. 😎


@SpringBootTest
@AutoConfigureMockMvc
@Tag("integration-test")
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 Tag는 나중에 어떤 역할을 하게 되나요? (아직 전부다 보지 않아서 ㅜㅠ)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

빌드시 통합 테스트는 테스트에서 제외하기 위해 Tag를 사용하였습니다.
현재도 bp-app-api 모듈의 build.gradle 에 적용해 두었습니다!

@RequestParam(name = "sort", required = false, defaultValue = "registeredDate") final String sort,
@RequestParam(name = "page", required = false, defaultValue = "0") final int page,
@RequestParam(name = "count", required = false, defaultValue = "10") final int count,
@RequestParam(name = "order", required = false, defaultValue = "asc") final String order)
Copy link
Collaborator

Choose a reason for hiding this comment

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

asc, desc 말고, lastest, oldest 같은 것들이 좀 더 의미를 잘 전달하지 않을까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

항상 registeredDate(등록일자) 기준으로 정렬하는건 아닙니다. rate(평점) 순으로 정렬할 수도 있고, 추후에 정렬이 될 수 있는 기준이 추가될 수 있기 때문에 asc, desc 를 사용하였습니다.

}

private void validateCount(Integer count) {
if (count > 100 || count < 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

만약에 validation로직이 복잡해 진다면?

@Autowired
private MockMvc mockMvc;

@MockBean
Copy link
Collaborator

Choose a reason for hiding this comment

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

ReviewService를 구현했는데, 굳이 Mocking할 필요가 있을까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

맞습니다. 이 부분은 다음 PR 떄 수정하도록 하겠습니다. 🙏

Copy link
Collaborator

@f-lab-lyan f-lab-lyan left a comment

Choose a reason for hiding this comment

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

몇 가지 이야기거리들이 있지만, 대체로 좋은 것 같습니다! :-) 고생하셨습니다.

@sssukho sssukho merged commit adb5b83 into main Nov 27, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Controller 코드 작성 및 테스트 코드 작성
2 participants