Skip to content

Conversation

@JamieShin0201
Copy link

@JamieShin0201 JamieShin0201 commented Feb 1, 2021

해야 할 일

  • HelloController 유닛 테스트
  • HelloController MockMVC 테스트
  • Task 모델 테스트
  • TaskService 테스트
  • TaskController 유닛 테스트
  • TaskController MockMVC 테스트
  • 리팩토링

요구 사항

  • 모든 인수 테스트를 통과해야 합니다.
  • 테스트 커버리지 100%를 달성해야 합니다.

Comment on lines 40 to 42
@Nested
@DisplayName("저장된 task가 있다면")
class Context_with_tasks {
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
Author

Choose a reason for hiding this comment

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

맞습니다!! 수정하였습니다 ㅎㅎ

Comment on lines 92 to 94
Task newTask = new Task();

taskService.createTask(newTask);
Copy link
Contributor

Choose a reason for hiding this comment

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

이 것은 우리가 실제로 테스트하는 대상이기 때문에 여기 있으면 안될 것 같아요

Comment on lines +122 to +123
@DisplayName("수정된 task를 리턴한다")
void it_returns_a_updated_task() {
Copy link
Contributor

Choose a reason for hiding this comment

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

수정된 task를 반환한다고 했는데, 반환한 task를 확인하고 있지 않아요

Copy link
Author

Choose a reason for hiding this comment

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

그러네요.. 수정된 task를 확인하는 테스트로 수정하였습니다!

Comment on lines 155 to 156
assertThatThrownBy(() -> taskService.deleteTask(EXISTING_ID))
.isInstanceOf(TaskNotFoundException.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

이건 없어도 될 것 같아요. 어떤 변화가 일어나는지 작성해주시면 더 좋을 것 같아요

@JamieShin0201
Copy link
Author

JamieShin0201 commented Feb 2, 2021

안녕하세요^_^

  1. 리뷰 확인하고 TaskServcieTest를 수정하였습니다.
  2. TaskControllerWebTest를 추가하였습니다.
  3. TaskTest를 추가하였씁니다.
  4. 모킹을 위해 Task클래스에 equals, hashcode를 추가하였습니다.

질문사항

given(taskService.updateTask(EXISTING_ID, task)).willReturn(task);

여기서 task와 리턴하라는 task가 달라서 동작이 제대로 안되더라구요.. 그래서 Task클래스에 equals, hashcode를 추가했습니다. 혹시나 다른방법이 있을까요?

감사합니다 ㅎ.ㅎ

Comment on lines 55 to 58
@BeforeEach
void setUp() {
taskService = new TaskService();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

비어있는 경우에 taskService를 새로 만들어주는 것 같아요. 그것보다는 항상 새로운 taskService를 만들고, task가 있는 경우를 createTask로 만들어서 GIVEN을 더 명확하게 표현해주는게 좋을 것 같아요

@hannut91
Copy link
Contributor

hannut91 commented Feb 2, 2021

질문에 대한 것은 제가 한 번 실행해봐야 알 것 같아요 ㅎㅎ

@hannut91
Copy link
Contributor

hannut91 commented Feb 2, 2021

given(taskService.updateTask(eq(100L), any(Task.class)))

아마 구체적인 task를 입력했고, 둘이 비교할 수 없으니 equalshashCode를 구현해주신 것 같아요. 위와 같이 작성하면 구체적인 task는 사실 테스트에 필요하지 않으니 같은 타입이 들어올 때만 처리할 수 있어요

@JamieShin0201
Copy link
Author

JamieShin0201 commented Feb 2, 2021

eq와 any를 사용해서 처리할수 있군요! ㅎㅎ
어떻게 테스트해야 좋을지 고민을 많이헀는데.. 알려주셔서 감사합니다 ㅎ.ㅎ

given(taskService.updateTask(eq(100L), any(Task.class)))

아마 구체적인 task를 입력했고, 둘이 비교할 수 없으니 equalshashCode를 구현해주신 것 같아요. 위와 같이 작성하면 구체적인 task는 사실 테스트에 필요하지 않으니 같은 타입이 들어올 때만 처리할 수 있어요

Comment on lines 102 to 115
@Nested
@DisplayName("setTitle 메서드는")
class Describe_setTitle {
@Nested
@DisplayName("title이 주어지면")
class Context_with_title {
@Test
@DisplayName("task의 title를 설정한다")
void it_set_task_title() {
task.setTitle(NEW_TASK_TITLE);
assertThat(task.getTitle()).isEqualTo(NEW_TASK_TITLE);
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

title이 안주어진 경우도 있을 것 같이 생겼어요 ㅎㅎ context는 Given이 달라질 때 사용하는 것이라서 이 경우에는 없어도 괜찮을 것 같아요

Copy link
Author

Choose a reason for hiding this comment

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

네 ㅎㅎ title이 안 주어지는 경우가 없으니 context를 작성하지 않는게 좋을 거 같습니다 ㅎ.ㅎ

}

@Nested
@DisplayName("setTitle 메서드는")
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
Author

Choose a reason for hiding this comment

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

아니요! 굳이 없어도 될것같습니다! 오히려 매번 '메서드는' 이라고 쓰는 작업이 귀찮을 것 같아요!

Comment on lines 73 to 100
@Nested
@DisplayName("getTitle 메서드는")
class Describe_getTitle {
@Nested
@DisplayName("task의 title가 존재한다면")
class Context_task_title_exist {
@Test
@DisplayName("task의 title를 리턴한다")
void it_returns_task_title() {
assertThat(task.getTitle()).isEqualTo(TASK_TITLE);
}
}

@Nested
@DisplayName("task의 title이 존재하지 않는다면")
class Context_task_title_does_not_exist {
@BeforeEach
void setUp() {
task = new Task();
}

@Test
@DisplayName("null을 리턴한다")
void it_returns_null() {
assertThat(task.getTitle()).isNull();
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

저는 이런 모양이 Given이 어떻게 다른지 더 잘 보여서 좋은 것 같은데 어떤게 더 나을 것 같나요?

    @Nested
    @DisplayName("getTitle 메서드는")
    class Describe_getTitle {
        @Nested
        @DisplayName("task의 title가 존재한다면")
        class Context_task_title_exist {
            @BeforeEach
            void setUp() {
                task = new Task();
                task.setTitle(TASK_TITLE);
            }

            @Test
            @DisplayName("task의 title를 리턴한다")
            void it_returns_task_title() {
                assertThat(task.getTitle()).isEqualTo(TASK_TITLE);
            }
        }

        @Nested
        @DisplayName("task의 title이 존재하지 않는다면")
        class Context_task_title_does_not_exist {
            @BeforeEach
            void setUp() {
                task = new Task();
            }

            @Test
            @DisplayName("null을 리턴한다")
            void it_returns_null() {
                assertThat(task.getTitle()).isNull();
            }
        }
    }

혹은 단순하게 작성한다면 다음과 같이 할 수도 있을거에요

@Test
void title() {
    Task task = new Task();
    
    assertThat(task.getTitle()).isNull();

    task.setTitle("some title");

    assertThat(task.getTitle()).isEqualTo("some title");
}

Copy link
Author

Choose a reason for hiding this comment

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

given이 명확한게 좋은 거 같습니다!! ㅎㅎ
그리고 아래처럼 작성하면 getTitle, setTitle이 한 테스트로 완료되는군요!
모델클래스의 getter, setter는 단순하게 테스트 해도 좋을거 같습니다!!

@JamieShin0201
Copy link
Author

안녕하세요^_^

  1. DisplayName의 '메서드는' 이라는 설명을 지웠습니다.
  2. Given을 명확하게 수정하였습니다.

질문사항

Task 모델 테스트에서

        @DisplayName("task의 title를 설정한다")
        @DisplayName("title를 설정한다")

'task의' 라는 설명이 굳이 필요 없을 듯하여 수정하였는데요. 혹시 어떻게 생각하시나요?

@hannut91
Copy link
Contributor

hannut91 commented Feb 4, 2021

task의는 저도 없어도 될 것 같아요 ㅎㅎ 이미 테스트 대상이 Task라는 것을 알고 있으니까요

import static org.assertj.core.api.Assertions.assertThat;

@DisplayName("Task 클래스")
class TaskTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

테스트가 굉장히 자세해서 좋은데, Task의 사용법을 더 드러내려면 단순하게 해도 괜찮을 것 같아요

    @Test
    void create() {
        Task task = new Task();
        task.setId(1L);
        task.setTitle("책 읽기");

        assertThat(task.getId()).isEqualTo(1L);
        assertThat(task.getTitle()).isEqualTo("책 읽기");
    }

}

@Test
@DisplayName("200코드와 저장된 task를 리턴한다")

Choose a reason for hiding this comment

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

웹은 리턴보다는 “응답”이 더 적절합니다.


@Test
@DisplayName("200코드와 저장된 task를 응답한다")
void it_replies_with_200_and_the_tasks() throws Exception {
Copy link

@ahastudio ahastudio Feb 5, 2021

Choose a reason for hiding this comment

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

웹의 응답은 reply 대신 respond를 씁니다.
저라면 여기를 it_responds_all_tasks_with_status_code_200 같은 식으로 써줄 것 같아요.
사실 camelCase를 써야 해서 itRespondsAllTasksWithStatusCode200이 더 정확할 거고, 이게 꽤 읽기 어렵기 때문에 메서드 이름은 그냥 list 정도로 하고 @DisplayName 쪽을 풍성하게 써줍니다.

Copy link
Author

Choose a reason for hiding this comment

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

reply와 respond 고민하다 reply로 했는데!
respond가 더 적절할 것 같네요! 감사합니다 ㅎ.ㅎ

Choose a reason for hiding this comment

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

이 문서를 첫 문장을 참고하시면 좋을 것 같아요: https://www.w3.org/Protocols/rfc2616/rfc2616-sec6.html

After receiving and interpreting a request message, a server responds with an HTTP response message.

Copy link
Contributor

@hannut91 hannut91 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다 ㅎㅎ사실 더 이상 어떤 커멘트를 드릴지 잘 모르겠어요. 고민인 부분은 mockMvc테스트 검증하는 부분인데, JavaScript는 JSON을 검증하기가 굉장히 쉬운데, Java는 복잡한 느낌이에요.

단순하게 테스트하려다보면 너무 조금만 검증하는 것 같고, 자세히 하려다보면 코드가 복잡해지더라구요.
그래서 저는 JSON 문자열로 검증했었는데 이 부분은 더 고민해보시면 좋을 것 같아요

@JamieShin0201
Copy link
Author

저도 테스트 코드가 너무 복잡하지 않나? 생각을 했었는데! 윤석님은 JSON 문자열로 검증하셨군요!
어느정도 테스트해야 안정감을 느낄수 있을지는 계속 고민해봐야 할 것같습니다.

이번 한주도 감사했습니다!! ㅎ.ㅎ

@hannut91 hannut91 merged commit b3cf886 into CodeSoom:JamieShin0201 Feb 7, 2021
@hannut91
Copy link
Contributor

hannut91 commented Feb 7, 2021

채널에 회고를 남겨주셔서 머지합니다! 고생하셨습니다!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants