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

3단계 - 테스트를 통한 코드 보호 #391

Open
wants to merge 21 commits into
base: bperhaps
Choose a base branch
from

Conversation

bperhaps
Copy link

@bperhaps bperhaps commented Oct 3, 2022

안녕하세요 :) 많이 늦었지만 3단계 제출합니다.

이번 미션을 진행하면서, 인수테스트 비슷한 방식의 테스트를 통해서 도메인을 보호해 보고자 했어요. (e2e는 아니기 때문에 완벽히 인수테스트라고는 생각하지 않습니다.)
그래서 cucumber 에서 영감을 얻은 간단한 프레임워크를 구현해서 테스트를 작성해 봤습니다.

BDD방식의 테스트코드 작성을 진행하면서 느낀게, 과연 인수테스트가 도메인 보호로서의 역할을 하는지 잘 모르겠다는 것 입니다.

인수조건을 만족하는지에 대해서는, 문서화된 형태로 보이고, 개발자가 아니더라도 읽을 수 있는 장점이 있는건 알겠지만, 조금 더 세세한 조건의 테스트를 진행하거나 할때는 생산성이 많이 떨어지고 오히려 보기 힘들어지더라구요. 역시 도메인 보호를 위해서는 integerationTest 나 mock slice test를 통해서 도메인 로직만 테스트하는게 좋았던걸까요? 저는 개인적으로는 그렇게 생각하는데 규남님 의견도 궁금합니다 !

잘 부탁드립니다~!

Copy link

@kyucumber kyucumber left a comment

Choose a reason for hiding this comment

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

안녕하세요

몇가지 코멘트 남겨두었으니 확인 부탁드립니다.

다음 요청 시 머지하도록 할게요~

@@ -13,97 +13,97 @@ docker compose -p kitchenpos up -d
## 요구 사항

Choose a reason for hiding this comment

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

BDD방식의 테스트코드 작성을 진행하면서 느낀게, 과연 인수테스트가 도메인 보호로서의 역할을 하는지 잘 모르겠다는 것 입니다.

인수조건을 만족하는지에 대해서는, 문서화된 형태로 보이고, 개발자가 아니더라도 읽을 수 있는 장점이 있는건 알겠지만, 조금 더 세세한 조건의 테스트를 진행하거나 할때는 생산성이 많이 떨어지고 오히려 보기 힘들어지더라구요. 역시 도메인 보호를 위해서는 integerationTest 나 mock slice test를 통해서 도메인 로직만 테스트하는게 좋았던걸까요? 저는 개인적으로는 그렇게 생각하는데 규남님 의견도 궁금합니다 !

사람마다 생각하는건 다르겠지만 일단 저도 인수 테스트만을 이용해 도메인 보호의 역할을 훌륭하게 수행할 수 있느냐? 라는 질문에 대해서는 의문이 있습니다. 인수 테스트가 아닌 통합 테스트 또한 마찬가지라고 생각해요.

이렇게 생각하는 이유는 인수 테스트든 통합 테스트든 하위 레이어의 도메인 모델이나 도메인 서비스 등 모든 부분에 대해 테스트 케이스를 작성하고 검증하는게 어렵다고 생각하기 때문이에요.
아무리 테스트를 꼼꼼히 잘 작성해도 인수 테스트, 통합 테스트 정도의 범위에서 바라보면 엣지 케이스를 누락할 수 있으며 또한 레이어 별로 달라지는 브랜치(if, else)에 따라 테스트 케이스가 너무 많아질 수 있습니다.
결국 인수 테스트든 통합 테스트든 위와 같은 문제 때문에 도메인의 모든 상황을 잘 커버할 수 없습니다.

인수 테스트와 통합 테스트가 쓸모없다는 얘길 하고 싶었던 건 아니에요, 당연한 이야기겠지만 도메인 로직, 클래스나 메소드 단위로 단위 테스트를 꼼꼼히 잘 작성하고 레이어 별 슬라이스 테스트를 잘 작성해주는게 중요하다는 말씀을 드리고 싶었어요.
단위 테스트가 꼼꼼히 작성된 이후 필요한 시나리오에 따라 통합 테스트나 인수 테스트를 추가해주시면 도메인 보호의 역할을 훌륭하게 수행할 수 있을거라 생각합니다.

Comment on lines +21 to +33
public static class FakeKitchenridersClient extends KitchenridersClient {

private int requestCount;

@Override
public void requestDelivery(UUID orderId, BigDecimal amount, String deliveryAddress) {
requestCount++;
}

public int getRequestCount() {
return requestCount;
}
}

Choose a reason for hiding this comment

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

테스트를 위한 FakeKitchenridersClient를 잘 만들어주셨네요 👍

여기서 FakeKitchenridersClient는 호출 횟수를 기록하고 있어요. 테스트 더블의 분류를 기준으로 보았을 때 Fake가 아닌 Spy라는 이름이 더 적절하지 않을까요?

사실 이렇게 테스트 더블이라거나 Stub, Mock, Fake, Spy 등 너무 세세하게 이름을 나누는 것을 선호하진 않습니다. 다만 학습의 과정에 계셔서 코멘트 남겼어요. 요 부분은 굳이 반영하지 않으셔도 무방합니다.

import java.util.UUID;
import kitchenpos.domain.MenuGroup;

public class MenuGroupMother {

Choose a reason for hiding this comment

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

Fixture 사용 👍


@SpringBootTest
@EnableTestGlue
class OrderServiceTest extends TestIsolationSupport {

Choose a reason for hiding this comment

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

테스트를 꼼꼼히 잘 작성해주셨습니다. 👍

테스트가 많아 제가 놓쳤을 수 있는데 아래 요구사항에 대한 검증이 빠진 것 같아요.

주문이 EAT_IN 타입이고, 소속된 주문 테이블에 완료되지 않은 다른 주문이 없다면, 주문 테이블을 초기화 한다.

import org.springframework.context.ApplicationContextAware;
import org.springframework.context.support.GenericApplicationContext;

public class TestGlueInitializer implements ApplicationContextAware {

Choose a reason for hiding this comment

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

업무도 바쁘셨을텐데 요런 시도를 해보셨군요. 👍 👍

이쪽은 미션에서 요구하는 방향과는 무관하다고 생각하여 별도로 리뷰를 남기진 않았습니다~

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.

2 participants