[BE] refactor: Repository 개선 및 서비스 단위 테스트 Fake 사용 (#648) #649

seokjin8678 · 2024-01-10T11:52:36Z

📌 관련 이슈

closed: [BE] Repository를 개선한다. #648

✨ PR 세부 내용

이슈 내용 그대로 작업했습니다.
그에 따라 서비스 단위 테스트를 모두 새롭게 작성했습니다.

기존 Service 단위 테스트에서 Mock을 사용하므로 인해, 비즈니스 로직을 테스트 하는 것이 아닌 그저 Stub을 테스트하는 테스트가 대다수 였는데, 이번 변화로 인해 Service에서 수행하는 비즈니스 로직을 테스트할 수 있게 되었습니다.

Before

class StudentServiceTest {
    ...
    @Test
    void 성공() {
        // given
        Long memberId = 1L;
        StudentVerificateRequest request = new StudentVerificateRequest("123456");
        Member member = MemberFixture.member().build();
        given(memberRepository.findById(anyLong()))
            .willReturn(Optional.of(member));
        given(studentCodeRepository.findByCodeAndMember(any(), any()))
            .willReturn(Optional.of(new StudentCode(
                new VerificationCode("123456"),
                new School("snu.ac.kr", "서울대학교"),
                member,
                "ohs",
                LocalDateTime.now()
            )));

        // when & then
        assertThatNoException()
            .isThrownBy(() -> studentService.verify(memberId, request));
    }
    ...
}

After

class StudentServiceTest {
    ...
    @Test
    void 성공하면_학생이_저장되고_StudentCode가_삭제된다() {
        // given
        Long memberId = member.getId();
        studentCodeRepository.save(
            new StudentCode(new VerificationCode(request.code()), school, member, "glen",
                LocalDateTime.now(clock)));

        // when
        studentService.verify(memberId, request);

        // when & then
        assertSoftly(softly -> {
            softly.assertThat(studentRepository.existsByMemberId(memberId)).isTrue();
            softly.assertThat(studentCodeRepository.findByMemberId(memberId)).isEmpty();
        });
    }
    ...
}

단점으로는, Repository를 새롭게 생성할 때 Fake 클래스를 만들어야 합니다.
또한 일부 Repository에 Pageable, Filter 등이 인자로 받는 경우가 있습니다.
해당 인자를 받는 Fake Repository를 구현하기가 거의 불가능 합니다. (JPA, QueryDSL 등 구현체에 의존적이므로)
따라서 다른 구현체에 의존적인 Repository의 경우 별도의 Repository 클래스로 만드는 것이 좋아보입니다. ex) QueryRepository
제안이니까 편하게 의견주세요!!

- 일부 Optional이 필요한 비즈니스 로직은 유지

github-actions · 2024-01-10T11:54:14Z

Test Results

96 files 96 suites 6s ⏱️
338 tests 338 ✅ 0 💤 0 ❌
352 runs 352 ✅ 0 💤 0 ❌

Results for commit 6e51df2.

♻️ This comment has been updated with latest results.

seokjin8678 · 2024-01-10T12:21:53Z

backend/src/main/java/com/festago/auth/application/AuthService.java

@@ -35,9 +33,9 @@ private Member signUp(UserInfo userInfo) {
        return memberRepository.save(userInfo.toMember());
    }

+    // TODO FCM 삭제를 위해 DeleteMemberEvent 발행 고려


나중에 해야 할 작업입니다.
추후 MemberFCM API가 분리된다면 이벤트를 발행해야 할 것 같네요!

seokjin8678 · 2024-01-10T12:22:17Z

backend/src/main/java/com/festago/entry/domain/EntryCodePayload.java

+// TODO Record 클래스로 바꿔도 좋을듯
 public class EntryCodePayload {


해당 클래스는 Record로 바꿔도 좋아 보입니다!

seokjin8678 · 2024-01-10T12:22:36Z

backend/src/main/java/com/festago/fcm/application/MemberFCMService.java

+    // TODO @Slf4j 어노테이션 사용하면 어떨까요
    private static final Logger log = LoggerFactory.getLogger(MemberFCMService.class);


Slf4j 어노테이션을 사용하는게 어떨까요?

진행을 진행시켜!!

seokjin8678 · 2024-01-10T12:24:18Z

backend/src/main/java/com/festago/fcm/application/MemberFCMService.java

@@ -63,6 +68,7 @@ private Long extractMemberId(String accessToken) {
        return authPayload.getMemberId();
    }

+    // TODO 사용하지 않는 메서드 같음
    @Async
    public void deleteMemberFCM(Long memberId) {
        memberFCMRepository.deleteAllByMemberId(memberId);


해당 메서드의 사용처를 보니, 테스트 코드 말고는 없더군요.
FCM API 분리를 할 때 지워야 할 듯 합니다.

seokjin8678 · 2024-01-10T12:25:15Z

backend/src/main/java/com/festago/festival/application/FestivalService.java

@@ -27,13 +26,13 @@
 public class FestivalService {

    private final FestivalRepository festivalRepository;
+    // TODO FestivalStageService를 FestivalController가 의존하게 하는게 좋을듯?


FestivalService가 FestivalStageService를 의존중입니다.
단순히 조회 로직을 위힘하므로, Service에 의존시키는 것이 아닌, Controller에 의존시켜도 좋을 것 같습니다.

FestivalStageService 보니 이 Serivce 는 예측하기 힘든 서비스인 것 같습니다.

그래서 저는 CQRS를 적용하고 컨벤션으로 Controller 는 ContextCommandService, ContextQueryService 만 접근이 가능하도록 제한하는 쪽으로 가는 것이 어떨까 생각이 듭니다.

BGuga

안녕하세요 글렌 🍤
작업 하시느라 고생하셨습니다!!

BGuga · 2024-01-10T14:16:51Z

backend/src/main/java/com/festago/fcm/application/MemberFCMService.java

+    // TODO @Slf4j 어노테이션 사용하면 어떨까요
    private static final Logger log = LoggerFactory.getLogger(MemberFCMService.class);


진행을 진행시켜!!

BGuga · 2024-01-10T14:21:15Z

backend/src/main/java/com/festago/fcm/application/MemberFCMService.java

+    // TODO 사용하지 않는 메서드 같음
+    // 비즈니스 파라미터의 isNewMember가 애매한 것 같음.
+    // 다른 회원이 같은 FCM 토큰을 저장하면??
+    // existsByFcmToken으로 Validation 고려해도 좋을듯 ex) 이미 사용중인 토큰입니다.


EventListener로 해당 로직이 이동해서 삭제해야 하네요.👍

BGuga · 2024-01-10T14:35:13Z

backend/src/main/java/com/festago/festival/application/FestivalService.java

@@ -27,13 +26,13 @@
 public class FestivalService {

    private final FestivalRepository festivalRepository;
+    // TODO FestivalStageService를 FestivalController가 의존하게 하는게 좋을듯?


FestivalStageService 보니 이 Serivce 는 예측하기 힘든 서비스인 것 같습니다.

그래서 저는 CQRS를 적용하고 컨벤션으로 Controller 는 ContextCommandService, ContextQueryService 만 접근이 가능하도록 제한하는 쪽으로 가는 것이 어떨까 생각이 듭니다.

BGuga · 2024-01-10T14:49:31Z

backend/src/test/java/com/festago/admin/repository/MemoryAdminRepository.java


 public class MemoryAdminRepository implements AdminRepository {

    private final ConcurrentHashMap<Long, Admin> memory = new ConcurrentHashMap<>();
    private final AtomicLong autoIncrement = new AtomicLong();

+    @SneakyThrows


Lombok이 제공하는 어노테이션인데, 리플렉션으로 private 필드(Id)를 수정하려면 NoSuchFieldException Checked 예외를 처리해야 하기 때문에 사용했습니다!

BGuga · 2024-01-10T14:55:01Z

backend/src/test/java/com/festago/ticket/application/TicketServiceTest.java

+        Stage stage = StageFixture.stage().build();
+        stageRepository.save(stage);
+
+        Ticket studentTicket = TicketFixture.ticket().ticketType(TicketType.STUDENT).stage(stage).build();
+        ticketRepository.save(studentTicket);
+        Ticket visitorTicket = TicketFixture.ticket().ticketType(TicketType.VISITOR).stage(stage).build();
+        ticketRepository.save(visitorTicket);


mocking 보다 fake 를 선택하신 이유가 궁금합니다!!

기존 Service 단위 테스트에서 Mock을 사용하므로 인해, 비즈니스 로직을 테스트 하는 것이 아닌 그저 Stub을 테스트하는 테스트가 대다수 였는데, 이번 변화로 인해 Service에서 수행하는 비즈니스 로직을 테스트할 수 있게 되었습니다.

저는 mocking 을 통해서 Service 내의 비즈니스 로직을 테스트하고 있다 생각했었습니다🍭

public void verify(Long memberId, StudentVerificateRequest request) { validateStudent(memberId); Member member = findMember(memberId); VerificationCode verificationCode = new VerificationCode(request.code()); StudentCode studentCode = studentCodeRepository.findByCodeAndMember(verificationCode, member) .orElseThrow(() -> new BadRequestException(ErrorCode.INVALID_STUDENT_VERIFICATION_CODE)); studentRepository.save(new Student(member, studentCode.getSchool(), studentCode.getUsername())); studentCodeRepository.deleteByMember(member); }

위의 비즈니스 로직을 수행하면 어떠한 결과를 기대하나요?

저는 해당 비즈니스 로직의 결과로 StudentRepository에 회원이 저장되어야 하고, StudentCodeRepository에 Member에 해당하는 엔티티가 삭제되어야 한다고 봅니다.

그리고 테스트 코드에서 비즈니스 로직이 표현되고 있는지 물어본다면 저는 잘 모르겠습니다. 😂

@Test void 성공() { // given Long memberId = 1L; StudentVerificateRequest request = new StudentVerificateRequest("123456"); Member member = MemberFixture.member().build(); given(memberRepository.findById(anyLong())) .willReturn(Optional.of(member)); given(studentCodeRepository.findByCodeAndMember(any(), any())) .willReturn(Optional.of(new StudentCode( new VerificationCode("123456"), new School("snu.ac.kr", "서울대학교"), member, "ohs", LocalDateTime.now() ))); // when & then assertThatNoException() .isThrownBy(() -> studentService.verify(memberId, request)); }

Mock을 사용하면 테스트는 단순해지고, 성공이 쉽겠지만 코드만 봤을 때 어떠한 결과를 기대하는지 알기가 힘듭니다.
특히 Repository와 관련된 로직일 경우 테스트 코드가 그저 Stub을 정의하는 테스트 밖에 되지 않습니다.

@Test void 무대_생성() { // given Festival festival = FestivalFixture.festival() .build(); StageCreateRequest request = new StageCreateRequest( LocalDateTime.now(), "애쉬,푸우,오리,글렌", LocalDateTime.now().minusDays(1), 1L ); given(festivalRepository.findById(anyLong())) .willReturn(Optional.of(festival)); given(stageRepository.save(any(Stage.class))) .willAnswer(invocation -> invocation.getArgument(0)); // when StageResponse response = stageService.create(request); // then assertThat(response.startTime()).isEqualTo(request.startTime()); }

해당 테스트를 Fake로 바꾼다면 다음과 같은 테스트 코드가 됩니다.

@Test void 무대_생성() { // given Festival festival = FestivalFixture.festival() .build(); festivalRepository.save(festival); StageCreateRequest request = new StageCreateRequest( LocalDateTime.now(), "애쉬,푸우,오리,글렌", LocalDateTime.now().minusDays(1), festival.getId() ); // when StageResponse response = stageService.create(request); // then assertThat(stageRepository.findById(response.id())).isPresent(); }

저와 비슷한 생각을 가진 글이 있어서 공유합니다!
https://sungjk.github.io/2023/09/10/effective-stub.html

carsago

고생하셨습니다 글렌.

PR은 이틀전 쯤 읽었는데, 테스트 코드를 어�떻게 작성해야할까?라는 부분을 꽤 고민하게 돼서 리뷰가 늦었습니다.

제가 했던 고민은 구체적으로 "Service Layer에서 DB를 통하지 않는 테스트가 큰 의미가 있나?"입니다.
다르게 말하면 Serivce Layer의 단위 테스트에 크게 의의가 없는 것처럼 여겨집니다.

대부분 로직이 DB에 의존적이고, 특히 단순한 로직들일 경우 그저 Repository method를 통해 쿼리를 호출할 뿐인데 여기서 mocking을 하든 fake를 하든 큰 의미가 없는 것처럼만 생각이 들어서, 구현한 핵심 기능을 정확하게 검증하기 위해서 테스트 코드를 작성한 것이 아니라 테스트를 하기 위해 테스트 코드를 작성한 느낌이 듭니다.

그래서 Serivce Layer는 단위 테스트를 최소화하거나 없애고, DB와의 통합 테스트를 통해 검증하는 것이 좋다고 느껴집니다(제 주관이에요).

첫번째는 테스트도 유지 보수의 대상이기 때문에, 큰 의미가 없는 테스트가 존재하는 것이 낭비라고 여겨지는 것
두번째는 통합테스트를 해야 Repository에서 자동 생성된 메소드(쿼리)가 우리가 의도한 방식으로 작동하는지 확인할 수 있다는 것
그리고 JPA라는 기술을 사용하는 상황에서 통합 테스트를 해야 변경감지 같은 JPA에 의존적인 부분을 정확하게 테스트할 수 있다는 생각이 드네요.

이런 관점에서 Controller 단에서 하위 레이어를 모킹하는 단위 테스트는 유의미하다고 여겨지네요.
URL 매핑, status code나 null 체크, 필요하다면 Response까지 검증 할 수 있으니

carsago · 2024-01-13T04:36:20Z

backend/src/main/java/com/festago/auth/application/AuthService.java

-        Member member = memberRepository.findById(memberId)
-            .orElseThrow(() -> new NotFoundException(ErrorCode.MEMBER_NOT_FOUND));
+        Member member = memberRepository.getOrThrow(memberId);


이런거 좋아요

seokjin8678 · 2024-01-13T06:34:02Z

고생하셨습니다 글렌.

PR은 이틀전 쯤 읽었는데, 테스트 코드를 어�떻게 작성해야할까?라는 부분을 꽤 고민하게 돼서 리뷰가 늦었습니다.

제가 했던 고민은 구체적으로 "Service Layer에서 DB를 통하지 않는 테스트가 큰 의미가 있나?"입니다. 다르게 말하면 Serivce Layer의 단위 테스트에 크게 의의가 없는 것처럼 여겨집니다.

대부분 로직이 DB에 의존적이고, 특히 단순한 로직들일 경우 그저 Repository method를 통해 쿼리를 호출할 뿐인데 여기서 mocking을 하든 fake를 하든 큰 의미가 없는 것처럼만 생각이 들어서, 구현한 핵심 기능을 정확하게 검증하기 위해서 테스트 코드를 작성한 것이 아니라 테스트를 하기 위해 테스트 코드를 작성한 느낌이 듭니다.

그래서 Serivce Layer는 단위 테스트를 최소화하거나 없애고, DB와의 통합 테스트를 통해 검증하는 것이 좋다고 느껴집니다(제 주관이에요).

첫번째는 테스트도 유지 보수의 대상이기 때문에, 큰 의미가 없는 테스트가 존재하는 것이 낭비라고 여겨지는 것 두번째는 통합테스트를 해야 Repository에서 자동 생성된 메소드(쿼리)가 우리가 의도한 방식으로 작동하는지 확인할 수 있다는 것 그리고 JPA라는 기술을 사용하는 상황에서 통합 테스트를 해야 변경감지 같은 JPA에 의존적인 부분을 정확하게 테스트할 수 있다는 생각이 드네요.

이런 관점에서 Controller 단에서 하위 레이어를 모킹하는 단위 테스트는 유의미하다고 여겨지네요. URL 매핑, status code나 null 체크, 필요하다면 Response까지 검증 할 수 있으니

저도 그래서 통합 테스트의 비율을 높이는게 오히려 좋겠다는 생각이 들더라구요. 😂

seokjin8678 · 2024-02-20T08:06:30Z

점진적으로 개선하면 좋을 것 같아 close 처리합니다!

seokjin8678 added 2 commits January 10, 2024 05:07

refactor: findById -> getOrThrow 변경

56b6fd8

- 일부 Optional이 필요한 비즈니스 로직은 유지

test: Service 단위 테스트에서 Mock Repository -> Fake Repository 사용하도록 변경

b16a0f6

seokjin8678 added BE 백엔드에 관련된 작업 ⚙️ 리팩터링 리팩터링에 관련된 작업 🧪 테스트 테스트에 관련된 작업 🙋‍♀️ 제안 제안에 관한 작업 labels Jan 10, 2024

seokjin8678 self-assigned this Jan 10, 2024

seokjin8678 linked an issue Jan 10, 2024 that may be closed by this pull request

[BE] Repository를 개선한다. #648

Closed

seokjin8678 requested review from xxeol2, carsago and BGuga January 10, 2024 11:53

seokjin8678 commented Jan 10, 2024

View reviewed changes

BGuga requested changes Jan 10, 2024

View reviewed changes

refactor: TODO 주석 삭제

6e51df2

carsago approved these changes Jan 13, 2024

View reviewed changes

seokjin8678 changed the title ~~refactor: Repository 개선 및 서비스 단위 테스트 Fake 사용 (#648)~~ [BE] refactor: Repository 개선 및 서비스 단위 테스트 Fake 사용 (#648) Jan 25, 2024

seokjin8678 closed this Feb 20, 2024

seokjin8678 deleted the feat/#648 branch April 8, 2024 13:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BE] refactor: Repository 개선 및 서비스 단위 테스트 Fake 사용 (#648) #649

[BE] refactor: Repository 개선 및 서비스 단위 테스트 Fake 사용 (#648) #649

seokjin8678 commented Jan 10, 2024

github-actions bot commented Jan 10, 2024 •

edited

Loading

seokjin8678 Jan 10, 2024

seokjin8678 Jan 10, 2024

seokjin8678 Jan 10, 2024

BGuga Jan 10, 2024

seokjin8678 Jan 10, 2024

seokjin8678 Jan 10, 2024

BGuga Jan 10, 2024

BGuga left a comment

BGuga Jan 10, 2024

BGuga Jan 10, 2024

BGuga Jan 10, 2024

BGuga Jan 10, 2024

seokjin8678 Jan 10, 2024

BGuga Jan 10, 2024

BGuga Jan 10, 2024

seokjin8678 Jan 10, 2024 •

edited

Loading

seokjin8678 Jan 11, 2024

carsago left a comment

carsago Jan 13, 2024

seokjin8678 commented Jan 13, 2024

seokjin8678 commented Feb 20, 2024

		// TODO Record 클래스로 바꿔도 좋을듯
		public class EntryCodePayload {

		// TODO @Slf4j 어노테이션 사용하면 어떨까요
		private static final Logger log = LoggerFactory.getLogger(MemberFCMService.class);

[BE] refactor: Repository 개선 및 서비스 단위 테스트 Fake 사용 (#648) #649

[BE] refactor: Repository 개선 및 서비스 단위 테스트 Fake 사용 (#648) #649

Conversation

seokjin8678 commented Jan 10, 2024

📌 관련 이슈

✨ PR 세부 내용

Before

After

github-actions bot commented Jan 10, 2024 • edited Loading

Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BGuga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seokjin8678 Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carsago left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seokjin8678 commented Jan 13, 2024

seokjin8678 commented Feb 20, 2024

github-actions bot commented Jan 10, 2024 •

edited

Loading

seokjin8678 Jan 10, 2024 •

edited

Loading