-
Notifications
You must be signed in to change notification settings - Fork 302
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
[MVC 구현하기 - 2단계] 무민(박무현) 미션 제출합니다. #440
Conversation
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.
안녕하세요 무민
2단계 구현하신 것 잘 봤습니다 ㅎㅎ
꼼꼼한 테스트와 직관적인 코드가 너무 인상적이네요 💯
보면서 궁금했던 것들 질문으로 남겼습니다!!
저번 1단계 리뷰에 추가적으로 답변 달아놨으니 확인해주세요 🫡
mvc/src/main/java/webmvc/org/springframework/web/servlet/ModelAndView.java
Show resolved
Hide resolved
app/src/test/java/com/techcourse/controller/mvc/MvcLoginControllerTest.java
Outdated
Show resolved
Hide resolved
mvc/src/main/java/webmvc/org/springframework/web/servlet/mvc/tobe/AnnotationHandlerMapping.java
Show resolved
Hide resolved
assertThat(handlerExecution.getMethod()).isEqualTo(handlerExecution.getMethod()).isEqualTo(viewIndex); | ||
} |
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.
assertThat(handlerExecution.getMethod()).isEqualTo(handlerExecution.getMethod())
?! 해당 equalTo는 빼도 좋지 않을까요?ㅎㅎ
해당 테스트는 handlerMapping에 대한 테스트인 것 같은데 분리후 대표값 하나만 테스트 해보는 것은 어떨까요??
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.
endpoint까지 제대로 된 api 테스트와 동일한 것처럼 최대한 흉내내기 위해 위와 같이 controller테스트를 진행했습니다. 만약 restassured나 endpoint 테스트 할 수있는 라이브러리가 있었다면 말씀하신 테스트는 대표값 한개로만 했을거 같습니다!
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.
앗 질문의 의미를 조금 더 첨언하자면!!
- 이게 assertThat(true).isEqualTo(true)랑 같은 것 아닌가요?
- getHandler 메서드는 handlerMapping 테스트로 분리하고 handlerMapping 테스트에서 다루면 어떨까 의견을 제시해본 것 이였습니다 ㅎㅎ
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.
- 엇 똑같은 부분이 두 개가 들어간 건 이제 봤네요 ㅋㅋㅋㅋㅋ 수정하겠습니다!
- 말씀하신 부분은 이해했습니다! 실제 프로젝트에서는 그렇게 할거 같은데 지금 부분에서는 위에서도 말했듯이 의도한바로 테스트를 분리하면 단위 테스트가 될 것 같아 E2E를 나타내기 위해 지금 같이 위치해놨씁니다.
피드백 반영하였고 코멘트 달았습니다! |
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.
안녕하세요 무민~!
리뷰에 답변 주신 것과 변경된 부분 확인했습니다 👍
요구사항은 모두 만족하신 것 같아 3단계 진행해주시면 될 것 같습니다 ㅎㅎ
안녕하세요 푸남회장님~
이번 2단계도 잘 부탁드립니다. 🙇🏻♂️
이번에 흐름은 다음과 같습니다.
More
DispatcherServlet 클래스가 관련된 것이 많아서 테스트가 어렵네요..ㅜ 혹시 더 좋은 방법이 있으면 알려주세요!
dependency가 없어 제대로 된 api 테스트가 가능한지 모르겠어서 현재 다음과 같은 방법으로 하고 있습니다.
패키지를 건들려고 했는데 3단계에 다음과 같은 말이 있어서 건들면 안될 것 같더라구요. 그래서 패키지는 3단계 때 다듬겠습니다.