-
Notifications
You must be signed in to change notification settings - Fork 4
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
Fixture 관련 쿼리를 테스트 메서드 실행 전 호출할 수 있도록 로직 변경 #748
Conversation
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.
질문
지토가 이슈에 적어주신 통합 테스트를 하기 위해 별도의 Fixture 클래스에서 필요한 데이터를 insert하는데, 코드로 하다보니 데이터를 저장하는 과정에서 @Transactional이 필요한 문제였습니다.
에 대한 이유가 궁금합니다.
이전에도 말씀드린 적이 있지만, 저희가 저번에 시간을 들여 repository의 jpa 의존성을 제거하고 도메인 레이어에 repository 인터페이스를 만들어서 분리하는 작업을 했었습니다.
그럼에도 불구하고 서비스 레이어에서는 jpa가 제공하는 변경감지를 사용하고 있기 때문에 이를 개선해야한다고 생각합니다.
마찬가지로 테스트코드의 경우에도 Jpa를 사용하지만 JpaRepository에 대한 테스트가 아니라면 최대한 그 의존을 줄여야한다고 생각하기 때문에 픽스처 생성 시에도 변경감지를 사용하지 않아야 한다고 생각합니다. (지금은 변경감지를 사용하기 때문에 지금 상태에서 @Transactional
만 뺀다면 테스트가 실패할 것 같네요) 아무튼 그래서 값을 변경한 엔티티의 경우 일일이 save()를 다시 해주는 것이 맞다고 생각하고, em.flush()
도 사용하지 않는 것이 좋다고 생각합니다.
또, 테스트에서 @Transactional
을 사용하지 않으면 픽스처에서 save() 호출 시마다 flush 및 commit이 일어날 것이기 때문에 굳이 em.flush()를 안 해도 되지 않을까요?
그리고 이 부분이 조금 애매한게 도메인 패키지에 JpaRepository를 위치시켰으니 이거는 컬렉션에서 도메인 엔티티를 꺼내는 용도로 사용되는 의미라고 볼 수도 있다고 생각합니다
그래서 아무튼 결론은 지금 cascade를 사용하기 때문에 save()를 호출해준다고 해도 불가능(TransactionRequiredException 예외 발생)하다고 볼 수 있고 이걸 어케 풀어나가느냐는 논의가 필요할 것 같네요 |
이 부분에 대해서는 저희 JpaRepository는 |
요런 부분은 인프라 리펙토링으로 인해서 JpaReposiory와 같은 구체적인 부분에 대한 의존은 약해졌지만 ORM을 사용한다는 가정 하에 비즈니스 로직을 작성했으니 어쩔 수 없다 싶기는 합니다
|
📄 작업 내용 요약
Fixture에 em.flush() 호출 추가
🙋🏻 리뷰 시 주의 깊게 확인해야 하는 코드
이슈에 관련된 내용을 첨부했습니다
예시로 BidServiceFixture만 수정했습니다
BidService로 한 이유는 외래 키 데드락 찾느라고 계속 얘만 돌리고 있기 때문입니다
📎 Issue 번호