[MVC 구현하기 - 2단계] 조조(조은별) 미션 제출합니다. by eun-byeol · Pull Request #763 · woowacourse/java-mvc

eun-byeol · 2024-09-26T16:22:35Z

초롱! 안녕하세요~
데모데이 일정으로 구현이 조금 늦어졌네요..!
이번에도 잘 부탁드립니다🙇‍♀️

추가 리뷰 반영 사항

현재 TestController의 모든 메소드에 @RequestMapping이 붙어있어 정상적으로 동작하지만, 만약 @RequestMapping이 없는 메소드가 존재한다면 어떻게 될까요??

리플렉션으로 모든 메서드를 가져온 후, @RequestMapping이 있는 메서드만 로직을 수행하도록 filter 조건을 걸었습니다.

step2 테스트 관련 로직

mockito의 ArgumentCaptor 클래스를 사용하면, 메소드 호출 시에 인자값을 확인할 수 있어요.
DispatcherServletTest 중 일부 코드입니다.

    ArgumentCaptor<String> viewNameCaptor = ArgumentCaptor.forClass(String.class); // 인자 타입 지정

    dispatcherServlet.service(request, response); // 메서드 호출
    
    verify(request).getRequestDispatcher(viewNameCaptor.capture()); // request.getRequestDispatcher() 호출 시 인자 가져오기
    String actual = viewNameCaptor.getValue(); // 인자값 반환
    
    assertThat(actual).isEqualTo("/register.jsp");

request.getRequestDispatcher() 인자를 가져오는 이유는 JspView의 render 메서드 중 viewName을 인자로 사용하고 있기 때문입니다.

    RequestDispatcher requestDispatcher = request.getRequestDispatcher(viewName); // viewName을 인자로 넘기는 부분
    requestDispatcher.forward(request, response);

eun-byeol · 2024-09-29T03:06:39Z

초롱! 안녕하세요~
ControllerScanner, HandlerMapping, HandlerAdapter 추가하여 다시 리뷰요청드려요!

💬 한 가지 질문이 있어요!
Q. 테스트 코드가 해당 패키지가 아닌, 다른 패키지에 있는데 괜찮을까요?
mvc 디렉토리(com.interface21.webmvc.servlet.mvc.tobe)에 기능구현을 했는데,
테스트 시에 app 디렉토리(com.techcourse.controller)가 필요했어요. 그래서 HandlerMapping, HandlerAdapter 관련 테스트를 app 디렉토리에 두게 되었습니다.

그렇지 않으면 TestController처럼 테스트를 위한 컨트롤러들을 mvc 디렉토리 하위에 만들어야 했어요. 굳이 필요하지 않는 코드들이 늘어나서 저는 app에 테스트 코드를 작성했는데, 이에 대해 어떻게 생각하시나요?
혹은 mvc에 테스트를 할 수 있는 더 좋은 방법이 있을까요?

HoeSeong123

안녕하세요 조조!
데모데이 준비하느라 많이 바쁘셨을텐데도 2단계 구현을 정말 잘해주셨네요!!
테스트 코드도 꼼꼼하게 잘 작성해주신 것 같고요ㅎㅎ
정말 고생 많으셨습니다.

간단한 코멘트 몇개와 질문 몇가지, 그리고 조조가 질문해주신 부분에 대한 제 생각도 남겨두었습니다.
모든 코멘트에 대해서는 온전히 저의 주관적인 생각일 뿐이므로 편하게 읽어봐주시면 감사드리겠습니다ㅎㅎ
이해가 잘 되지 않는 부분은 편하게 코멘트나 디엠으로 연락주세요!!
얼마 남지 않았지만 화이팅입니다ㅎㅎ

HoeSeong123 · 2024-09-29T07:26:11Z

        manualHandlerMapping.initialize();
+        addHandlerMapping(manualHandlerMapping);
+
+        AnnotationHandlerMapping annotationHandlerMapping = new AnnotationHandlerMapping("com.techcourse.controller");


이렇게 하면 컨트롤러를 만들 때 컨트롤러 패키지에 넣는 것이 강제될 것 같습니다.
또한 techcourse 패키지 명이 변경되거나 한다면 관리하기 힘들 수도 있을 것 같네요.
패키지를 동적으로 구해올 수 있는 방법은 없을까요??

(반영 완료)this.getClass().getPackage().getName()로 DispatcherServlet 패키지를 가져오도록 했습니다.

다만 조금 걸리는 부분은,

기존 basePackage는 com.techcourse.controller인데, 해당 방식은 com.techcourse을 가져옵니다. 프로그램 돌아가는데 문제는 없지만, 리팩토링으로 볼 순 없을 것 같아요.

항상 DispatcherServlet과 같은 패키지에 basePackage를 둬야 합니다. 리뷰 기다리면서 step3번 요구사항을 봤었는데, DispatcherServlet를 결국 mvc로 옮기는 요구사항이 있었어요. 이 경우에는 techcourse 패키지의 컨트롤러들을 init하지 못해요.

물론, 지금은 말씀드린 부분보다는 초롱의 리뷰에 동의해요. 추후 다음 step 구현할때 문제가 되면 수정할 수 있을 것 같아요😊

저도 처음에는 DispatcherServlet 패키지를 가져오도록 했었는데 Application 클래스에서 패키지를 가져오도록 변경하였습니다!!
Application 클래스는 반드시 존재해야 하기도 하고, 컨트롤러는 Application 클래스와 반드시 같은 패키지에 있으니 조조가 말씀하신 부분도 해결할 수 있을 것 같더라구요ㅎㅎ
저도 이번에 3단계에 대해 고민하다가 알게된 사실입니�다😅

HoeSeong123 · 2024-09-29T08:28:58Z

+    @Test
+    void Controller_타입_핸들러이면_true를_반환한다() {
+        Controller supportedHandler = new RegisterViewController();
+
+        assertThat(controllerAdapter.supports(supportedHandler)).isTrue();
+    }


💬 한 가지 질문이 있어요!
Q. 테스트 코드가 해당 패키지가 아닌, 다른 패키지에 있는데 괜찮을까요?
mvc 디렉토리(com.interface21.webmvc.servlet.mvc.tobe)에 기능구현을 했는데,
테스트 시에 app 디렉토리(com.techcourse.controller)가 필요했어요. 그래서 HandlerMapping, HandlerAdapter 관련 테스트를 app 디렉토리에 두게 되었습니다.

우선 어떤 클래스에 대한 테스트는 반드시 같은 패키지 안에 있어야 한다고 생각합니다. 다른 패키지, 다른 모듈에 있어도 동작 자체에 큰 문제는 없겠지만 제가 생각하는 이유는 다음과 같습니다.

첫 번째로 관리와 가독성입니다. 어떤 클래스에 대한 테스트를 찾고 싶을 때 다른 패키지, 심지어 다른 모듈에 있다면 관리하기가 매우 힘들 것입니다.
두 번째로는 패키지 접근 권한입니다. 현재는 문제가 없지만 만약 default 메소드를 사용하게 됐고, 그 메소드에 대한 테스트를 작성해야 한다면 어떻게 해야 할까요?

이런 이유로 클래스와 그에 대한 테스트는 반드시 같은 패키지 안에 있어야 한다고 생각합니다.
그렇다면 해당 테스트처럼 다른 모듈에 대한 의존이 필요한 경우에는 어떻게 테스트해야 할까요?

사실 저도 완벽한 답은 찾지 못했습니다. 많은 고민을 하기는 했지만 어떤 방식이 좋을지는 잘 모르겠더라구요😅
제가 생각한 답은 모킹과 직접 구현입니다.
예를 들면 이 메소드는 다음과 같이 작성할 수 있겠네요

@Test void Controller_타입_핸들러이면_true를_반환한다() { Controller supportedHandler = mock(Controller.class); assertThat(controllerAdapter.supports(supportedHandler)).isTrue(); }

또는 이런 식으로도 작성할 수 있을 것 같습니다.

@Test void Controller_타입_핸들러이면_true를_반환한다() { Controller supportedHandler = new Controller() { @Override public String execute(HttpServletRequest req, HttpServletResponse res) throws Exception { return ""; } }; assertThat(controllerAdapter.supports(supportedHandler)).isTrue(); }

저희가 이 테스트를 통해 알고 싶은건 뭘까요?

RegisterViewController는 지원하고, RegisterController는 지원하지 않는다

이걸 알고 싶은 걸까요? 저는 아니라고 생각합니다.
제가 생각하는 이 테스트를 통해 얻고 싶은 것은

Controller 인터페이스를 상속한 클래스는 지원하고, Controller 어노테이션을 사용한 클래스는 지원하지 않는다.

라고 생각합니다.

그렇게 생각한다면 app 모듈에 있는 클래스들을 사용하지 않더라도 충분히 테스트 코드를 작성할 수 있을 것 같아요. 이 클래스 뿐 아니라 다른 클래스들에 대한 테스트도 마찬가지로요!!

조조의 생각은 어떠신가요??
이 클래스에 대한 테스트는 반드시 app 모듈에 있는 클래스들이 있어야만 테스트가 가능한걸까요?

저도 아직 잘 모르고 100% 주관적인 저의 생각일 뿐이므로 편하게 조조의 의견도 공유해주시면 감사드리겠습니다😁

(반영 완료) 테스트 코드 패키지 이동했고, app 패키지 내 controller 의존하지 않도록 수정했습니다.

어떤 클래스에 대한 테스트는 반드시 같은 패키지 안에 있어야 한다고 생각합니다.

두가지 근거에 매우 공감합니다👍 mock을 사용하는 방식도 좋아요.

반드시 app 모듈에 있는 클래스들이 있어야만 테스트가 가능한걸까요?

반드시 해당 모듈에 있어야한다는 의미는 아니었습니다.
HandlerMappingRegistry 테스트의 경우, 통합 테스트를 의도했어요. 즉, implements Controller한 컨트롤러와 @Controller 붙인 컨트롤러의 handlerMapping을 셋팅하고, 각 핸들러에 맞는 Adapter까지 반환하는 것을 테스트했어요.
그러기 위해서는 구현된 HandlerMapping이 필요했는데, 이번에 추가한 TestManualHandlerMapping 와 같은 테스트 전용 클래스들이 생겨나는 것이 오히려 복잡하다고 생각했어요. 그래서 이미 구현된 app 모듈 클래스들을 활용했었구요.

그러나, 테스트 전용 컨트롤러를 만드는 것보다 말씀해주신 두 가지 근거들이 더 치명적이라고 생각해서 수정했습니다. 좋은 의견 감사합니다👍

eun-byeol · 2024-09-29T14:58:21Z

초롱, 리뷰 반영 후 다시 요청드립니다.
이후 파일 변경사항은 여기에서 확인하시면 편하실 것 같아요!

좋은 질문 많이 주셔서 리팩토링과 학습에 많은 도움이 되었어요! 감사합니다🙇‍♀️
레벨4 바쁘실텐데 남은 일정들도 화이팅입니다!!

HoeSeong123

주말인데도 피드백 반영이 굉장히 빠르시네요👍
변경사항 전부 확인했습니다. 제가 생각했던 방향과 거의 100% 일치해서 더이상 제가 피드백드릴 수 있을만한 부분은 없는 것 같네요😅
다음 미션으로 넘어가도 좋을 것 같습니다!!

주말에 미션하시느라 고생하셨습니다 조조ㅎㅎ
마지막 3단계도 화이팅입니다!!

HoeSeong123 · 2024-09-29T16:52:11Z

+        addHandlerMapping(new ManualHandlerMapping());
+        addHandlerMapping(new AnnotationHandlerMapping(this.getClass().getPackage().getName()));


코드가 굉장히 간결해졌네요👍

HoeSeong123 · 2024-09-29T16:53:35Z


-        Object handler = handlerMappingRegistry.getHandler(request)
-                .orElseThrow(() -> new UnsupportedOperationException("지원하지 않는 요청입니다."));
+        Object handler = handlerMappingRegistry.getHandler(request);


(질문) 정말 단순한 궁금증입니다!
예외처리의 책임을 DispatcherServlet이 아닌 HandlerMappingRegistry에게 주신 이유가 궁금합니다

단순히, DispatcherServlet의 코드를 최대한 간결하게 하고 싶어서 Registry에서 예외처리를 했어요. Optional을 반환해서 꺼내는 로직 처리도 번거로운 작업이라고 생각했어요.
그러나 재사용성을 고려하면 Optional을 반환하는 것이 더 좋을 것 같긴 해요. empty일 때 처리 로직이 있는 경우에는요! 지금은, 그런 코드가 없어서 괜찮다고 생각합니다:)

HoeSeong123 · 2024-09-29T16:55:26Z

+    public Map<Class<?>, Object> getControllers() {
        Set<Class<?>> clazz = reflections.getTypesAnnotatedWith(Controller.class);
-        this.controllers = instantiateControllers(clazz);
+        return Collections.unmodifiableMap(instantiateControllers(clazz));


불변 객체로 반환..
저는 항상 놓치는 부분인데 조조는 매번 꼼꼼하게 잘 챙기시네요👍

eun-byeol added 6 commits September 23, 2024 23:00

feat: @RequestMapping 붙은 메서드만 처리하도록 수정

c56d960

docs: 2단계 기능 목록 추가

1958fba

feat: Legacy MVC와 @MVC 통합

b64c138

chore: 로그 메시지 수정

ccfe67c

test: Legacy MVC와 @MVC 통합 테스트

1e57ac1

feat: 매핑되는 URI가 없는 경우 예외 발생

4723f82

eun-byeol requested a review from HoeSeong123 September 26, 2024 16:22

eun-byeol added 10 commits September 27, 2024 14:49

docs: 2단계 추가 기능 목록 업데이트

16c8723

refactor: ControllerScanner 클래스 추가

51f4418

refactor: HandlerMapping 인터페이스 분리

6d5c764

refactor: HandlerMapping 인터페이스 분리

6eeb4df

test: HandlerMappingRegistry 테스트 추가

d9bacf0

refactor: 디렉토리 구조 변경

9fc9c3d

test: 에러 메시지 검증 추가

95ec2eb

test: HandlerAdapterRegistry 테스트 추가

a5b5886

test: HandlerExecutionAdapter 테스트 추가

38385ed

test: ControllerAdapter 테스트 추가

dda1461

HoeSeong123 requested changes Sep 29, 2024

View reviewed changes

eun-byeol added 10 commits September 29, 2024 20:55

refactor: HandlerMapping 중복 메서드 분리

546ae4e

refactor: base package 동적 생성하도록 수정

850d19b

refactor: HandlerMappingRegistry 에서 지원하지 않는 handler 예외 처리

207f4fb

refactor: Reflections를 필드로 갖도록 수정

485d76c

feat: request URI, method null 검증 로직 추가

733a0a7

feat: request method 검증 로직 추가

4c1ea8c

test: ControllerAdapterTest 패키지 이동 및 mock 사용

aedec51

test: HandlerAdapterRegistryTest 패키지 이동 및 mock 사용

e233f41

test: HandlerExecutionAdapterTest 패키지 이동 및 mock 사용

60d5923

test: HandlerMappingRegistryTest 패키지 이동 및 TestManualHandlerMapping 추가

27ee621

HoeSeong123 approved these changes Sep 29, 2024

View reviewed changes

HoeSeong123 merged commit 7dd8f4b into woowacourse:eun-byeol Sep 29, 2024

eun-byeol mentioned this pull request Sep 30, 2024

[MVC 구현하기 - 3단계] 조조(조은별) 미션 제출합니다. #822

Merged

		addHandlerMapping(new ManualHandlerMapping());
		addHandlerMapping(new AnnotationHandlerMapping(this.getClass().getPackage().getName()));

Conversation

eun-byeol commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

추가 리뷰 반영 사항

step2 테스트 관련 로직

Uh oh!

eun-byeol commented Sep 29, 2024

Uh oh!

HoeSeong123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eun-byeol Sep 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eun-byeol commented Sep 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HoeSeong123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

eun-byeol commented Sep 26, 2024 •

edited

Loading

eun-byeol Sep 29, 2024 •

edited

Loading

eun-byeol commented Sep 29, 2024 •

edited

Loading