-
Notifications
You must be signed in to change notification settings - Fork 264
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
[레거시 코드 리팩터링 - 4단계] 테오(최우성) 미션 제출합니다. #739
Conversation
어디다 달지 몰라서 테오의 pr 메시지에 대한 답변은 여기다 남길게요! 먼저 저도 멀티 모듈은 처음이라 제가 미션을 진행하면서 정확히 테오랑 같은 생각을 했습니다. 인수 테스트 부분에서 Order를 생성하려면 Product가 있어야하는 등등 이렇게 연결 되는 부분은 어떻게 해야할지 많이 고민이 있었습니다! 한참을 고민하다가 결론적으로 저는 모든 테스트를 테오처럼 app에 두었습니다. 제 생각을 짧게 말씀 드리겠습니다! 혹시 테오가 보시고 별로면 저한테 살짝 말씀해주세요 ㅋㅋ (리뷰하면서 테오한테 더 많이 질문하고 더 배워서 항상 감사합니다 ㅠㅠ 🙇🏻♂️) 그리고 제가 말을 진짜 잘 못하는데 그냥 보시고 이해 안되시면 dm 한 번 주세요 ㅠㅠ 최대한 노력해서 제 생각 표현해보겠습니다
이를 개발자 입장에서 본다면, 상품을 담으려면 먼저 가게가 존재해야하고, 가게가 상품을 등록해야하고... 여러 절차가 있습니다. 이런 절차들은 어떤 순서에 맞게 등록이 되겠죠? (여기서 순서란 [가게 등록 -> 가게의 상품 등록]과 같은 걸 의미합니다!) 그러기 때문에 실제 주문 시나리오를 맞추려면 어느 일련의 과정이 있어야 한다고 생각합니다. 반면에 저희는 이런 주문의 프로세스를 코드로 구현하다 보니 가게 등록부터 주문까지의 과정을 다 한 번에 표현할 수 없고, 중간 중간 도메인이라는 것을 두어서 나눠 표현한다고 생각합니다. 그러다 보니 이런 문제가 발생하지 않았나 싶습니다 ㅎㅎ 저는 그래서 모든 일련의 과정을 표현하는 이런 통합 테스트는 app 모듈로 보냈습니다. (어차피 app은 모든 모듈을 참조하다 보니깐 그렇습니다! 흐름을 담당하는 모듈로 저는 생각하고 분리했습니다.) |
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.
안녕하세요 테오!
테오의 생각에 대한 답변과 몇 가지 코멘트를 남겨봤습니다 ㅎㅎ
제가 리뷰어지만 배우는게 항상 많은 것 같습니다!
좋은 주말 보내시고 혹시 잘 이해 안되는 그런 부분들이 있다면 편하게 언제든지 DM 보내주세요~
build.gradle
Outdated
@@ -12,17 +12,87 @@ repositories { | |||
mavenCentral() |
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.
테오 남길 곳이 여기밖에 없어서 여기다 남겨요..ㅎㅎ
루트에서 src랑 모든 것을 지웠는데 plugins (1번째 줄부터의 gradle 문법) 등등이 필요해보이지 않습니다!
(저도 이번이 처음이라 혹시! 만약에 잘못 됐다면 꼭 알려주세요..!)
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.
오 감사합니다!
말씀해주신 plugins 부분을 지워봤는데요!
다른 repositories 블록은 같은 건 지워도 빌드가 잘 되는데, plugins 블록은 지우면 빌드가 안되더라고요.. ㅠㅠ
조금 오래된 자료긴 한데, 찾아본 결과에 따르면 allprojects, subprojects에서는 plugins 블록을 선언할 수 없다고 해요.
The new plugins {...} syntax cannot be used within a allprojects {...} or subprojects {...} closure.
https://stackoverflow.com/questions/26236308/how-to-apply-plugin-to-allprojects-with-new-gradle-plugins-mechanism
따라서 plugins 블록을 통해 루트 프로젝트에 플러그인들을 미리 선언해두고, subprojects 혹은 allprojects에서 가져다 쓰는 구조를 채택하는 것 같아요. 아래처럼요! (구조적으로 왜 이렇게 되어있는지는 조금 찾아봐야 하겠네요)
plugins {
id 'org.springframework.boot' version '2.7.15'
id 'io.spring.dependency-management' version '1.1.3'
id 'java'
}
subprojects {
group = 'camp.nextstep.edu'
version = '0.0.1-SNAPSHOT'
sourceCompatibility = '11'
apply plugin: 'java'
apply plugin: 'org.springframework.boot'
apply plugin: 'io.spring.dependency-management'
따라서 plugins 블록은 제외하고 나머지 부분은 제거할 수 있을 것 같은데.. 제이는 어떻게 생각하시나요? 저도 멀티모듈은 처음이라 틀린 부분이 있을 수 있어서 제이의 의견도 궁금합니다!! 🙇♂️
관련 gradle 공식 문서도 첨부드립니다!(https://docs.gradle.org/current/userguide/plugins.html#sec:subprojects_plugins_dsl)
apply plugin: 'java' | ||
apply plugin: 'java-library' | ||
apply plugin: 'org.springframework.boot' | ||
apply plugin: 'io.spring.dependency-management' |
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.
java-library는 혹시 뭔가요 ㅎㅎ 이건 궁금해서 여쭤봅니다!
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.
이 친구는 implementation
이 아닌 api
를 사용할 때 끌고오는 친구인데.. 어쩌다 낑겼나 봅니다..
사용하지 않으므로 삭제하도록 하겠습니다!
build.gradle
Outdated
implementation 'org.springframework.boot:spring-boot-starter-validation' | ||
implementation 'org.springframework.boot:spring-boot-starter-web' | ||
|
||
implementation 'org.flywaydb:flyway-core' |
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.
모든 서브 모듈이 flyway를 사용하지 않아보입니다!
app 모듈에만 해당 dependency만 가지고 있는 건 어떨까요?
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.
맞습니다!
그게 나을 것 같네요 ㅎㅎ 변경하도록 하겠습니다!
build.gradle
Outdated
} | ||
} | ||
|
||
project(':menu') { |
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.
한번에 모듈 간 의존성을 확인할 수 있다는 장점이 있을 것 같아요!
해당 정보를 기반으로 다이어그램으로 도식화도 쉽게 가능해보입니다 ㅎㅎ
저는 개인적으로 루트 프로젝트의 gradle 파일에 의존성을 명시하는 편이 나은 것 같은데, 제이는 어떻게 생각하시나요? 어떤 방법을 선택하셨는지도 궁금합니다!
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 class Application { | ||
|
||
public static void main(String[] args) { | ||
SpringApplication.run(Application.class, args); |
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.
app이 아닌 서브 모듈에서도 실행을 할 수 있군요 ㅎㅎ 처음 알았네요
여기서 컨텍스트를 띄우면 해당 컨텍스트의 요청만 받을 수 있나봅니다!
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 동작은 안하더라고요.. 🥹
flyway 스크립트가 app 쪽에 있어서 테이블 생성이 안되는 것 같아요.
flyway를 사용하지 않는 경우에는 잘 동작하는 건 확인이 되었습니다!
해당 모듈에 있거나 의존하고 있는 다른 모듈의 엔티티들만 끌고와서 테이블을 잘 만들어 주고, 동작도 잘 하더라고요!
슬랙에서도 flyway에 대해 비슷한 이야기가 나왔던 것 같은데.. 어렵네요 😅
만약 flyway를 사용하지 않는 경우라면 어플리케이션을 따로 두는게 좋을 것 같아요!! 👍
안녕하세요 제이! 좋은 답변 주셔서 감사합니다! 말씀해주신대로 테스트에서 검증하고자 하는 내용 자체가 어떠한 일련의 과정들이기 때문에 컨텍스트 모듈마다 분리해서 가져가기는 어려울 것 같아요! 따라서 지금의 방법을 유지해야겠다는 생각이 들었습니다! 사실 여러 도메인의 정보를 다뤄야 하는 테스트라면, 개별 컨텍스트에 두는 것도 이상하겠죠..🤔 만약 개별 컨텍스트(모듈)에서도 service 테스트를 하고 싶은 경우라면 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.
테오 미션 하시느라 수고 많으셨습니다!
마지막 미션인데 영광이네요 ㅎㅎ
단계 별로 진행할 때 많이 고민하신 게 느껴졌고 어떤 식으로 진행해야 리뷰어에게 좋을지도 많이 배웠습니다.
다음에 캠퍼스에서 뵙겠습니다 :)
안녕하세요 제이!!
매번 유익한 리뷰를 주셔서 감사할 따름입니다..
이번에는 요구사항에 맞게 컨텍스트 간 독립된 모듈을 가지도록 구성했습니다.
모듈 간 의존성은 루트 프로젝트의 build.gradle을 참고하시면 좋을 것 같습니다!
그런데 멀티모듈을 처음 사용하다보니 고민사항이 생겼는데, 제이와 같이 이야기해보면 좋을 것 같아요!
Service 테스트의 경우는 어떻게 분리해야 할지 고민입니다. 현재 하나의 서비스 메소드를 제대로 검증하기 위해서는 Dummy 데이터들이 필요합니다. 따라서 다른 모듈의 Repository에 의존하는 경우가 있습니다. (
MenuServiceTest
를 보시면 이해가 되실 거에요)그런데 이런 경우 모듈별로 서비스 테스트를 두기가 어렵습니다. 프로덕션 코드에서의 의존성은 없어도 테스트 코드에서 결국 의존성이 생겨나기 때문인데요. 따라서 현재는 서비스 테스트를 전부 app 모듈에 배치했습니다.
사실 app 모듈에 서비스 테스트가 위치한다는게 그렇게 만족스럽지는 않은 것 같습니다. 이를 해결하기 위해서는 어떻게 해야할까요? Mocking을 사용할까도 고민해봤는데, 배보다 배꼽이 더 커지는 상황인 것 같아 고민중입니다. 의존성 때문에 제대로 된 테스트를 못하게 되는 것 같아서요. 제이라면 이 상황에서 어떻게 해결하실지 궁금하네요!
처음 멀티모듈을 적용해봐서 궁금한 점이 많은 것 같네요.. 이번에도 좋은 리뷰 기다리고 있겠습니다! 🙇♂️