-
Notifications
You must be signed in to change notification settings - Fork 0
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
[#7] Service 및 테스트 코드 작성 #8
Conversation
- 생성자 주입 방식이기 때문에 멤버 변수에 final 필요
private static final String STORAGE_IMPLEMENTATION_FILE_SYSTEM = "FILESYSTEM"; | ||
private static final String STORAGE_IMPLEMENTATION_S3 = "S3"; | ||
|
||
@Value("${storage.implementation:null}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추후 서비스 프로덕션/테스트 코드 리팩토링 PR에 반영하겠습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추후 �API 스펙 변경 PR에 반영하도록 하겠습니다!
private final StorageService storageService; | ||
private final FacilityRepository facilityRepository; | ||
private final OperationRepository operationRepository; | ||
private final CategoryRepository categoryRepository; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private final StorageService storageService; | |
private final FacilityRepository facilityRepository; | |
private final OperationRepository operationRepository; | |
private final CategoryRepository categoryRepository; | |
private final StorageService storageService; | |
private final FacilityRepository facilityRepository; | |
private final OperationRepository operationRepository; | |
private final CategoryRepository categoryRepository; |
ShopService에서 다른 리포지토리들을 보는 것에 대해서
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
서비스를 만들어서 분리하는 것으로
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 내용은 8주차 화상 멘토링에서 말씀드렸다시피 아직 구현하지 않아 위와 같이 작성했었습니다.
추후 서비스 프로덕션/테스트 코드 리팩토링 PR에 반영하겠습니다!
import org.springframework.web.multipart.MultipartFile; | ||
|
||
@Slf4j | ||
public class FileSystemStorageService implements StorageService { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 코드는 좋은데, 일단 가장 큰 문제는 단일 머신에서밖에 쓸 수 없다는 문제가 있긴하겠죠? ㅎㅎ 만약 테스트에서 사용하려고 하신다면, 코드는 test쪽으로 옮겨두시는 것도 좋은 판단일 것 같습니다. (물론 mocking을 하신다면, 그 쪽을 더 추천하긴 합니다. 😁 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트 코드를 위한 prdocut 코드의 변경이 없도록 해당 코드 삭제하여 서비스 프로덕션/테스트 코드 리팩토링 PR에 반영하겠습니다!
@@ -23,8 +32,14 @@ public ResponseMessage findReviewList(final FindReviewListRequestParameters para | |||
createFindReviewListDummySuccessResponseBody()); | |||
} | |||
|
|||
@Transactional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이게 필요한 이유는 혹시 delete가 실패할까봐 이신거죠?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
의도한 바는 말씀하신 부분이 맞습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그렇군요. 그런 의도라면, 괜찮은 것 같습니다. :-)
public ResponseMessage registerShop(final ImageFiles images, | ||
final ShopRegistrationRequest shopRegistrationRequest) { | ||
|
||
return ResponseMessage.createResponseMessage(new ShopRegistrationResult("732e934")); | ||
// TODO: ShopRegistrationRequest 내 IdName 제거하고 id 값만 string 으로 받을 수 있게 수정 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
List<Operation> operations = operationRepository.findByIdIn(operationIds); | ||
List<Facility> facilities = facilityRepository.findByIdIn(facilityIds); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 validation을 해야하는게 아니라면, 그냥 id를 믿고 넣을 수 있으면 좋을텐데 말이죠...
Page<ShopListFindResult> page = shopRepository.findAll(pageable) | ||
.map(foundShop -> | ||
ShopListFindResult.createShopListFindResult(foundShop, | ||
new String(Base64.getEncoder().encode(storageService.loadThumbnail( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 ... 섬네일을 base64로 내려줄 생각이셨던 거군요!!!!!! 두둥!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
presignedurl 에 대한 내용을 전혀 알지 못한 상태에서 API 스펙을 작성해버려서 생각해낸게 요정도였습니다 😂
추후 Shop API 스펙 변경 PR 에서 수정하여 반영하도록 하겠습니다!
void given_e() throws Exception { | ||
|
||
} | ||
// @Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
필요없는 코드는 지우는게 어떨까요? 😄
@@ -84,10 +110,12 @@ void given_shopRegistrationRequest_when_failed_then_getErrorResponseMessage() th | |||
|
|||
@Test | |||
@DisplayName("Shop 리스트 조회 요청 성공시 value 가 JSON Array 인 ResponseMessage 객체 응답을 받는다.") | |||
@Transactional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트 코드에 @transactional을 붙여서 얻을 수 있는 이득은 어떤 것이 있나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 기존에 인지하고 있던 이점은 테스트 코드에서 @Trasnactional 을 사용할 경우 'DB에 commit 된 내용들을 rollback 시켜줄 수 있다.' 였습니다.
근데 코멘트 달아주신 내용 보고 다시 한번 찾아보았더니, 아래와 같은 상황에서 production 코드에서 예상치 못한 문제들이 발생하는 사례들이 있는 것을 확인했습니다.
- 테스트 코드에만 @transactional 어노테이션이 존재하고, production 코드에는 @transactional 어노테이션이 붙지 않은 경우
- production 코드에는 @transactional (readOnly=true) 로, 테스트 코드에는 @Transactionl 로 작성된 경우
- 테스트 코드에서 @transactional 에 적용된 정책(=rollback 이 기본이기 때문에 변경 감지(dirty checking)가 정상적으로 동작하지 않는)으로 인한 예상치 못한 문제 발생
테스트 코드는 기본적으로 최대한 production 환경을 구성하여 작성해야 한다고 생각하기 때문에 앞으로 테스트 코드에서의 @transactional 사용은 최대한 지양하는 방향으로 작성해야 할 것 같습니다.
관련된 수정은 차후 서비스 프로덕션/테스트 코드 리팩토링 PR 에서 진행하도록 하겠습니다!
@DisplayName("Review 삭제 처리시 이상없이 메서드가 종료된다.") | ||
void given_deleteReview_when_succeed_then_nothing() throws Exception { | ||
// given | ||
final String deleteTargetId = ReviewTestFixture.MOCKED_VALID_REVIEW_ENTITIES[0].getId(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요기를 위해서 Fixture를 사용하셨다면, 이정도는 그냥 hardcoded해도 되지 않았을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 맞습니다. 해당 내용은 추후 PR 에 반영하도록 하겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 고생하셨어요! 추후 작업은 다른 PR로 하시면 좋을 것 같아요! 😄
ShopService 일부 테스트 코드 작성하였으며, 추후에 계속해서 다른 기능들의 테스트 코드도 작성해 나갈 예정입니다 😎
file changed 가 좀 많은데, ShopService, ShopServiceTest 클래스 위주로 리뷰 부탁드리겠습니다. 🙏
이외에 아래와 같이 추가로 궁금한 부분이 있습니다.