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

Step3 - 테스트를 통한 코드 보호 #855

Open
wants to merge 5 commits into
base: wocks1123
Choose a base branch
from

Conversation

wocks1123
Copy link

안녕하세요! 늦었지만 PR 드립니다.
서비스 클래스에 대한 단위테스트를 작성할까 고민하다가 통합테스트를 작성했습니다. 나중에 리팩토링을 진행하면 서비스 코드가 많이 변경될텐데 통합테스트로 구현하면 테스트 코드 수정이 비교적 적지 않을까 생각했습니다. Order 관련 테스트 코드는 분량이 많아 작업중입니다.
이번 과제 수행중 궁금한 내용은 테스트 데이터 초기화입니다. 테스트 메서드 시작전에 repository를 사용해 DB에 데이터를 추가했는데, 테스트 종료 후 menuRepository.deleteAll()을 호출하면 MenuProduct와 외래키 제약때문에 오류가 발생했습니다. repository로 delete를 호출하지 않는 방식을 생각해봤습니다.

  • @Transactional을 붙이면 메서드 종료 후 롤백을 수행하지만, 트랜잭션 범위를 테스트 메서드까지 확장하는건 적절하지 않은 방식 같습니다.
  • sql을 작성해서 테스트 전후에 삽입, 삭제 쿼리를 실행하면 테스트 데이터 관리가 명확해지지만 SQL까지 관리해야하는 점이 번거로울 것 같습니다.
    지금은 관리해야할 내용이 추가되더라도 테스트용 SQL 파일을 추가하는게 맞지 않을까 생각중입니다. 연관관계가 복잡한 테스트 데이터가 필요할 때 어떻게 관리할 수 있을지 질문드립니다.
    감사합니다!

- 테스트를 위한 Product 객체 생성 클래스 추가
- 테스트를 위한 MenuGroup 객체 생성 클래스 추가
- fixture 사용, db 데이터 저장 후 테스트
@HackSung
Copy link

서비스 클래스에 대한 단위테스트를 작성할까 고민하다가 통합테스트를 작성했습니다.
나중에 리팩토링을 진행하면 서비스 코드가 많이 변경될텐데 통합테스트로 구현하면 테스트 코드 수정이 비교적 적지 않을까 생각했습니다.

통합테스트가 리팩토링 시 테스트 코드 변경을 대체로 줄일 수 있겠지만 항상 그런 것은 아닌 것 같습니다.
통합테스트는 시스템의 여러 계층이 상호작용하는 흐름을 검증하기 때문에
계층 간의 인터페이스가 변경되면 테스트 코드도 수정해야 하기 때문입니다.
반대로 단위 테스트는 특정 클래스나 메서드의 동작만 검증하기 때문에 
클래스가 변경되더라도 테스트 코드 수정이 최소화 될 수도 있구요
혹시 테스트 피라미드 원칙에 대해 들어보신적이 있으실까요?
이 원칙은 상위 계층으로 갈수록 더 적은 테스트를 작성해야 하고
작고 빠른 단위 테스트를 많이 작성하는게 좋다고 얘기합니다.
관련되어 자세한 글이 있는데 내용은 길지만 시간을 들여 읽어 볼 가치가 있는 것 같아 아래 공유드립니다.

연관관계가 복잡한 테스트 데이터가 필요할 때 어떻게 관리할 수 있을지 질문드립니다.

재찬님이 말씀하신 부분은 개발자라면 누구나 고민하게 되는 현실적이면서 중요한 문제인 것 같습니다.
테스트 데이터 관리에 대해 이미 여러가지 방법을 고려해 보셨는데 
각 방법마다 장점과 단점이 존재하고 있어 상황에 맞는 방법을 택해야 할 것 같습니다.
제 의견도 재찬님의 생각과 거의 동일한데
지금 미션처럼 간단한 CRUD성 테스트는 H2같은 메모리DB를 사용하는게 편할 수 있고
트랜잭션관련 테스트가 아니라면 @Transaction, @DataJpaTest, @TestEntityManager등을 활용할 수도 있고
연관관계가 복잡하다면 말씀하신 것처럼 SQL스크립트나 데이터 픽스쳐 패턴을 사용하는 것이 좋다고 생각합니다.

Copy link

@HackSung HackSung left a comment

Choose a reason for hiding this comment

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

바쁘신데 늦은 시간까지 미션진행하시느라 너무 고생하셨습니다! 👍
테스트 코드도 잘 작성해 주셨네요! 💯
그럼 리뷰 확인 부탁드립니다~ 🙏


@Test
@DisplayName("메뉴를 등록한다.")
void registerMenu() {

Choose a reason for hiding this comment

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

요구 사항을 다시 읽어보면 메뉴 등록이나 가격변경시 몇가지 실패케이스에 대한 테스트가 누락된게 보이네요?

메뉴의 가격은 지정한 상품들의 가격 총합보다 작아야 한다.

테스트 코드 작성하시면서 요구 사항의 [ ] 체크박스를 [x] 로 하나씩 표시하시면 놓치지 않으실 것 같아요.

Choose a reason for hiding this comment

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

혹시 메뉴의 숨김이나 표시관련 기능의 테스트는 필요 없을까요? 🤔

  • 지정한 메뉴를 노출한다.
  • 지정한 메뉴를 숨김처리한다.

@Autowired
private MenuGroupRepository menuGroupRepository;

@Nested

Choose a reason for hiding this comment

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

@Nested 를 사용하여 테스트 케이스를 그륩화하니 한결 보기 좋은 것 같아요! 👍

Comment on lines +70 to +71
MenuGroup menuGroup1 = MenuGroupFixture.createMenuGroup("한마리메뉴");
MenuGroup menuGroup2 = MenuGroupFixture.createMenuGroup("두마리메뉴");

Choose a reason for hiding this comment

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

테스트 코드의 변수명에 넘버링보다 한글변수명등을 사용하면 직관적이 되어 가독성이 향상될 수 도 있어요. 😄

Suggested change
MenuGroup menuGroup1 = MenuGroupFixture.createMenuGroup("한마리메뉴");
MenuGroup menuGroup2 = MenuGroupFixture.createMenuGroup("두마리메뉴");
MenuGroup 한마리_메뉴그룹 = MenuGroupFixture.createMenuGroup("한마리메뉴");
MenuGroup 두마리_메뉴그룹 = MenuGroupFixture.createMenuGroup("두마리메뉴");

private MenuGroupService menuGroupService;

@Autowired
private MenuGroupRepository menuGroupRepository;

Choose a reason for hiding this comment

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

혹시 조금 여유가 되신다면 필수는 아니나 수업시간에 배우신 Fake객체로 Repository를 대체하면
어떤 이점이 있는지 직접 체감해 보시는 것도 좋을 것 같아요. 😄

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