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] 인수 테스트 에서 사용하는 레포지토리 제거 및 설정 명확하게 변경 (#512) #522

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Oct 3, 2024

📌 관련 이슈

✨ PR 세부 내용

해당 내용은 바로 머지하려고 하긴 보다 더 좋은 코드를 위한 토론의 초석 느낌입니다.

P3 : 컨트롤러 테스트 변경 방향성

@Test
@DisplayName("소셜(리뷰이 -> 리뷰어)에 대한 피드백을 작성한다.")
void create() {
    String 액세스_토큰 = MemberGiven.멤버_로그인(githubOAuthProvider, MemberFixture.MEMBER_ROOM_MANAGER_JOYSON());
    long 방번호 = RoomGiven.방생성(액세스_토큰, 2)
            .id();

    String 리뷰어_토큰 = MemberGiven.멤버_로그인(githubOAuthProvider, MemberFixture.MEMBER_PORORO());
    String 리뷰이_토큰 = MemberGiven.멤버_로그인(githubOAuthProvider, MemberFixture.MEMBER_YOUNGSU());

    방_참가(리뷰어_토큰, 방번호);
    방_참가(리뷰이_토큰, 방번호);

    매칭(액세스_토큰, 방번호);

    Long 리뷰이_아이디 = 리뷰이_목록_조회(리뷰어_토큰, 방번호)
            .stream()
            .findAny()
            .map(MatchResultResponse::userId)
            .orElseThrow();


    리뷰_완료(리뷰어_토큰, 방번호, 리뷰이_아이디);

    SocialFeedbackRequest request = new SocialFeedbackRequest(
            리뷰이_아이디,
            4,
            List.of("방의 목적에 맞게 코드를 작성했어요", "코드를 이해하기 쉬웠어요"),
            "유용한 블로그나 아티클도 남겨주시고, \n 사소한 부분까지 잘 챙겨준게 좋았씁니다."
    );

    //@formatter:off
    RestAssured.given().auth().oauth2(리뷰어_토큰).body(request).contentType(ContentType.JSON)
            .when().post("/rooms/" + 방번호 + "/social/feedbacks")
            .then().statusCode(200);
    //@formatter:on
}

이와 같이 인수 테스트를 작성해봤습니다. ( 한글은 각자 기호 차이 )
신경쓴 부분은

  • Service , Repository 의존성 최대한 제거 ( 보시면, Import 문 Mocking 부분 뺴고 다 제거 되었습니당 )
  • 실제 요청을 통한 검증

이렇게 테스트를 작성하며 느낀점으론
해당 로직이 무조건 Request - Response 단위에서 동작함을 보장할 수 있습니다.
( When 을 위한 Given 도 실제 요청 이므로, 사용자가 행하는 로직과 동일(유사)하다 )

추가로, 우리가 코드를 잘 짜고 있는지? 에 대해서도 생각을 하게 도와주는 거 같습니다.
( MatchResultRespone 가 각 MatchResult 의 ID 를 가지고 있고, 이를 통해 조회를 해도 되지 않을까? 라는 생각이 들었어용
-> 인덱스 용이 )

하지만, 좀 복잡하거나 힘들다고 하면 힘든것도 사실 인듯용 🥲
이게 괜찮다면, 다른 컨트롤러 테스트도 이와 같이 변경해나갈 거 같습니다.


P4 효율적 Mocking

    @BeforeEach
    void setUp() {
        PullRequestInfo info = Mockito.mock(PullRequestInfo.class);
        when(info.containsGithubMemberId(anyString())).thenReturn(true);
        when(info.getPullrequestLinkWithGithubMemberId(anyString())).thenReturn("");
        when(pullRequestProvider.getUntilDeadline(anyString(), any(LocalDateTime.class)))
                .thenReturn(info);

        PullRequestReviews pullRequestReviews = Mockito.mock(PullRequestReviews.class);
        when(pullRequestReviews.getReviewUrl(anyString())).thenReturn("");

        when(githubOAuthProvider.getPullRequestReview(anyString())).thenReturn(pullRequestReviews);
    }

이와같이 setup 부분에서 Mocking 을 지정하는 부분이 있습니다.
이 부분을 사용하려면 모든 부분에서 목 설정을 해야만 합니다.
이를 좀 더 효율적으로 변경할 필요가 있는거 같습니다.
PR 올렸는지 & 리뷰 했는지 확인하는 부분을 인터페이스로 분리 or 전역 Mocking 할 방법을 찾는걸 생각했는데
다양한 의견들을 주세용 🙂

P4 Gtihub Provider 일관성

p.s 현재, GtihubOAuthProvider 가 Review 까지 조회하는걸 확인했는데
현재 저희가 리뷰 조회 & PR 조회 & 유저 조회 3가지의 Github API 로직을 사용하는데
이들을 하나의 Provider 가 담당하게 할지, 각각 담당하게 할지 얘기해봐야 할 거 같아용.
( 현재는 1개 / 2개 가지고 있음 )

이상이고, 초안 느낌으로 올린거라 컨벤션 어디 틀린 부분들도 있을듯용 ☺️

@github-actions github-actions bot added BE 백엔드 개발 관련 작업 리팩터링 리팩터링 작업 labels Oct 3, 2024
Copy link
Contributor Author

github-actions bot commented Oct 3, 2024

Test Results

 44 files   44 suites   7s ⏱️
125 tests 122 ✅ 3 💤 0 ❌
133 runs  130 ✅ 3 💤 0 ❌

Results for commit 096a2ba.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@ashsty ashsty left a comment

Choose a reason for hiding this comment

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

조하~

말씀하신 대로 컨벤션 오류가 조금 있어서 체크 코멘트만 달았어요.

전반적으로 조금 어렵긴 한데 ㅇ.ㅠ 이전 테스트보다는 훨씬 좋은 것 같네요!
이번에 테스트 쫌쫌따리 추가하면서도 느낀 건데 클래스들이 다 너무 중구난방이더라고요 ㅎㅎ...
조금 어렵더라도 통일해서 쓸 수 있다면 좋을 것 같습니다!


P3 : 컨트롤러 테스트 변경 방향성

그래서 저는 이거 좋은듯요?
한글로 작성된 코드도 아까 구두로 들은 이유 때문이라면 이렇게 유지해도 괜찮을 것 같습니다

P4 효율적 Mocking

저는 전역 Mocking 쪽에 한 표입니다

Gtihub Provider 일관성

저는 1개가 전부 담당하는 편이 좋은 것 같아요~
(크게 보았을 때 하나의 클래스가 담당해도 좋을 것 같음: 같은 성격의 서비스)

import java.util.Arrays;

public record PullRequestReviews(GithubPullRequestReview[] pullRequestReviews) {
public String getReviewUrl(String githubUserId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public String getReviewUrl(String githubUserId) {
public String getReviewUrl(String githubUserId) {

import java.util.List;

public class MatchingGiven {
public static void 매칭(String accessToken, long roomId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public static void 매칭(String accessToken, long roomId) {
public static void 매칭(String accessToken, long roomId) {

import io.restassured.http.ContentType;

public class ReviewGiven {
public static void 리뷰_완료(String accessToken, long roomId, long revieweeId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public static void 리뷰_완료(String accessToken, long roomId, long revieweeId) {
public static void 리뷰_완료(String accessToken, long roomId, long revieweeId) {

import java.util.List;

public class RoomGiven {
public static RoomResponse 방생성(String accessToken, int matchingSize) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public static RoomResponse 방생성(String accessToken, int matchingSize) {
public static RoomResponse 방생성(String accessToken, int matchingSize) {

.then().assertThat().statusCode(201).extract().as(RoomResponse.class);
//@formatter:on
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

.map(MatchResultResponse::userId)
.orElseThrow();


Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

assertThatThrownBy(() -> pullRequestReviews.getReviewUrl("notExist"))
.isInstanceOf(CoreaException.class);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

};

@Test
@DisplayName("리뷰가 존재하면, 리뷰의 URL 을 가져온다.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@DisplayName("리뷰가 존재하면, 리뷰의 URL 을 가져온다.")
@DisplayName("리뷰가 존재하면, 리뷰의 URL을 가져온다.")

}

@Test
@DisplayName("리뷰가 존재하지 않는 ID 를 넣으면 예외를 발생한다.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@DisplayName("리뷰가 존재하지 않는 ID 를 넣으면 예외를 발생한다.")
@DisplayName("리뷰가 존재하지 않는 ID를 넣으면 예외를 발생한다.")

Copy link
Contributor

@jcoding-play jcoding-play left a comment

Choose a reason for hiding this comment

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

일단 토론의 초석 느낌이라 하셔서 approve는 누르고 시작할게요~

P3 : 컨트롤러 테스트 변경 방향성

말해준 방향성 너무 좋습니다.
한글로 작성하는 것도 상관없어요~

Service , Repository 의존성 최대한 제거 ( 보시면, Import 문 Mocking 부분 뺴고 다 제거 되었습니당 )
실제 요청을 통한 검증

정확한 인수 테스트라 할 수 있겠네요! 👍👍👍

P4 효율적 Mocking

어차피 여러 인수 테스트에서 중복으로 사용된다면, 전역 Mocking 좋은 것 같아요.
방법은 제가 생각할 때 AcceptanceTest(이름은 대충 썼음)라는 추상 클래스를 만들고 이 부분에서 Mocking 해준 후 다른 테스트들이 상속받는 구조가 제일 바람직하다 생각합니다.

P4 Gtihub Provider 일관성

Copy link
Contributor

@hjk0761 hjk0761 left a comment

Choose a reason for hiding this comment

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

테스트 통일 작업이 쉽지 않았을텐데, 좋은 방향 제시해주서 고마워요 ㅎㅎ

패키지 이름은 Behavior Driven Development 에서 온거겠죠??

컨트롤러 테스트 변경 방향성

수많은 레포지토리와 서비스로부터 의존을 끊어내고 실제 요청으로 컨트롤러 테스트가 돌아가니 좋네요. 정말 제대로 api 를 테스트하는 느낌이어서 좋습니다!

효율적 Mocking

저도 전역 mocking 이 괜찮아 보이네요!

Gtihub Provider 일관성

이 부분은 하나로 합쳐도 괜찮을 것 같아요. Github API 를 중계하는 역할이 나뉠 필요는 없을 것 같다는 생각이 듭니다!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BE 백엔드 개발 관련 작업 리팩터링 리팩터링 작업
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BE] 인수 테스트 에서 사용하는 레포지토리 제거 및 설정 명확하게 변경
4 participants