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

Step4 리뷰 요청드립니다 #433

Open
wants to merge 20 commits into
base: sang-eun
Choose a base branch
from
Open

Conversation

sang-eun
Copy link

감사합니다!
이유는 잘 모르겠는데 step3 리팩토링 코드랑 좀 섞여있네요 ㅎㅎ 유념 부탁드립니다!

StationRepository stationRepository;

StationService stationService;
PathService pathService;
Copy link
Author

Choose a reason for hiding this comment

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

StationService의 createStationResponse 메서드를 그대로 사용하고 싶어서, stationService를 모킹하지 않고 따로 생성하도록 하였는데요
@Injectmocks도 사용할 수 없어 불편하고, 테스트할 대상 하나만 원본 클래스를 가져다 쓰고 나머지는 다 목으로 진행해야 한다는 목 테스트의 이념(?) 에도 맞지 않는 것 같아 고민이 됩니다.

혹시 다른 깔끔한 테스트 방법이나 리팩토링 방법이 있을까요?

Copy link

Choose a reason for hiding this comment

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

주로 지금과 같이 layered 아키텍처에서는 application(service) layer는 domain 객체들을 조합하는 역할을 하는데요,
지금 코드에서는 응답 Response 객체를 생성하는 역할 또한 service가 갖고 있어서 inject해야하는 문제가 생기는 것 같아요.

mock testing 방식을 사용했을 때 크게 느껴지는 단점 중 하나는 의존하는 객체가 많으면 많아질수록 테스트 작성이 까다로워진다는 거에요.
(when, thenReturn을 해야하니 내부 구현 자체를 직접적으로 알아야겠죠)

그런 점에서 service끼리 의존을 해야되는 경우엔 의존하려는 service에서 필요로 하는 기능이 정말 service에 있어야할까를 생각해보시면 좋을 것 같습니다 :)

지금의 경우는 Response 객체를 만드는걸 Response 객체로 넘겨주어도 될 것 같습니다 :)
(static factory method 등을 활용할 수 있을 것 같네요!)

Copy link

@ivvve ivvve left a comment

Choose a reason for hiding this comment

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

안녕하세요 상은님~

설계적으로 리팩터링 관련해서 생각해보시면 좋을 것 같아 코멘트 남깁니다.
확인 부탁드리겠습니다 🙏

StationRepository stationRepository;

StationService stationService;
PathService pathService;
Copy link

Choose a reason for hiding this comment

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

주로 지금과 같이 layered 아키텍처에서는 application(service) layer는 domain 객체들을 조합하는 역할을 하는데요,
지금 코드에서는 응답 Response 객체를 생성하는 역할 또한 service가 갖고 있어서 inject해야하는 문제가 생기는 것 같아요.

mock testing 방식을 사용했을 때 크게 느껴지는 단점 중 하나는 의존하는 객체가 많으면 많아질수록 테스트 작성이 까다로워진다는 거에요.
(when, thenReturn을 해야하니 내부 구현 자체를 직접적으로 알아야겠죠)

그런 점에서 service끼리 의존을 해야되는 경우엔 의존하려는 service에서 필요로 하는 기능이 정말 service에 있어야할까를 생각해보시면 좋을 것 같습니다 :)

지금의 경우는 Response 객체를 만드는걸 Response 객체로 넘겨주어도 될 것 같습니다 :)
(static factory method 등을 활용할 수 있을 것 같네요!)

Comment on lines +33 to +49
@BeforeEach
public void setUp() {
super.setUp();

강남역 = 지하철역_생성_요청("강남역").jsonPath().getLong("id");
양재역 = 지하철역_생성_요청("양재역").jsonPath().getLong("id");
신논현역 = 지하철역_생성_요청("신논현역").jsonPath().getLong("id");
고속터미널역 = 지하철역_생성_요청("고속터미널역").jsonPath().getLong("id");

Map<String, String> lineCreateParams = createLineCreateParams(강남역, 양재역);
신분당선 = 지하철_노선_생성_요청(lineCreateParams).jsonPath().getLong("id");
지하철_노선에_지하철_구간_생성_요청(신분당선, createSectionCreateParams(강남역, 양재역));
지하철_노선에_지하철_구간_생성_요청(신분당선, createSectionCreateParams(신논현역, 강남역));

Map<String, String> line9CreateParams = createLineCreateParams(고속터미널역, 신논현역);
구호선 = 지하철_노선_생성_요청(line9CreateParams).jsonPath().getLong("id");
}
Copy link

Choose a reason for hiding this comment

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

공간과 관련된 알고리즘에 테스트를 하기 때문에
나중에 자신이 보거나 다른 개발자가 이 테스트 읽고 파악하기가 힘들 수 있을 것 같습니다.

image

이번 단계 예시에 있던 것 처럼 주석으로 픽스쳐를 설명해주면 나중에 다시 보았을 때도 쉽게 테스트를 파악할 수 있을 것 같습니다 :)

Comment on lines 44 to 46
@Test
@DisplayName("경로 조회")
void getPath() {
Copy link

Choose a reason for hiding this comment

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

추가적으로 예외 상황에 대해서 테스트를 해주시면 좋을 것 같습니다!

image

대신에 지금과 같이 service layer에 비즈니스 로직이 들어가게 되면 테스트 해봐서 아시겠지만
service에서 필요한 (test double이든 실제 객체든)의존성이 갖춰진 상태에서 테스트를 해야하니 테스트 작성이 번거롭거나 까다로워지는데요.

첫번째 단계의 요구사항을 통해 경험하셨듯, 길 찾기에 대한 도메인 객체를 만들어서 단위 테스트를 해보시는건 어떨까요? :)

Copy link
Author

Choose a reason for hiding this comment

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

해당 리뷰를 반영하면서
출발역과 도착역이 연결이 되어 있지 않은 경우 에 대해 pathService 서비스 레이어 mock 테스트를 짜는데,
이를 pathService에서 테스트하려니 stationService를 목처리해둬서, 테스트를 짜려면 stationService.findById에서 에러를 던지도록 목 테스트를 진행해야 했습니다. 하지만, 이는 코드에 대한 테스트를 짜기보다는 테스트를 위한 테스트라는 생각이 들어서 결국 이 경우는 실제 객체 테스트에서 진행해야 했는데요.
이와 같은 경우는 제가 한 것처럼 mock테스트를 피하는게 맞는 것인지? 아니면 다른 방법이 있는지 궁금합니다!

Copy link

Choose a reason for hiding this comment

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

@sang-eun
네네 mock을 사용한 테스팅의 경우 의존하는 객체를 매번 설정을 하는 것이 매우 번거롭죠...!
그래서 첫번째 단계에서 얘기드렸듯이 저는 fake 객체를 사용하는 걸 선호하는 편입니다!

지금과 같은 경우 길을 찾는 역할을 갖는 객체를 따로 만든다면 좀 더 가벼운 테스팅이 가능할 것 같습니다!
(이미 Path 객체에서 처리하도록 하셨네요 👍 )

@ivvve
Copy link

ivvve commented Aug 15, 2022

@sang-eun
혹시 다시 리뷰 요청을 하시길 원하시면
넥스트스텝 사이트에서 리뷰 요청을 다시 해주시면 됩니다!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants