TemplateServiceTest 추가 #661

HoeSeong123 · 2024-09-18T00:44:17Z

⚡️ 관련 이슈

close #645

📍주요 변경 사항

TemplateService 테스트 작성

🎸기타

테스트 실패 문제

켬미와 동일하게 flyway 설정 때문에 테스트 코드가 자꾸 실패하는 문제가 발생했었습니다.
이에 테스트 코드에서 사용하는 application-db.yml에 다음과 같은 설정을 추가해주었습니다.

flyway:
  enabled: false

중복되는 테스트 코드

TemplateService의 경우 아래 코드와 같이 TemplateRepository의 반환값을 그대로 리턴하는 메소드들이 몇 개 있습니다.

public Template getById(Long id) {
    return templateRepository.fetchById(id);
}

public List<Template> getByMemberId(Long memberId) {
    return templateRepository.findByMemberId(memberId);
}

public Page<Template> findAll(
        Long memberId, String keyword, Long categoryId, List<Long> tagIds, Pageable pageable
) {
    return templateRepository.findAll(
            new TemplateSpecification(memberId, keyword, categoryId, tagIds), pageable);
}

이런 경우 RepositoryTest와 중복되는 테스트들이 몇 개 생겨나는데 이런 경우에도 ServiceTest를 작성해야 하는지 의문입니다.
- 우선 저는 중복되더라도 테스트 코드를 작성하였습니다.
- 테스트 코드는 해당 클래스의 명세서와도 같고, 테스트 코드를 봤을 때 해당 클래스를 어떻게 사용하는거구나, 어떤 에러가 발생할 수 있구나 등을 알 수 있게 하는 것이 목적이라고 생각하기 때문입니다.
- 다른 분들의 의견이 궁금합니다.

지연 로딩 문제

[REFACTOR] Service Layer 리팩토링 #650 (comment)
저도 마찬가지로 짱수와 동일한 프록시 객체 접근 문제가 발생합니다.
몰리와 동일하게 @Transactional 어노테이션을 통해 트랜잭션을 생성하는 방식으로 처리했는데, 우선은 프로덕션 코드는 수정되면 안 될 것 같아서 테스트 코드에 @Transactional을 추가하는 식으로 해결하였습니다.
추후에 해당 어노테이션을 지우고, 프로덕션 코드에 적용하는 식으로 변경해야 할 것 같습니다.

기타 리팩토링 사항

TemplateService로 넘어오는 파라미터 변경
- [REFACTOR] Service Layer 리팩토링 #650 (comment)
리스트를 반환하는 JPA 쿼리 메서드명 변경
- [REFACTOR] Service Layer 리팩토링 #650 (comment)

kyum-q

초롱 검색 케이스까지 철저하게 테스트 구현해주셨네요 ~~ 수고하셨어요 ~! 굿 !
간단한 코멘트 남겨뒀습니다. 확인부탁드려요 !

kyum-q · 2024-09-18T14:38:47Z

backend/src/test/java/codezap/template/service/TemplateServiceTest.java

+            assertAll(
+                    () -> assertThat(actual.getTitle()).isEqualTo(templateRequest.title()),
+                    () -> assertThat(actual.getMember()).isEqualTo(member),
+                    () -> assertThat(actual.getCategory()).isEqualTo(category)
+            );


description이랑 소스코드, 썸네일 순서, 태그도 확인하면 좋을 것 같아요 !

추가적으로 service에서 반환하는 객체만 비교하고, 데이터베이스에 실제로 추가되었는지 체크 안해도 될까요?

레포지토리 테스트가 아니라서 정답은 못찾겠지만, 저는 실제 데이터베이스에 추가되었는지 체크하는 로직이 있으면 더 좋을 것 같아요 ! (제 코드도 한번 더 확인해봐야겠네요..)

이에 대해 초롱과 다른 사람들은 어떻게 생각하는지 궁금합니다 !

Repository 테스트를 항상 잘 작성한다면 굳이 체크를 할 필요가 없을 것 같기는 합니다만....
저도 고민이 되네요.
서비스 계층의 특별한 역할이 존재하는 것이 아니라, 거의 Repository 메서드로 포워딩 하는 느낌이라서 무엇을 테스트 해야 하나 하는 고민도 들고요.

저는 데이터베이스에 추가되는지 확인하는 로직은 있으면 좋고, 없어도 그만~ 의 느낌인 것 같네요!

저는 findAll 메서드의 결과만 놓고 검증해야 한다고 생각합니다.

구체적인 로직을 몰라도 테스트를 할 수 있어야 한다고 생각하기 때문입니다.

저도 제우스 의견에 동의합니다.
해당 메소드에 대한 테스트는 어떤 입력을 했을 때 이런 출력이 나와를 테스트하는 거지, 내부의 로직이 전부 정상적으로 동작하는지를 확인하는 것은 아니라고 생각합니다.

description이랑 소스코드, 썸네일 순서, 태그도 확인하면 좋을 것 같아요 !

썸네일 순서와 태그는 Template이 가지고 있는 필드가 아니어서 검사 로직에서 제외했습니다.
sourceCode의 경우는 양방향으로 연결되어 있어 sourceCode가 저장되어 있지 않으면 값이 들어오지 않습니다. 따라서 제외했습니다.
description은 추가하였습니다.

backend/src/test/java/codezap/template/service/TemplateServiceTest.java

jminkkk

안녕하세요 초롱~
수고하셨습니다 🔥
확인 부탁드려요 ㅎㅎ

jminkkk · 2024-09-19T04:49:11Z

backend/src/test/java/codezap/template/service/TemplateServiceTest.java

-        CreateTemplateRequest createdTemplate = makeTemplateRequest("title");
-        saveTemplate(createdTemplate, new Category("category1", member), member);
+        @Test
+        @DisplayName("멤버 ID로 템플릿 조회 성공: 해당하는 템플릿이 없는 경우")


Suggested change

@DisplayName("멤버 ID로 템플릿 조회 성공: 해당하는 템플릿이 없는 경우")

@DisplayName("멤버 ID로 템플릿 조회 성공: 해당하는 템플릿이 없는 경우 빈 목록을 반환")

jminkkk · 2024-09-19T04:49:58Z

backend/src/test/java/codezap/template/service/TemplateServiceTest.java

+    @DisplayName("검색 기능")
+    class FindAll {


Suggested change

@DisplayName("검색 기능")

class FindAll {

@DisplayName("템플릿 목록 조회")

class FindAll {

몰리가 작성했던 테스트들을 그대로 가져왔던 것들인데 검색 기능이 더 명확하지 않나요?
템플릿 목록 조회도 하기는 하지만, 옵션에 따라 다르게 반환하는데 단순히 템플릿 목록 조회라고 하면 명확하지 못 한 것 같습니다.

맞아요~ 요청드린 것 중에 여러개가 제가 작성한 것도 알고 있어요
그래서 제가 더 정정해야겠다고 생각했어요

저는 템플릿을 조회하는 기능 중에 키워드 검색이 있는 것 같다고 생각을 했었어요.
또 검색 보다는 조회가 더 큰 범위라고 생각해서 정정 부탁드렸습니답...

마이페이지에서 내 글만 조회를 하거나, 필터링이 전혀 없이 단순 GET /templates 을 한다고 생각을 했을 때 오히려 조회가 더 명확할 것 같아서요. 그런데 사람마다 다르게 느껴질 수도 있겠네요 🤔

제 의견에 대한 설명이 없어서 의도와 다르게 전달된 것 같아서 의견 전달만 명확하게 드려봅니다~ 🔥

jminkkk · 2024-09-19T04:50:44Z

backend/src/test/java/codezap/template/service/TemplateServiceTest.java

+        private Template template1, template2, template3;
+
+        @Test
+        @DisplayName("검색 기능: 회원 ID로 템플릿 조회 성공")


Suggested change

@DisplayName("검색 기능: 회원 ID로 템플릿 조회 성공")

@DisplayName("템플릿 목록 조회 성공: 특정 회원의 템플릿 목록 조회")

jminkkk · 2024-09-19T04:51:54Z

backend/src/test/java/codezap/template/service/TemplateServiceTest.java

+            String keyword = null;
+            Long categoryId = null;
+            List<Long> tagIds = null;
+            Pageable pageable = PageRequest.of(0, 10);
+
+            Page<Template> actual = sut.findAll(memberId, keyword, categoryId, tagIds, pageable);


Suggested change

String keyword = null;

Long categoryId = null;

List<Long> tagIds = null;

Pageable pageable = PageRequest.of(0, 10);

Page<Template> actual = sut.findAll(memberId, keyword, categoryId, tagIds, pageable);

Pageable pageable = PageRequest.of(0, 10);

Page<Template> actual = sut.findAll(memberId, null, null, null, pageable);

(다른 메서드도 포함) 변수 할당을 안해도 의미가 충분히 보일 것 같아요!

저는 조금 반대입니다. 다른 테스트들도 같이 본다면 변수 할당을 하지 않아도 어떤 변수들에 null이 들어간건지 바로 알 수 있지만, 해당 테스트 코드만을 봤을 때는 알기 힘들다고 생각해요

초롱이 잘 보일 것 같다고 생각하면 유지하는 게 맞는 것 같아요 최고 🔥

jminkkk · 2024-09-19T04:57:07Z

backend/src/test/java/codezap/template/service/TemplateServiceTest.java

+                            .containsExactlyInAnyOrder(
+                                    templateRepository.fetchById(1L),
+                                    templateRepository.fetchById(2L)),


이것도 취향 차이지만...
한줄로 적어도 좋을 것 같아요!
위의 allMatch보다 길이가 짧아서 가능할 것 같은...??

Suggested change

.containsExactlyInAnyOrder(

templateRepository.fetchById(1L),

templateRepository.fetchById(2L)),

.containsExactlyInAnyOrder(templateRepository.fetchById(1L),templateRepository.fetchById(2L)),

이거...한 줄로 쓰면 자동으로 줄바꿈 됩니다..😭

jminkkk · 2024-09-19T04:57:35Z

backend/src/test/java/codezap/template/service/TemplateServiceTest.java

+                                    template.getMember().getId().equals(member1.getId())
+                                            && (template.getTitle().contains(keyword) || template.getDescription()
+                                            .contains(keyword)))
+            );


Suggested change

template.getMember().getId().equals(member1.getId())

&& (template.getTitle().contains(keyword) || template.getDescription()

.contains(keyword)))

);

template.getMember().getId().equals(member1.getId())

&& (template.getTitle().contains(keyword)

|| template.getDescription().contains(keyword)))

);

으엥 이렇게 보니까 이상한 것 같긴 한데 .. 암튼 요런 느낌.. 어떨까요?

저도 몰리 의견에 한 표 추가합니다!

6.4. 줄바꿈 허용 위치
네이버 핵데이 컨벤션인데요. 여기서도 연산자 앞에서 줄바꿈을 규칙으로 지정하고 있어요.

조건 연산자로 연결된 줄이 길어질 경우, 조건 연산자 앞에서 개행을 해주면 훨씬 가독성이 좋아지는 것 같네용

해당 테스트 코드들을 TemplateJpaRepositoryTest에서 가져와서..
줄바꿈 수정하는 김에 해당 클래스도 같이 수정해서 올리겠습니다.

jminkkk · 2024-09-19T04:59:03Z

backend/src/test/java/codezap/template/service/TemplateServiceTest.java

+                    () -> assertThat(actual.getContent()).containsExactlyInAnyOrder(templateRepository.fetchById(1L),
+                            templateRepository.fetchById(2L))


⚡️ 전체적으로 통일을 위한 확인이 필요할 것 같아용

zangsu

고생하셨어요 초롱! 몇가지 코멘트 남겨 두었으니 확인 부탁드려요~~~

zangsu · 2024-09-19T00:44:59Z

backend/src/test/java/codezap/template/service/TemplateServiceTest.java

+            assertAll(
+                    () -> assertThat(actual.getTitle()).isEqualTo(templateRequest.title()),
+                    () -> assertThat(actual.getMember()).isEqualTo(member),
+                    () -> assertThat(actual.getCategory()).isEqualTo(category)
+            );


Repository 테스트를 항상 잘 작성한다면 굳이 체크를 할 필요가 없을 것 같기는 합니다만....
저도 고민이 되네요.
서비스 계층의 특별한 역할이 존재하는 것이 아니라, 거의 Repository 메서드로 포워딩 하는 느낌이라서 무엇을 테스트 해야 하나 하는 고민도 들고요.

저는 데이터베이스에 추가되는지 확인하는 로직은 있으면 좋고, 없어도 그만~ 의 느낌인 것 같네요!

backend/src/test/java/codezap/template/service/TemplateServiceTest.java

zangsu · 2024-09-19T06:44:48Z

backend/src/test/java/codezap/template/service/TemplateServiceTest.java

+                                    template.getMember().getId().equals(member1.getId())
+                                            && (template.getTitle().contains(keyword) || template.getDescription()
+                                            .contains(keyword)))
+            );


저도 몰리 의견에 한 표 추가합니다!

6.4. 줄바꿈 허용 위치
네이버 핵데이 컨벤션인데요. 여기서도 연산자 앞에서 줄바꿈을 규칙으로 지정하고 있어요.

조건 연산자로 연결된 줄이 길어질 경우, 조건 연산자 앞에서 개행을 해주면 훨씬 가독성이 좋아지는 것 같네용

zeus6768

테스트 코드가 정말 많네요. 작성하느라 고생 많았어요 😭

다른 분들이 변경사항에 대해 충분히 언급하신 것 같아, 저는 의견만 이모지로 표현했습니다.

테스트 코드는 해당 클래스의 명세서와도 같고, 테스트 코드를 봤을 때 해당 클래스를 어떻게 사용하는거구나, 어떤 에러가 발생할 수 있구나 등을 알 수 있게 하는 것이 목적이라고 생각하기 때문입니다.

테스트 코드는 클래스의 명세가 아니라 기능의 명세라고 정정하고 싶어요.

클래스의 명세라는 관점으로 접근하면, 그 클래스의 구현까지 고려해야할 것 같다는 생각이 들기 때문입니다.

반면 저는 원하는 기능만 올바르게 동작하는지 확인하면 된다고 생각해요.

서비스는 DB에 의존하고 있으니 서비스 테스트에서 DB에 잘 저장되었는지까지 확인해야 할까요?

그렇다면 어떤 객체를 테스트할 때 그 객체의 모든 의존성이 발생할 수 있는 예외를 검증해야 한다는 말인데, 이건 말이 안 된다고 생각해요.

단위 테스트 책에서 리팩토링 내성에 관한 챕터를 읽어보면 좋을 것 같습니다.

다른 의견 있다면 답변 부탁드려요~!

zeus6768 · 2024-09-19T16:30:06Z

backend/src/test/java/codezap/template/service/TemplateServiceTest.java

+            assertAll(
+                    () -> assertThat(actual.getTitle()).isEqualTo(templateRequest.title()),
+                    () -> assertThat(actual.getMember()).isEqualTo(member),
+                    () -> assertThat(actual.getCategory()).isEqualTo(category)
+            );


저는 findAll 메서드의 결과만 놓고 검증해야 한다고 생각합니다.

구체적인 로직을 몰라도 테스트를 할 수 있어야 한다고 생각하기 때문입니다.

zeus6768 · 2024-09-19T16:39:05Z

backend/src/test/java/codezap/template/service/TemplateServiceTest.java

-        assertThat(templateRepository.findAll()).isEmpty();
+    @Nested
+    @DisplayName("검색 기능")
+    class FindAll {


pageable이 있는 경우와 없는 경우를 먼저 테스트해서 Pageable이 필수 인자라는 것을 보여주면 좋을 것 같아요.

좋은 테스트네요!! 다만 현재는 Pageable에 대한 null 체크가 없어서 NPE가 발생하고 있습니다.
추후에 null 체크 로직이 필요할 것 같아서 @disabled 처리해놨습니다!

zeus6768

고생 많았습니다 🚀

kyum-q

초롱 수고하셨어요 ~

zangsu

고생했습니다 초롱~!
이번 미션도 여기서 approve 하겠습니다!
다음 미션도 파이팅입니다 🔥

jminkkk

안녕하세요 초롱 ~
리뷰 반영된 것 같아 어프루브 합니다! 최고 👍

별개로 리뷰 중 제 의도와 다르게 전달된 부분들이 있는 것 같아서 코멘트를 남겼는데 확인해주시면 좋을 것 같아요~!

jminkkk · 2024-09-20T06:52:19Z

backend/src/test/java/codezap/template/service/TemplateServiceTest.java

+            String keyword = null;
+            Long categoryId = null;
+            List<Long> tagIds = null;
+            Pageable pageable = PageRequest.of(0, 10);
+
+            Page<Template> actual = sut.findAll(memberId, keyword, categoryId, tagIds, pageable);


초롱이 잘 보일 것 같다고 생각하면 유지하는 게 맞는 것 같아요 최고 🔥

jminkkk · 2024-09-20T11:08:44Z

backend/src/test/java/codezap/template/service/TemplateServiceTest.java

+    @DisplayName("검색 기능")
+    class FindAll {


맞아요~ 요청드린 것 중에 여러개가 제가 작성한 것도 알고 있어요
그래서 제가 더 정정해야겠다고 생각했어요

저는 템플릿을 조회하는 기능 중에 키워드 검색이 있는 것 같다고 생각을 했었어요.
또 검색 보다는 조회가 더 큰 범위라고 생각해서 정정 부탁드렸습니답...

마이페이지에서 내 글만 조회를 하거나, 필터링이 전혀 없이 단순 GET /templates 을 한다고 생각을 했을 때 오히려 조회가 더 명확할 것 같아서요. 그런데 사람마다 다르게 느껴질 수도 있겠네요 🤔

제 의견에 대한 설명이 없어서 의도와 다르게 전달된 것 같아서 의견 전달만 명확하게 드려봅니다~ 🔥

test: 템플릿 서비스 테스트코드 작성

ea5a4c3

HoeSeong123 added refactor 요구사항이 바뀌지 않은 변경사항 BE 백엔드 labels Sep 18, 2024

HoeSeong123 added this to the 5차 스프린트🍗 milestone Sep 18, 2024

HoeSeong123 requested review from zangsu, zeus6768, jminkkk and kyum-q September 18, 2024 00:44

HoeSeong123 self-assigned this Sep 18, 2024

kyum-q requested changes Sep 18, 2024

View reviewed changes

jminkkk requested changes Sep 19, 2024

View reviewed changes

zangsu reviewed Sep 19, 2024

View reviewed changes

zeus6768 reviewed Sep 19, 2024

View reviewed changes

HoeSeong123 added 6 commits September 20, 2024 13:21

refactor: 템플릿 생성 테스트 검증 추가

9848b6e

refactor: displayname 수정

07a0923

refactor: 줄바꿈 수정

d39e9a6

refactor: 멤버 별로 다른 카테고리를 갖도록 수정

8be36c3

refactor: 불필요한 코드 삭제

a65a565

refactor: 검색시 Pageable에 대한 null 검증 테스트 추가

f14dfe8

zeus6768 approved these changes Sep 20, 2024

View reviewed changes

kyum-q approved these changes Sep 20, 2024

View reviewed changes

zangsu approved these changes Sep 20, 2024

View reviewed changes

jminkkk reviewed Sep 20, 2024

View reviewed changes

jminkkk approved these changes Sep 20, 2024

View reviewed changes

kyum-q merged commit e107f89 into dev/be Sep 20, 2024
7 checks passed

kyum-q deleted the test/645-template-service branch September 20, 2024 13:27

zangsu mentioned this pull request Sep 27, 2024

[REFACTOR] 서비스 테스트 #645

Closed

	@DisplayName("멤버 ID로 템플릿 조회 성공: 해당하는 템플릿이 없는 경우")
	@DisplayName("멤버 ID로 템플릿 조회 성공: 해당하는 템플릿이 없는 경우 빈 목록을 반환")

	@DisplayName("검색 기능: 회원 ID로 템플릿 조회 성공")
	@DisplayName("템플릿 목록 조회 성공: 특정 회원의 템플릿 목록 조회")

		() -> assertThat(actual.getContent()).containsExactlyInAnyOrder(templateRepository.fetchById(1L),
		templateRepository.fetchById(2L))

TemplateServiceTest 추가 #661

TemplateServiceTest 추가 #661

Conversation

HoeSeong123 commented Sep 18, 2024 • edited Loading

⚡️ 관련 이슈

📍주요 변경 사항

🎸기타

테스트 실패 문제

중복되는 테스트 코드

지연 로딩 문제

기타 리팩토링 사항

kyum-q 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

HoeSeong123 Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

jminkkk 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

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

zangsu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeus6768 left a comment • edited Loading

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

zeus6768 left a comment

Choose a reason for hiding this comment

kyum-q left a comment

Choose a reason for hiding this comment

zangsu left a comment

Choose a reason for hiding this comment

jminkkk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HoeSeong123 commented Sep 18, 2024 •

edited

Loading

HoeSeong123 Sep 20, 2024 •

edited

Loading

zeus6768 left a comment •

edited

Loading