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

[레거시 코드 리팩터링 - 1단계] 성하(김성훈) 미션 제출합니다. #518

Merged
merged 21 commits into from
Oct 14, 2023

Conversation

sh111-coder
Copy link

안녕하세요 말랑!!!! 이번 미션 잘부탁드립니다 ㅎㅎㅎ 🤣

첫번째 요구사항 부분은 README.md에 요구사항을 분석했습니다!

그리고 두번째 요구사항인 테스트 작성이 좀 리소스가 많이 들고 힘들었는데요!

처음에 모든 Business Object에 대한 테스트 코드를 작성한다.의 요구사항을 보고
Business Object에 DAO이 포함되냐, 되지 않느냐로 고민했었는데요!
고민했던 부분들을 아래에 나열해보도록 하겠습니다!

❓ DAO는 Business 로직?

처음에는 영속화하는 것까지도 비즈니스 로직으로 생각해서 DAO 테스트까지 진행했었습니다. (커밋에도 있네요 ㅠㅠ 😭)
그러나 진행 도중에 JPA로 마이그레이션 하게 된다면 어차피 사라질 테스트라고 생각했고,
또 비즈니스 로직에 영속화 하는 로직은 들어가지 않는다고 생각하여 DAO 테스트를 지우고 Service 테스트만 진행하게 되었습니다!


❓ Service 테스트 DAO 모킹 or 주입

이러한 생각 이후에는 Service 테스트에서 DAO를 모킹할지, 실제로 주입해서 테스트할지 고민했었습니다!
결론적으로 저는 DAO를 모킹해서 테스트하게 되었습니다.
모킹했던 이유는 위에서 DAO 테스트를 지우고 Service 테스트만 진행한 이유와 관련이 있는데요!

DAO를 비즈니스 로직으로 생각하지 않아서 DAO 테스트를 진행하지 않았기 때문에
요구사항인 Business Object에 대한 테스트 코드를 작성한다.에서 Service 테스트에서 DAO를 주입해서 테스트하게 된다면
비즈니스 로직인 Service 로직에 비즈니스 로직이라고 생각하지 않는 DAO 로직이 섞이는 느낌을 받았기 때문에
DAO의 의존성을 분리하여 모킹하게 되었습니다!!


※ DAO 테스트를 삭제하기 이전에는 Service 테스트 시 DAO를 주입해서 진행했어서
MenuServiceTest에는 실제 DAO를 주입해서 테스트한 코드가 남아있습니다 😭
그래도 시간 안에 제출하는 것이 좋다고 생각하여 이번에 리팩토링 하면 좋을 것 같다고 코멘트 주시면 이번에 하고,
아니면 2단계에서 한꺼번에 리팩토링 하도록 하겠습니다!! 죄송합니다 말랑 😭😭

@sh111-coder sh111-coder self-assigned this Oct 12, 2023
Copy link

@shin-mallang shin-mallang left a comment

Choose a reason for hiding this comment

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

안녕하세요 성하~ 말랑입니다!
반가워용 ㅎㅎ

코드 너무 잘 작성해 주셨고, 간단한 커멘트 남겼습니다!

그리고 전체적으로 질문드리고 싶은(같이 토론하고 싶은?) 부분이 있어서 여기서 말씀드리려 해요!

성하 질문하고도 연관이 있으니 한번에 하도록 하겠습니다.

❓ Service 테스트 DAO 모킹 or 주입
DAO를 비즈니스 로직으로 생각하지 않아서 DAO 테스트를 진행하지 않았기 때문에
요구사항인 Business Object에 대한 테스트 코드를 작성한다.에서 Service 테스트에서 DAO를 주입해서 테스트하게 된다면
비즈니스 로직인 Service 로직에 비즈니스 로직이라고 생각하지 않는 DAO 로직이 섞이는 느낌을 받았기 때문에
DAO의 의존성을 분리하여 모킹하게 되었습니다!!

(앞으로 말씀드리는 내용은 저의 개인적인 생각입니다..! 틀릴 가능성도 많아요!)
저는 리팩토링 전 테스트를 작성하는 이유를 다음과 같이 생각하고 있어요.
현재 잘 동작하는 코드를 변경하였을 때 기존와 정확히 동일한 기능을 수행함을 보장하기 위해, 즉 리팩토링 후 바뀐 코드의 올바른 동작에 대한 신뢰성을 보장하기 위해 리팩토링 전에 테스트 코드를 작성한다고 생각했어요!

하나위 예시로, 성하가 현재 상황에서 Dao 코드를 리팩토링 했는데, 메서드 시그니처는 갖지만 내부 코드에서만 지지고 볶다가 잘못된 동작을 하게끔 코드를 변경했다고 해볼게요!
그럼 그 잘못된 코드가 현재 상황에서 검출이 될까요?
저는 그렇지 못한다고 생각합니다!

또 모킹은 어쩔 수 없이 코드의 내부 구현이 테스트코드에 노출된다고 생각하는데요, 이로 인해 내부 구현이 바뀌면 테스트코드에도 변경이 가해져야 합니다!

리팩토링 시 코드가 어떤 식으로 바뀔 지 예측이 불가능하기 때문에, 저는 서비스의 잔체 동작이 리팩토링 이전과 이후로 동일한가를 검증하는 코드가 반드시 있어야 한다고 생각해요!

서실 저는 이러한 이유로 모든 테스트 다 버리고, 오직 인수 테스트만을 작성했습니다.
인수 테스트가 리팩토링 전 후로 동일하게 통과한다면, 리팩토링 후에도 이는 올바르게 동작함을 보증할 수 있다고 생각했기 때문에요!
(물론 서비스 코드로 100%의 상황을 다 ㅓ버할 수는 없겠지만요!)

이에 대해서 성하는 어떻게 생각하시나요??
리팩토링 후에도 코드가 오류없이 동작하는지 현재의 테스트 코드가 보증해 줄 것이라 생각하시나요?

@@ -0,0 +1,15 @@
package kitchenpos.exception;

public class MenuGroupException extends RuntimeException {

Choose a reason for hiding this comment

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

저는 1단계 요구사항이 현재 코드가 리팩토링 이후에도 잘 동작하는지를 보증하기 위해 리팩토링 전 단계로써 테스트를 작성하는 것이라 이해했습니다!
성하처럼 테스트를 진행하면서 코드의 수정이 가해지면 코드의 동작이 기존과 달라져도 알아차릴 수 없을 가능성이 높지 않을까요..?
사실 이부분은 잘 모르겠어서 성하의 의견이 궁금합니다!
(저는 그래서 코드에 아무런 변경을 가하지 않고 테스트코드만 작성했거든요!)

Copy link
Author

@sh111-coder sh111-coder Oct 13, 2023

Choose a reason for hiding this comment

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

저도 제출 후에 리뷰이의 코드를 보고 아차 싶더라구요.. ㅎㅎ;;;
제가 미션의 의도를 잘 파악하지 못하고 맘대로 프로덕션 코드를 바꾼 것 같아요 ㅠㅠ
저도 다시 돌아간다면, 프로덕션 코드를 건드리지 않고 맞춰서 테스트 코드를 작성할 것 같네요 😂


그래도 프로덕션 코드 리팩토링을 진행한만큼, 진행했을 때 제 생각은 다음과 같았습니다!
처음 프로덕션 코드를 봤을 때 아무런 메시지 없이 동일한 IllegalArgumentException으로 예외를 발생시키는 것이 좋지 않은 코드라고 생각해서 프로덕션 예외 코드 정도만 리팩토링하게 되었습니다!
그래도 중요한 비즈니스 로직은 건드리지 않고, 통일된 예외들만 건드리게 되었어요!

저는 1단계 요구사항이 현재 코드가 리팩토링 이후에도 잘 동작하는지를 보증하기 위해 리팩토링 전 단계로써 테스트를 작성하는 것이라 이해했습니다!

그래서 저는 무의식적으로 '현재 코드'가 주어진 코드가 아닌,
프로덕션 예외 코드 리팩토링 후의 코드를 '현재 코드'로 생각했던 것 같아요!

그래서 테스트 코드가 짠 시점에서의 코드의 동작이 프로덕션 코드와 싱크가 맞다는 점에서
이후에 코드의 동작이 달라져도 알아차릴 수 있을 것이라고 생각합니다!!

public static class NotFoundOrderLineItemMenuExistException extends OrderException {

public NotFoundOrderLineItemMenuExistException() {
super("[ERROR] 주문 항목 목록에 메뉴가 누락된 주문 항목이 존재합니다.");

Choose a reason for hiding this comment

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

커스텀 익셉션을 하나 만들고, 프리픽스로 Error를 붙여주는 것도 괜찮을 것 같아용!

Copy link
Author

Choose a reason for hiding this comment

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

오 대박!! 말씀해주신대로 리팩토링해봤습니다!!
코드가 더 컴팩트해져서 가독성이 좋아진 것 같아요!! 👍🏻👍🏻

Choose a reason for hiding this comment

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

그럼요 누가 알려줬는데요~
라고 할 뻔 👍👍

final Menu createdMenu = menuService.create(menu);

// then
assertSoftly(softly -> {

Choose a reason for hiding this comment

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

assertSoftly를 사용하신 이유가 있나요?
(소프트콘 먹고싶어여)

Copy link
Author

@sh111-coder sh111-coder Oct 13, 2023

Choose a reason for hiding this comment

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

여러 개의 검증을 테스트에서 수행할 때
Assertions.assertAll()SoftAssertions.assertSoftly()가 있는 걸로 알고 있습니다!
둘 중에 assertSoftly()가 테스트 실패 시에 여러 개의 검증 중에 틀린 검증부를 추적할 수 있다는 점에서 사용하게 되었습니다!
성능면에서는 assertAll()이 미세하게 빠르다고 하는 것 같아요!

(저도 롯데리아 소프트콘 너무 먹고 싶어여)

Choose a reason for hiding this comment

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

맞아요 저도 AssertAll이 깨진 부분 찾기가 조금 힘들어서 별로 사용하지 않게 되는것 같더라구요
답변 감사합니다 :)

(소프트콘 사주시져)


@Test
@DisplayName("생성에 성공한다.")
void success() {

Choose a reason for hiding this comment

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

앞으로 단계를 진행하며 리팩토링을 진행하게 된다면, 이렇게 mock으로 작성된 서비스 코드가 굉잔히 많이 변경될수도 있지 않을까요?

(모킹의 특성 상 내부 구현에 대한 부분이 테스트코드에 노출이 될 수밖에 없어서, 테스트코드의 변경이 반드시 발샐할 것 같아요..!)


리팩토링으로 인해 코드 변경 -> 코드 변경으로 인해 테스트 깨짐(모킹으로 인해) -> 테스트코드 변경


위 상황에서 바뀐 내 코드가 올바른가..?를 확인하는 것이 조금 믿음직스럽지 못할 것 같다는 생각도 들어요!
(하지만 경험해보지 못했기에 확실히는 말 못하겠네요 🥲)
성하는 이 부분에 대해 어떻게 생각하시나요?


Copy link
Author

@sh111-coder sh111-coder Oct 13, 2023

Choose a reason for hiding this comment

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

제가 생각하기로는 모킹하지 않고 DAO를 주입해서 테스트를 작성해도,
DAO 로직?의 변경에 관해서는 테스트 코드의 변경이 많이 발생할 것 같아요!

현재 모킹을 통해서 직접 데이터를 넣지 않고 데이터가 존재하는 것처럼 DAO의 반환값을 지정해주고 있는데요!
DAO를 주입해서 테스트를 했을 때 데이터를 넣는 코드가 존재할 것이기 때문에
서비스 코드에서 모킹한 DAO 로직이 변경되어서 DAO를 모킹한 코드가 변경된다면,
실제 DAO를 주입한 테스트에서도 데이터 넣는 코드가 변경될 것 같아요!

리팩토링으로 인해 코드 변경 -> 코드 변경으로 인해 테스트 깨짐(모킹으로 인해) -> 테스트코드 변경

그래서 해당 과정에서 서비스의 DAO 로직이 변경되어서 테스트가 깨지는 부분은 모킹과 상관없이 둘 다 깨질 것이라고 생각합니다!
그래서 DAO 관련 로직 변경으로 인한 테스트 코드 변경은 어쩔 수 없다고 생각합니다!!
(제가 잘못 생각했을 수도 있을 것 같은데, 지금 생각은 이렇습니다!! ㅎㅎ,,,)

Copy link

@shin-mallang shin-mallang Oct 14, 2023

Choose a reason for hiding this comment

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

음 생각해보니, 그럴 것 같기도 하네요!
하지만 저는 다음과 같은 상황을 조금 고려했어요!

예를 들어 어떤 주문이 존재하는지를 기존에는 dao.existsBy..으로 검증했다가, 이후 dao.findBy...().isPresent() 등으로 검증하거나, count등으로 검증하도록 로직이 바뀐다면, 그런 구현에 따라 mocking해주는 부분이 달라지지 않을가 싶었어요!
그러나 해당 부분을 Stub 혹은 실제 객체를 사용하여 통합테스트를 진행하면 테스트는 변함이 없지 않을까 생각했네요..!

하지만 결국은 다 생각이니, 직접 다음 단계 하면서 같이 느껴봅시당

Comment on lines +13 to +15
/**
* NAME
*/

Choose a reason for hiding this comment

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

개인적으로 필요 없는 주석이라 생각해요!
이미 이름으로 충분히 가격임을 알 수 있지 않을까요?

Copy link
Author

Choose a reason for hiding this comment

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

저도 개별적으로 볼 때는 상수 네이밍만으로도 어떤 의미인지 파악이 가능하다는 것에는 동의합니다!!

저는 픽스쳐를 만들어서 테스트 코드의 중복되는 변수 선언을 줄이기 위해 사용했는데요!
개인적으로 처음 보는 테스트 코드를 봤을 때 픽스쳐를 사용한 코드는 해당 픽스쳐를 들어가서 변수를 확인해보게 되더라구요!

그래서 픽스쳐 클래스 내의 가독성을 높이기 위해 주석을 사용하는 편입니다!!
지금은 변수가 적어서 조금 필요 없을수도 있을 것 같은데,
변수가 많은 프로젝트에서 사용해보니 가독성이 확실히 좋아지더라구요! 그래서 놔두려고 하는데 괜찮을까요?!

Choose a reason for hiding this comment

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

변수명 자체에 NAME이 있는데 굳이 NAME이라 한번 더 적어줘야 할 필요성은 아직 와닿지는 않네요!
차라리 한국어로 적혀있다면 모를까..?
그치만 남겨뒀을 때 문제가 생기는 것도 아니고, 가독성은 사람마다 다른 부분이니 성하 입장에서 가독성이 높다면 남겨두는 것도 괜찮을 것 같아요~

public class TableGroupFixtures {

/**
*

Choose a reason for hiding this comment

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

????

Copy link
Author

Choose a reason for hiding this comment

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

앗...ㅋㅋㅋㅋㅋㅋ 실수해버렸네요 ㅋㅋㅋㅋㅋ 추가하겠습니다!

@shin-mallang
Copy link

MenuServiceTest에는 실제 DAO를 주입해서 테스트한 코드가 남아있습니다 😭
그래도 시간 안에 제출하는 것이 좋다고 생각하여 이번에 리팩토링 하면 좋을 것 같다고 코멘트 주시면 이번에 하고,
아니면 2단계에서 한꺼번에 리팩토링 하도록 하겠습니다!! 죄송합니다 말랑 😭😭

2단계에서 해주시죠 ㅎㅎ

1 similar comment
@shin-mallang
Copy link

MenuServiceTest에는 실제 DAO를 주입해서 테스트한 코드가 남아있습니다 😭
그래도 시간 안에 제출하는 것이 좋다고 생각하여 이번에 리팩토링 하면 좋을 것 같다고 코멘트 주시면 이번에 하고,
아니면 2단계에서 한꺼번에 리팩토링 하도록 하겠습니다!! 죄송합니다 말랑 😭😭

2단계에서 해주시죠 ㅎㅎ

@shin-mallang
Copy link

❓ DAO는 Business 로직?

처음에는 영속화하는 것까지도 비즈니스 로직으로 생각해서 DAO 테스트까지 진행했었습니다. (커밋에도 있네요 ㅠㅠ 😭)
그러나 진행 도중에 JPA로 마이그레이션 하게 된다면 어차피 사라질 테스트라고 생각했고,
또 비즈니스 로직에 영속화 하는 로직은 들어가지 않는다고 생각하여 DAO 테스트를 지우고 Service 테스트만 진행하게 되었습니다!

저도 Dao는 비즈니스 로직은 아니라 생각합니다~
그런데 구현에 따라서 Dao에 비즈니스 로직이 들어갈 수도..?(ㅎ..)

그런데 하나 질문 드리고 싶은게, JPA로 마이그레이션 하게되면 어차피 사라질 테스트가 된다고 생각할 수 있지만, 갑자기 다른 기술을 통해 리팩토링을 진행하게 되면 어떻게 될 것 같나요..?

추가로 비즈니스 로직에 영속화가 들어가지 않는다..? 는 잘 모르겠습니다..!
뭔가 회원 가입 등을 생각해 보면, 영속화까지 이루어지는 것이 하나의 비즈니스 로직을 처리하는 거 아닐까요..?

영속화는 비즈니스 로직은 아니지만, 비즈니스 로직을 구현하기 위한 필수적인 기능이라 생각이 드네요..!

@sh111-coder
Copy link
Author

안녕하세요 말랑!! 칭찬도 잘 해주시고 의견들 많이 남겨주셔서 감사합니다 ㅎㅎㅎ
코멘트에 대한 답변들 코멘트에 달았어요!

PR 코멘트로 주신 부분에 대해 아래에 답변해보겠습니다!!


JPA로 마이그레이션 하게되면 어차피 사라질 테스트가 된다고 생각할 수 있지만, 갑자기 다른 기술을 통해 리팩토링을 진행하게 되면 어떻게 될 것 같나요..?

이 부분을 고려해서라도 테스트가 존재해야 할 것 같다는 것에 동의합니다!!
이번에 테스트를 따로 하지 않은 이유는 아무래도 실제 프로젝트가 아닌 '미션'이고
지금 상황에서 시간을 많이 쓸 수 없다보니 어쩔 수 없이 작성하지 않은 것 같아요!!


영속화는 비즈니스 로직은 아니지만, 비즈니스 로직을 구현하기 위한 필수적인 기능이라 생각이 드네요..!

저도 말랑의 의견을 듣고 보니 동의합니다!!
비즈니스 로직이 아닌 것이 '영속화' 개념이 아니라, '영속화'를 어떻게 구현하는지에 관한 것이라고 생각이 드네요!!
감사합니다 ㅎㅎㅎ


리팩토링 시 코드가 어떤 식으로 바뀔 지 예측이 불가능하기 때문에, 저는 서비스의 잔체 동작이 리팩토링 이전과 이후로 동일한가를 검증하는 코드가 반드시 있어야 한다고 생각해요!

저도 이러한 부분은 애플리케이션 시작부터 끝까지 테스트를 하는 E2E 테스트를 통해 전체 동작이 검증되어야 한다고 생각합니다!!
제가 서비스 테스트를 이렇게 작성했던 이유는 DAO와 분리된 서비스의 로직만을 테스트하기 위해 DAO를 모킹하여 테스트했습니다!!

그래서 보통 저는 DAO(Repository) 테스트를 작성을 하고, DAO의 테스트를 작성했으니
Service에서는 DAO의 의존을 끊고 서비스의 로직만 테스트한 후에
전체적인 동작은 E2E 테스트로 테스트하는 방식을 전체적으로 적용했었습니다!

그래서 시간이 많고 제대로 해야했다면 위의 방식을 사용해서 테스트를 짰을 것 같은데,
현재 구조에서는 확실히 전후의 동작을 검증하기가 힘들 것 같긴하네요!!

2단계 때 고려해서 리팩토링 해봐야 좀 더 알 것 같아유 ㅠㅠ 😭

Copy link

@shin-mallang shin-mallang left a comment

Choose a reason for hiding this comment

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

질문에 대한 답변 너무 감사합니다~
성하 덕분에 저도 여러 부분으로 많이 생각해 볼 수 있었던 것 같아요~
1단계는 빠르게 merge하겠습니다!
2단계도 기대할게요 :)

@shin-mallang shin-mallang merged commit 86168c0 into woowacourse:sh111-coder Oct 14, 2023
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