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

[BE] test: 도메인 연관관계 재설정 후 테스트 작성 #101

Merged
merged 14 commits into from
Jul 25, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@
import jakarta.persistence.Id;
import jakarta.persistence.ManyToOne;
import lombok.AccessLevel;
import lombok.EqualsAndHashCode;
import lombok.Getter;
import lombok.NoArgsConstructor;

@Entity
@NoArgsConstructor(access = AccessLevel.PROTECTED)
@EqualsAndHashCode(of = "id")
@Getter
public class GithubIdReviewerGroup {

Expand Down
9 changes: 7 additions & 2 deletions backend/src/main/java/reviewme/member/domain/Member.java
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,13 @@ public class Member {
@Embedded
private GithubId githubId;

public Member(String name, long githubId) {
public Member(Long id, String name, GithubId githubId) {
this.id = id;
this.name = name;
this.githubId = new GithubId(githubId);
this.githubId = githubId;
}

public Member(String name, long githubId) {
this(null, name, new GithubId(githubId));
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. id를 밖에서 넣어주는 생성자가 필요한 이유가 궁금해요~
  2. 추측으로는.. 테스트에서 id가 있는 멤버가 필요한 경우가 있는데, 그 때마다 매번 멤버를 db에 저장해서 id를 세팅해주는 게 번거로워서 그런걸까요?
  3. 만약 2번 이유라면, 이게 다른 개발자 입장에서 이걸 테스트만이 아닌 다른 용도로 잘못 쓸 수 있는 가능성을 열어준 건 아닐까 생각이 들어요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

수정했습니다 (위쪽 테드 의견 반영)

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import lombok.AccessLevel;
import lombok.Getter;
import lombok.NoArgsConstructor;
import reviewme.member.domain.exception.DescriptionLengthExceededException;
import reviewme.member.domain.exception.InvalidDescriptionLengthException;
import reviewme.member.domain.exception.InvalidGroupNameLengthException;
import reviewme.member.domain.exception.SelfReviewException;
import reviewme.review.domain.Review;
Expand Down Expand Up @@ -68,7 +68,7 @@ public ReviewerGroup(Member reviewee, List<GithubId> reviewerGithubIds,
throw new InvalidGroupNameLengthException(MAX_GROUP_NAME_LENGTH);
}
if (description.length() > MAX_DESCRIPTION_LENGTH) {
throw new DescriptionLengthExceededException(MAX_DESCRIPTION_LENGTH);
throw new InvalidDescriptionLengthException(MAX_DESCRIPTION_LENGTH);
}
if (reviewerGithubIds.contains(reviewee.getGithubId())) {
throw new SelfReviewException();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@

import reviewme.global.exception.BadRequestException;

public class DescriptionLengthExceededException extends BadRequestException {
public class InvalidDescriptionLengthException extends BadRequestException {

public DescriptionLengthExceededException(int maxLength) {
public InvalidDescriptionLengthException(int maxLength) {
super("리뷰어 그룹 설명은 %d자 이하로 작성해야 합니다.".formatted(maxLength));
}
}
10 changes: 7 additions & 3 deletions backend/src/test/java/reviewme/fixture/MemberFixture.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,24 @@

import lombok.Getter;
import lombok.RequiredArgsConstructor;
import reviewme.member.domain.GithubId;
import reviewme.member.domain.Member;

@RequiredArgsConstructor
@Getter
public enum MemberFixture {

회원_산초("산초", 1L),
회원_아루("아루", 2L),
회원_산초(1001L, "산초", 1L),
회원_아루(1002L, "아루", 2L),
회원_커비(1003L, "커비", 3L),
회원_테드(1004L, "테드", 4L),
;

private final long id;
Copy link
Contributor

Choose a reason for hiding this comment

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

회원 Fixture에서 id가 필요한 이유가 무엇인가요??

Copy link
Contributor Author

@donghoony donghoony Jul 25, 2024

Choose a reason for hiding this comment

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

이게 Review 내부 로직에서 같음을 판단할 때 ID를 사용하기 때문이에요. 이렇게 되면 도메인 로직보다는 서비스의 도움을 받는 것이 더 타당해 보이기도 합니다. 의견 자유롭게 이야기해보시죠 👍🏻

Copy link
Contributor

Choose a reason for hiding this comment

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

도메인만 놓고 봤을 때 id는 사실 데이터베이스에 유니크 값을 위한 값이기 때문에 데이터베이스 없이 도메인 만으로도 로직이 돌아가야 한다고 생각해요. 사실 멤버가 저장되지 않은 상태에서 사용할 일은 없겠지만, id가 없는 생성자도 존재하기에 id가 null 이 될 수도 있고요. 또 테스트 Fixture를 위해서 id값을 사용하는 생성자를 프로덕션 코드에 추가한 것도 조금 걸리긴 합니다..

github Id 중복이 없다면 id 대신 이러한 값을 사용하여 비교하는 것은 어떤가용?

Copy link
Contributor Author

@donghoony donghoony Jul 25, 2024

Choose a reason for hiding this comment

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

크흐 좋습니다! GithubId에서의 Equals 정의, Member에서의 id null인 경우 GithubId 비교하도록 했습니다.

private final String name;
private final long githubId;

public Member create() {
return new Member(name, githubId);
return new Member(id, name, new GithubId(githubId));
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

오오 이거 알잘딱깔센 픽스처네요👍

Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package reviewme.fixture;

import java.time.LocalDateTime;
import java.util.List;
import lombok.Getter;
import lombok.RequiredArgsConstructor;
import reviewme.member.domain.GithubId;
import reviewme.member.domain.Member;
import reviewme.member.domain.ReviewerGroup;

@RequiredArgsConstructor
@Getter
public enum ReviewerGroupFixture {

데드라인_남은_그룹("데드라인 이전 그룹", "설명", LocalDateTime.now().plusDays(1)),
데드라인_지난_그룹("데드라인 지난 그룹", "설명", LocalDateTime.now().minusDays(1)),
;

private final String groupName;
private final String description;
private final LocalDateTime deadline;

public ReviewerGroup create(Member reviewee, List<GithubId> reviewerGithubIds) {
return new ReviewerGroup(reviewee, reviewerGithubIds, groupName, description, deadline);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,49 +2,106 @@

import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static reviewme.fixture.MemberFixture.회원_산초;
import static reviewme.fixture.MemberFixture.회원_커비;

import java.time.LocalDateTime;
import java.util.List;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;
import reviewme.member.domain.exception.DescriptionLengthExceededException;
import reviewme.member.domain.exception.InvalidDescriptionLengthException;
import reviewme.member.domain.exception.DuplicateReviewerException;
import reviewme.member.domain.exception.EmptyReviewerException;
import reviewme.member.domain.exception.InvalidGroupNameLengthException;
import reviewme.member.domain.exception.SelfReviewException;

class ReviewerGroupTest {

@Test
void 리뷰_그룹이_올바르게_생성된다() {
// given
Member sancho = new Member("산초", 1);
Member member = 회원_산초.create();
String groupName = "a".repeat(100);
String description = "a".repeat(50);
LocalDateTime createdAt = LocalDateTime.of(2024, 7, 17, 12, 0);
LocalDateTime createdAt = LocalDateTime.now();
List<GithubId> githubIds = List.of(new GithubId(3));

// when, then
assertDoesNotThrow(() -> new ReviewerGroup(sancho, List.of(new GithubId(3)), groupName, description, createdAt));
assertDoesNotThrow(
() -> new ReviewerGroup(member, githubIds, groupName, description, createdAt));
}

@ParameterizedTest
@ValueSource(ints = {0, 101})
void 리뷰_그룹_이름_길이_제한을_벗어나는_경우_예외를_발생한다(int length) {
void 리뷰_그룹_이름_길이_제한을_벗어날_수_없다(int length) {
// given
String groupName = "a".repeat(length);
Member sancho = new Member("산초", 1);
LocalDateTime createdAt = LocalDateTime.of(2024, 7, 17, 12, 0);
Member sancho = 회원_산초.create();
LocalDateTime createdAt = LocalDateTime.now();
List<GithubId> githubIds = List.of();

// when, then
assertThatThrownBy(() -> new ReviewerGroup(sancho, List.of(), groupName, "설명", createdAt))
assertThatThrownBy(() -> new ReviewerGroup(sancho, githubIds, groupName, "설명", createdAt))
.isInstanceOf(InvalidGroupNameLengthException.class);
}

@Test
void 리뷰_그룹_설명_길이_제한을_벗어나는_경우_예외를_발생한다() {
void 리뷰_그룹_설명_길이_제한을_벗어날_수_없다() {
// given
String description = "a".repeat(51);
Member sancho = new Member("산초", 1);
LocalDateTime createdAt = LocalDateTime.of(2024, 7, 17, 12, 0);
Member sancho = 회원_산초.create();
LocalDateTime createdAt = LocalDateTime.now();
List<GithubId> githubIds = List.of();

// when, then
assertThatThrownBy(() -> new ReviewerGroup(sancho, List.of(), "그룹 이름", description, createdAt))
.isInstanceOf(DescriptionLengthExceededException.class);
assertThatThrownBy(() -> new ReviewerGroup(sancho, githubIds, "그룹 이름", description, createdAt))
.isInstanceOf(InvalidDescriptionLengthException.class);
}

@Test
void 리뷰어_목록에_리뷰이가_들어갈_수_없다() {
// given
Member member = 회원_산초.create();
String groupName = "Group";
String description = "Description";
LocalDateTime createdAt = LocalDateTime.now();
List<GithubId> reviewerGithubIds = List.of(member.getGithubId());
Copy link
Contributor

Choose a reason for hiding this comment

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

just 제안) reviewee를 나타내는 Member 객체가 sancho, member, reviewee 등으로 다양하게 변수명이 정의되어있는데 reviewee로 통일해봐도 좋을 것 같아요!

예를들어 이곳의 reviewerGithubIds 변수가, reviewee의 아이디를 갖고 reviewerGithubIds를 만드는 상황이라는 게 더 직관적으로 보일 것 같아서요~

Suggested change
List<GithubId> reviewerGithubIds = List.of(member.getGithubId());
List<GithubId> reviewerGithubIds = List.of(reviewee.getGithubId());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

통일 완료!


// when, then
assertThatThrownBy(() -> new ReviewerGroup(member, reviewerGithubIds, groupName, description, createdAt))
.isInstanceOf(SelfReviewException.class);
}

@Test
void 리뷰어_목록이_비어있을_수_없다() {
Member member = 회원_산초.create();
String groupName = "Group";
String description = "Description";
LocalDateTime createdAt = LocalDateTime.now();
List<GithubId> githubIds = List.of();

// when, then
assertThatThrownBy(() -> new ReviewerGroup(member, githubIds, groupName, description, createdAt))
.isInstanceOf(EmptyReviewerException.class);
}

@Test
void 리뷰어를_중복으로_추가할_수_없다() {
// given
Member reviewee = 회원_산초.create();
Member reviewer = 회원_커비.create();

String groupName = "Group";
String description = "Description";
LocalDateTime createdAt = LocalDateTime.now();
List<GithubId> githubIds = List.of(reviewer.getGithubId());
ReviewerGroup reviewerGroup = new ReviewerGroup(reviewee, githubIds, groupName, description, createdAt);
Copy link
Contributor

Choose a reason for hiding this comment

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

ReviewerGroup의 필드명과 맞춰서 변수명을 githubIds → reviewerGithubIds로 하는 건 어떨까요?
(+ 위에 다른 곳에서는 두 가지 모두 쓰이고 있네요! 혹시 둘의 차이를 둔 이유가 있을까요?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

처음에는 빈 리스트를 githubIds로 했다가 리뷰어 생기면서 앞에 접두사가 붙었는데, 통일하는 게 낫겠습니다.

GithubIdReviewerGroup githubIdReviewerGroup = new GithubIdReviewerGroup(reviewee.getGithubId(), reviewerGroup);

// when, then
Copy link
Contributor

Choose a reason for hiding this comment

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

개행 (;´д`)ゞ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

반영완료!

assertThatThrownBy(() -> reviewerGroup.addReviewerGithubId(githubIdReviewerGroup))
.isInstanceOf(DuplicateReviewerException.class);
Comment on lines +120 to +121
Copy link
Contributor

Choose a reason for hiding this comment

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

이 함수 사용되는 곳이 테스트밖에 없는 것 같은데 맞나요..?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

지금은 사용하지 않는 함수인데, 추후 리뷰 그룹 수정 때 필요해서 들어갔나 봐요. 그대로 둘까요, 삭제하는 게 좋을까요 ?

}

Copy link
Contributor

Choose a reason for hiding this comment

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

리뷰 추가와 리뷰어 깃허브 id 추가 기능에 대한 테스트도 있으면 좋을 것 같아요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addReviewnew Review할 때 자동으로 실행돼서 따로 테스트하지 않았어요. 후자도 마찬가지입니다!

}
70 changes: 69 additions & 1 deletion backend/src/test/java/reviewme/review/domain/ReviewTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,28 @@
import static reviewme.fixture.KeywordFixture.꼼꼼하게_기록해요;
import static reviewme.fixture.KeywordFixture.의견을_잘_조율해요;
import static reviewme.fixture.MemberFixture.회원_산초;
import static reviewme.fixture.MemberFixture.회원_아루;
import static reviewme.fixture.MemberFixture.회원_커비;
import static reviewme.fixture.ReviewerGroupFixture.데드라인_남은_그룹;
import static reviewme.fixture.ReviewerGroupFixture.데드라인_지난_그룹;

import java.time.LocalDateTime;
import java.util.List;
import org.junit.jupiter.api.Test;
import reviewme.keyword.domain.Keyword;
import reviewme.member.domain.GithubId;
import reviewme.member.domain.Member;
import reviewme.member.domain.ReviewerGroup;
import reviewme.review.domain.exception.DeadlineExpiredException;
import reviewme.review.domain.exception.IllegalReviewerException;
import reviewme.review.domain.exception.RevieweeMismatchException;
import reviewme.review.exception.GithubReviewerGroupUnAuthorizedException;
import reviewme.review.exception.ReviewAlreadySubmittedException;

class ReviewTest {

@Test
void 리뷰어와_리뷰이가_다른_경우_예외를_발생한다() {
void 리뷰어와_리뷰이가_같을_수_없다() {
// given
Member member = 회원_산초.create();
LocalDateTime createdAt = LocalDateTime.now();
Expand All @@ -25,4 +35,62 @@ class ReviewTest {
assertThatThrownBy(() -> new Review(member, member, null, keywords, createdAt))
.isInstanceOf(IllegalReviewerException.class);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@Test
    void 리뷰어와_리뷰이가_다른_경우_예외를_발생한다() {

"같은_경우" 로 변경되어야 할 것 같슴다

Copy link
Contributor Author

@donghoony donghoony Jul 25, 2024

Choose a reason for hiding this comment

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

리뷰어와_리뷰이가_같을_수_없다로 수정합니다~!

@Test
void 마감_기한이_지난_그룹에_리뷰를_등록할_수_없다() {
// given
Member reviewer = 회원_산초.create();
Member reviewee = 회원_아루.create();
ReviewerGroup reviewerGroup = 데드라인_지난_그룹.create(reviewee, List.of(reviewer.getGithubId()));
LocalDateTime createdAt = LocalDateTime.now();
List<Keyword> keywords = List.of();

// when, then
assertThatThrownBy(() -> new Review(reviewer, reviewee, reviewerGroup, keywords, createdAt))
.isInstanceOf(DeadlineExpiredException.class);
}

@Test
void 하나의_리뷰_그룹에_중복으로_리뷰를_등록할_수_없다() {
// given
Member reviewer = 회원_산초.create();
Member reviewee = 회원_아루.create();
ReviewerGroup reviewerGroup = 데드라인_남은_그룹.create(reviewee, List.of(reviewer.getGithubId()));
new Review(reviewer, reviewee, reviewerGroup, List.of(), LocalDateTime.now());
LocalDateTime createdAt = LocalDateTime.now();
List<Keyword> keywords = List.of();

// when, then
assertThatThrownBy(() -> new Review(reviewer, reviewee, reviewerGroup, keywords, createdAt))
.isInstanceOf(ReviewAlreadySubmittedException.class);
}

@Test
void 리뷰어로_등록되지_않은_회원은_리뷰를_등록할_수_없다() {
// given
Member reviewer = 회원_산초.create();
Member reviewee = 회원_아루.create();
ReviewerGroup reviewerGroup = 데드라인_남은_그룹.create(reviewee, List.of(new GithubId(3)));
Copy link
Contributor

Choose a reason for hiding this comment

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

이 부분은 fixture를 사용하지 않고 member를 직접 생성하는 건 어떨까요?
리뷰어 그룹인 사용자 아이디는 3이나, 리뷰어가 아닌 아이디가 1인 사용자가(산초)가 리뷰를 하려는 상황이라는 게 드러나면 좋을 것 같아서요!

Suggested change
Member reviewer = 회원_산초.create();
Member reviewee = 회원_아루.create();
ReviewerGroup reviewerGroup = 데드라인_남은_그룹.create(reviewee, List.of(new GithubId(3)));
Member reviewer = 회원_산초(1001L, "산초", new GithubId(1));
Member reviewee = 회원_아루.create();
ReviewerGroup reviewerGroup = 데드라인_남은_그룹.create(reviewee, List.of(new GithubId(3)));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

테스트에서 확인되는 게 좋겠네요. 수정합니다 !

LocalDateTime createdAt = LocalDateTime.now();
List<Keyword> keywords = List.of();

// when, then
assertThatThrownBy(() -> new Review(reviewer, reviewee, reviewerGroup, keywords, createdAt))
.isInstanceOf(GithubReviewerGroupUnAuthorizedException.class);
}

@Test
void 그룹_내에서_그룹_밖으로_리뷰를_작성할_수_없다() {
// given
Member reviewer = 회원_산초.create();
Member reviewee = 회원_아루.create();
Member other = 회원_커비.create();
ReviewerGroup reviewerGroup = 데드라인_남은_그룹.create(reviewee, List.of(reviewer.getGithubId()));
LocalDateTime createdAt = LocalDateTime.now();
List<Keyword> keywords = List.of();

// when, then
assertThatThrownBy(() -> new Review(reviewer, other, reviewerGroup, keywords, createdAt))
.isInstanceOf(RevieweeMismatchException.class);
}
}