[럿고] 점진적인 리팩토링 미션 1단계 제출합니다. #8
Conversation
README.md
Outdated
|
|
||
| ### 메뉴 | ||
| - 메뉴를 등록할 수 있다. | ||
| - 메뉴의 이름과 가격, 메뉴 그룹과 메뉴 상품 목록을 이볅해 등록할 수 있다. |
README.md
Outdated
| - 가격은 null과 0보다 작을 수 없다. | ||
| - 등록되지 않은 메뉴 그룹을 입력할 수 없다. | ||
| - 메뉴는 특정 메뉴 그룹에 속해있어야 한다. | ||
| - 메뉴에 속한 상품의 합이 메뉴의 가격보다 클 수 없다. |
There was a problem hiding this comment.
1단계 문서에 적혀있는 내용으로는 아래와 같은데 저도 좀더 분석해봐야 할 것 같네요. 🤔
메뉴에 속한 상품 금액의 합은 메뉴의 가격보다 크거나 같아야 한다.
There was a problem hiding this comment.
MenuService Line 58
if (price.compareTo(sum) > 0) {
throw new IllegalArgumentException();
}🙄
There was a problem hiding this comment.
menu를 세트라고 생각하고 menu_product를 단품이라고 생각했어야 했네요...ㅎㅎㅎ
잘못된 도메인 분석이였습니다 ㅠㅠ
감사합니다!
| 모든 Business Object에 대한 테스트 코드를 작성한다. | ||
| @SpringBootTest를 이용한 통합 테스트 코드 또는 @ExtendWith(MockitoExtension.class)를 이용한 단위 테스트 코드를 작성한다. | ||
|
|
||
| ## 치킨집 요구 사항 |
There was a problem hiding this comment.
요구사항을 좀더 세세하게 분석하셨네요. 👍
src/main/resources/db/migration 디렉터리의 .sql 파일을 보고 어떤 관계로 이루어져 있는지 참고한다.
order_line_item, menu_product도 분석해서 정리하면 좋을 것 같아요.
There was a problem hiding this comment.
이 부분을 넣지 않은 이유가 실제로 도메인은 존재하지만, 맨 아래 용어 사전에서 충분히 설명이 된다고 생각하기 때문에 따로 추가하지 않았습니다. 아울러 menu에 menu_product에 대한 예외 부분들이 충분히 설명이 되어 있다고 생각이 듭니다.
혹시 어떻게 생각하시는지 궁금합니다~
There was a problem hiding this comment.
👌 우선은 말씀처럼 충분할 것 같네요. 다음 단계들을 진행하면서 menu_pdocut에 대해서도 별도로 정리할 필요가 있다고 판단되면 그때 추가하도록 하죠.
| final MenuGroup menuGroup = new MenuGroup(); | ||
| menuGroup.setName("한마리 메뉴"); |
There was a problem hiding this comment.
우선 final을 많이 사용해주셨는데 어떤 이유로 사용했는지가 궁금해요. (단순하게는 기존 Service 코드에서 사용한 형태를 따랐다가 될 수도 있을 것 같아요.)
utils.TestFixture.getMenuGroup()를 사용할 수 있을 것 같네요.
There was a problem hiding this comment.
이번 미션에서는 프로덕션 코드와 통일성을 위해 사용했습니다.
final을 사용하는 이유를 생각해본다면 재할당을 막기 위해 불변성 보장이 있을 것 같습니다.
재할당을 막으면서 지역변수에 대해서도 불변성을 유지하려고 했습니다.
아울러 프로덕션 코드를 작성하신 코치님에게도 물어봤지만, 취향이라는 답변과 다른 글들을 보니 지역 변수에 대해 취향이라는 이유가 많았습니다~ㅎㅎ
저의 개인적인 생각에서는 final을 지역변수에 붙혀 단점이 없다면 충분히 고려해볼수 있다고 생각합니다.(가독성 측면에 대한 이야기가 나오기도 하지만, final로 인해 가독성이 떨어진다고 생각하지 안습니다. 단순히 코드가 길다고 가독성이 떨어지지 않는것과 똑같은 이유일것 같습니다!)
There was a problem hiding this comment.
넵, 말씀처럼 취향차이로 생각됩니다.(취향이 확고하신 분들은 취향으로 안보시기도 하지만.... 😱 )
개인적인 선호로는 지역 변수내에서는 final을 붙이지 않는 편입니다.
사실 이 위치에서 문의드린 이유가 있는데
final MenuGroup menuGroup = new MenuGroup();
menuGroup.setName("한마리 메뉴");final이라 menuGroup의 참조는 바뀌지 않지만 menuGroup의 name이 바뀌면서 menuGroup이 불변하지 못합니다.
현재로서는 final 키워드가 붙은 지역변수가 불변하다는걸 나타내는게 아닌 참조가 바뀌지 않는다는 것 밖엔 나타내지 못한다고 봐요. 그런 면에서 final 키워드만 보고 불변한 변수라고 오해하는 사람이 생길 수도 있다는 점에서도 저는 지역 변수 내 final 사용은 지양합니다. (사용을 안하는 편이다 보니 지역 변수내에서는 불필요한 키워드로 보여 필요없이 코드가 길어지는 느낌도 드는게 사실입니다.) 당장 지역 변수에서 사용한 기억은 안나는데 그래서 사용해도 저같은 경우는 기본 타입에만 사용하는 편입니다.
사실 코틀린의 var, val 처럼 언어 레벨에서 손쉽게(?) 사용하게 제공해준다면 고민할 필요는 없다고 봐요. 이런 점에서 자바가... 😢
저도 제이슨(아마도?)처럼 취향차이로 생각하기 때문에 사용하지 않는 사람의 이유도 알아두면 좋을 것 같아서 길게 적어 봤어요ㅎㅎ
| ); | ||
| } | ||
|
|
||
| @DisplayName("create: 상품 등록 확인 테스트") |
| @DisplayName("list: 전체 주문을 확인하는 테스트") | ||
| @Test | ||
| void listTest() { | ||
| final OrderLineItem orderLineItem = TestFixture.getOrderLineItem(); | ||
| final Order order = TestFixture.getOrderWithCooking(orderLineItem, 1L); | ||
| when(orderDao.findAll()).thenReturn(Collections.singletonList(order)); | ||
|
|
||
| final List<Order> orders = orderService.list(); | ||
|
|
||
| assertAll( | ||
| () -> assertThat(orders).hasSize(1), | ||
| () -> assertThat(orders.get(0)).isEqualTo(order) | ||
| ); | ||
| } |
There was a problem hiding this comment.
전체 주문이라고 하기엔 하나만 확인하는건 테스트가 너무 빈약한게 아닐까요?
취향차 일 수도 있겠지만
() -> assertThat(orders.contains(order)).isTrue()위와 같이도 작성 가능할 것 같네요. list의 사이즈가 커져도 순서를 고려해서 get하지 않아도 되겠네요.
| .isInstanceOf(IllegalArgumentException.class); | ||
| } | ||
|
|
||
| // TODO: 2020/10/05 이런 경우에는 테스트를 어떻게 하는지 물어보기 |
There was a problem hiding this comment.
사실 Mock을 잘 사용하지 않아요. Mock을 사용할 때는 내 서비스가 아닌 외부 서비스에 대해서 mocking을 하는 편입니다.
예를 들면 관리자 권한을 가진 사람만 게시글을 삭제 할 수 있을 때, 내가 담당하는 서비스는 게시판까지만이고 인증/인가를 담당하는 서비스는 별도로 존재한다면 게시글을 지울 때마다 권한이 있는 사용자인지 인증 서버에 확인 요청을 해야 할겁니다.(사용자 정보를 캐싱 등을 하지 않는다고 전제하고)
예를 들면 관리자 권한을 가진 사람만 게시글을 삭제 할 수 있을 때, 게시판을 관리하는 서비스, 인증 인가를 관리하는 서비스가 별도로 존재 할 때 게시글을 지울 때 마다 게시판 서비스에서는 외부 서비스인 인증 서비스에 매번 요청을 해서 사용자의 권한을 확인해야 합니다.
(사용자 정보를 캐싱등 별도 저장하지 않는다는 전제, 별도 서비스는 외부 api 서버를 말하는 겁니다.)
여기서 외부 서비스를 mock 처리하지 않으면 인증 서버가 장애가 발생하면 테스트 하고자 했던 게시글 삭제 로직과는 별개로 실패를 하게 됩니다. 독립적이여 할 테스트가 외부 서비스의 상태에 종립적이게 됩니다. 그렇기 때문에 테스트와 무관해야 할 외부 환경에 대한 종속성을 끊기 위해서 주로 외부 서비스들에 한해서만 Mock을 사용해요. 외부 서비스에 대해서 Mock 처리하면 다른 장점도 있는데 외부 환경이 장애가 났다던지 특정 응답을 했을 때에 대한 테스트도 손 쉽게 할 수 있답니다. 그게 아니라면 상대 서버가 장애 났을 때만 테스트를 한다던지 장애 상황을 만들어 달라고 요청해야겠죠? 😱
Mock을 사용하는 대신 h2를 사용하여 테스트합니다.
Mock에 대해서는 의견이 조금 나뉠 수 있을텐데 저 같은 경우는 모의객체는 스텁이 아니다(Mocks Aren't Stubs)에서 나오는
모의객체를 이용한 테스트(Tests with Mock Objects)
모의객체의 경우에는 행위검증을 사용한다. 여기서는 order가 warehouse에 대해 올바르게 호출했는지를 살펴보는 것을 의미한다.
이 이유로 Mock 사용은 지양하는 편입니다. 어떻게 테스트 할 지 고민 되신 이유도 테이블 그룹이 해제된 상태인지 확인을 하고 싶은데 상태를 검증하지 않기 때문이죠.
사실 h2도 문제가 없는건 아니라서 mybatis를 사용하는 환경에서 필요에 의해 db 방언을 사용했을 때 h2에서는 호환이 안돼서 문제가 되거나 그외에도 서로 다른 db로 인해 테스트가 실패하거나, 테스트는 성공했지만 운영 환경에서 문제가 될 수도 있습니다.(애초에 쿼리에 로직을 넣지 말고 쿼리는 단순하게 만들면 그럴 일은 줄어듭니다.) 그래서 이런 문제가 우려되는 곳에서는 로컬 환경에서의 테스트도 도커를 사용하여 운영 환경에서와 db와 동일한 db를 띄어서 사용하는 경우도 있습니다.
Mock을 사용하지 않았을 때의 단점 중 하나가 db crud로 인해 테스트 속도가 느려지고, 각각 독립적이여 할 테스트들이 db 데이터로 인해서 순서가 고려되야 할 수도 있습니다. 그래서 순서를 고려할 필요 없게 각 테스트마다 데이터 정리가 필요할 수도 있습니다. (전체 삭제 테스트 후 조회 테스트를 하면 실패하겠죠?)
저도 지양을 하는거지 Mock을 사용하면 안된다는 아니고, 여러 의견이 있다는건 어느게 틀리다/맞다가 정답이 없기 때문에 럿고만의 방향을 고민을 해보면 좋을 것 같아요. 당장 어떤 방향을 정하기보다는 우선 마음에 들거나, 더 편한 방법을 사용해보면서 차근 차근 경험을 쌓아가면서 고민해보는 것도 좋은 방법이라고 생각합니다. (저도 아직 고민 중인 것들이 많네요. 보면 시니어 분들도 비슷한 고민들을 다 가지고 계시더군요. 😋)
There was a problem hiding this comment.
너무 감사합니다~ 추가적인 질문은 DM으로 여쭤봣습니다! 시간 나시면 답변 부탁드릴께요 ㅎㅎ
| 모든 Business Object에 대한 테스트 코드를 작성한다. | ||
| @SpringBootTest를 이용한 통합 테스트 코드 또는 @ExtendWith(MockitoExtension.class)를 이용한 단위 테스트 코드를 작성한다. | ||
|
|
||
| ## 치킨집 요구 사항 |
There was a problem hiding this comment.
👌 우선은 말씀처럼 충분할 것 같네요. 다음 단계들을 진행하면서 menu_pdocut에 대해서도 별도로 정리할 필요가 있다고 판단되면 그때 추가하도록 하죠.
| final MenuGroup menuGroup = new MenuGroup(); | ||
| menuGroup.setName("한마리 메뉴"); |
There was a problem hiding this comment.
넵, 말씀처럼 취향차이로 생각됩니다.(취향이 확고하신 분들은 취향으로 안보시기도 하지만.... 😱 )
개인적인 선호로는 지역 변수내에서는 final을 붙이지 않는 편입니다.
사실 이 위치에서 문의드린 이유가 있는데
final MenuGroup menuGroup = new MenuGroup();
menuGroup.setName("한마리 메뉴");final이라 menuGroup의 참조는 바뀌지 않지만 menuGroup의 name이 바뀌면서 menuGroup이 불변하지 못합니다.
현재로서는 final 키워드가 붙은 지역변수가 불변하다는걸 나타내는게 아닌 참조가 바뀌지 않는다는 것 밖엔 나타내지 못한다고 봐요. 그런 면에서 final 키워드만 보고 불변한 변수라고 오해하는 사람이 생길 수도 있다는 점에서도 저는 지역 변수 내 final 사용은 지양합니다. (사용을 안하는 편이다 보니 지역 변수내에서는 불필요한 키워드로 보여 필요없이 코드가 길어지는 느낌도 드는게 사실입니다.) 당장 지역 변수에서 사용한 기억은 안나는데 그래서 사용해도 저같은 경우는 기본 타입에만 사용하는 편입니다.
사실 코틀린의 var, val 처럼 언어 레벨에서 손쉽게(?) 사용하게 제공해준다면 고민할 필요는 없다고 봐요. 이런 점에서 자바가... 😢
저도 제이슨(아마도?)처럼 취향차이로 생각하기 때문에 사용하지 않는 사람의 이유도 알아두면 좋을 것 같아서 길게 적어 봤어요ㅎㅎ
|
|
||
| assertAll( | ||
| () -> assertThat(menuGroups).hasSize(2), | ||
| () -> assertThat(menuGroups.contains(oneMenuGroup)).isTrue() |
There was a problem hiding this comment.
이미 알고 계실 수도 있겠지만 Map에 있는 getOrDefault도 상황에 따라서는 굉장히 많은 도움이 된답니다.
그래서 생각날 때마다 한번 씩 Collection이나 Optional에 어떤 메서드들이 있는지 봐두면 좋답니다.
참고로 자바 9 이상을 사용하시면 자바8에서는 호환되지 않는 기능이 있을 수 있으니 이점은 조심하셔야 해요.
JAVA 9: UPDATES TO OPTIONAL
안녕하세요 휴! 온보딩때 만났던게 어제같은데 벌써 마지막 미션에서 다시 만나게 되네요~ㅎㅎ
이번에도 잘 부탁드리겠습니다 ㅎㅎ
1단계 요구사항이 프로덕션 코드를 변경하지 않고 테스트 코드를 작성하는 것이 목적이라 생각하여 프로덕션 코드는 전혀 건들지 않고 진행했습니다.
또한 저는 controller 테스트를 통합 테스트 느낌으로 진행을 했고, service 테스트는 단위 테스트로 Mock을 사용했습니다.
그러나, Mock을 사용하다 보니 void에 대한 테스트가 너무 힘들더라고요....ㅠㅠ 그래서 하다보니, Fake가 더 나은 방식이였다고 생각하게 되었습니다!
혹시 휴는 지금과 같은 요구사항이였다면 어떻게 테스트 코드를 짜는 걸 선호하시는지 궁금합니다~ㅎㅎㅎㅎ