3주차 과제 - Spring 테스트 작성하기 #83
Conversation
| @Override | ||
| public boolean equals(Object o) { | ||
| Task task = (Task) o; | ||
| return Objects.equals(this.id, task.getId()) && this.title.equals(task.getTitle()); | ||
| } |
There was a problem hiding this comment.
early return 기법을 적용해서 o 가 null인 경우에 빠르게 false를 리턴하도록 수정해보세요.
| @AutoConfigureMockMvc | ||
| class TaskControllerTest { |
There was a problem hiding this comment.
구조적으로 테스트하는 기법을 익혀보도록 합시다. JUnit5의 @Nested를 사용해 계층 구조의 테스트 코드를 작성하는 방법을 조사해서 적용해 보세요.
There was a problem hiding this comment.
테스트 코드를 작성하면서 궁금한 점이 생겨서 여쭤봅니다!
@Nested
@DisplayName("id가 null이거나 id에 해당하는 Task가 없다면")
class Context_IdIsNullAndInvalidId{
private final Long nullId = null;
private final Long invalidId = 100L;
private final Task updateTask = new Task();
...위와 같이Context 내부의 It에 필요한 준비 단계들이 중복되는 경우가 많아 전역으로 선언할까 고민해봤습니다
하지만 의미상으로는 각 @Test의 준비단계라서 중복을 제거하기 보단 테스트 코드의 의미를 명확하게 전달하는게 좋을 것 같아 그대로 두었습니다. 중복을 신경쓰지 않는게 좋을까요??
- 이번 과제를 진행하면서
mock(+ 테스트 더블)을 왜 쓰는지 알아보았습니다.- 스프링을 실행시켜 테스트하면 시간이 오래 걸린다
- 테스트 대상이 복잡해지면 상황 설정이 어렵다
- 테스트 대상에 집중할 수 없다
mock을 사용하지 않고 테스트를 하는 상황도 있지 않을까 싶은데요. 종립님은 어떤 기준으로 결정 하시는지 궁금합니다
There was a problem hiding this comment.
Context를 한 겹으로만 쓰지 않아도 괜찮아요. 중복되는 조건이 있다면 그 중복 조건을 Context로 설정하고, 안쪽에 좀 더 상세한 Context를 만들어서 진행하면 되죠. 이건 설명을 어떻게 적는지에 대한 것이기도 해요.
Describe updateTask 메소드는
Context 유효 기한이 지난 Task에 대해
// 유효 기한이 지난 Task 를 준비한다.
Context 정상 입력이 주어지면
// 정상 입력 케이스 공통을 준비한다
Context (정상 입력 케이스 1)
// 정상 입력케이스 공통에 추가로 정상 입력케이스 1을 준비한다.
Context (정상 입력 케이스 2)
// 정상 입력케이스 2를 준비한다.
mock에 대해서는 취향이 많이 갈립니다. mock의 사용에 대해서는 팀마다, 서비스 애플리케이션의 성격에 따라 달라질 수 있어서 mock을 최소한으로 사용하고 가능한 한 안 쓰려 하는 방향과 적극적으로 사용하는 방향을 잘 저울질해서 선택할 수 있어야 합니다. mock 에 대해서는 다른 분의 코드 리뷰에 남긴 제 코멘트를 읽고 고민해보시는 것도 도움이 될 것 같습니다. given에 대한 이야기이긴 하지만 given을 mock으로 바꿔 읽어도 괜찮습니다.
There was a problem hiding this comment.
유닛테스트 작성 후에 given을 사용하지 않고 스프링 DI를 사용하여 TaskController 테스트를 작성 해보겠습니다 ㅎㅎ
| public boolean equals(Object o) { | ||
| if (this == o) | ||
| return true; | ||
| if (o == null || this.getClass() != o.getClass()) | ||
| return false; | ||
| Task task = (Task) o; | ||
| return Objects.equals(this.id, task.getId()) && this.title.equals(task.getTitle()); | ||
| } |
There was a problem hiding this comment.
좋습니다! 다만 if 문은 한 줄짜리라도 꼭 중괄호를 생략하지 않는 습관을 들이는 것이 좋습니다. 비싸지 않으면서도 치명적인 실수를 방어해주는 좋은 습관이죠.
이와 관련된 유명하고 재미있는 버그 이야기가 있습니다. Apple의 SSL/TLS 프로토콜에서 발견된 끔찍한 실수입니다. if 중괄호가 습관화되어 있었다면 하지 않았을 실수죠.
https://embeddedgurus.com/barr-code/2014/03/apples-gotofail-ssl-security-bug-was-easily-preventable/
| @DisplayName("Task가 존재한다면") | ||
| class Context_NotEmptyTask{ |
There was a problem hiding this comment.
설명과 클래스 이름이 서로 맞지 않게 작성되어 있어요.
| @Test | ||
| @DisplayName("모든 Task를 반환한다") | ||
| void It_ReturnAllTask(){ | ||
| setDefaultSizeTasks(); |
There was a problem hiding this comment.
이런 준비 코드는 Context 영역에서 @BeforeEach에 넣어주도록 작업해 보세요. Context는 실험을 준비하는 곳이고, @Test는 실험을 실행하는 곳이라 생각하시면 됩니다. 준비 영역과 실행 영역을 나누는 것을 의식적으로 연습하고 실천해 보세요.
| @Nested | ||
| @DisplayName("id가 null이라면") | ||
| class Context_NullPathVariable{ | ||
|
|
||
| @Test | ||
| @DisplayName("TaskNotFoundException이 발생한다") | ||
| void It_ThrowException(){ | ||
| assertThatThrownBy(() -> controller.detail(null)) | ||
| .isInstanceOf(TaskNotFoundException.class); | ||
| } | ||
| } |
There was a problem hiding this comment.
id가 null인 경우에 대한 Context를 만들었으므로, Context 영역에서 아이디를 선언해서 사용해 보세요. 예를 들자면 이렇게요.
| @Nested | |
| @DisplayName("id가 null이라면") | |
| class Context_NullPathVariable{ | |
| @Test | |
| @DisplayName("TaskNotFoundException이 발생한다") | |
| void It_ThrowException(){ | |
| assertThatThrownBy(() -> controller.detail(null)) | |
| .isInstanceOf(TaskNotFoundException.class); | |
| } | |
| } | |
| @Nested | |
| @DisplayName("id가 null이라면") | |
| class Context_NullPathVariable{ | |
| private final givenId = null; | |
| @Test | |
| @DisplayName("TaskNotFoundException를 던진다") | |
| void It_ThrowException(){ | |
| assertThatThrownBy(() -> controller.detail(givenId)) | |
| .isInstanceOf(TaskNotFoundException.class); | |
| } | |
| } |
한편 ~예외가 발생한다라는 표현에 대해 생각해볼 점이 하나 있습니다. 그것은 우리가 예외를 취급하는 코드를 작성할 때 throw, catch, @throws... 같은 단어를 쓴다는 거죠. 79번 라인의 assert 메소드 이름을 읽어보세요. assertThatThrownBy라고 되어 있죠?
우리가 애플리케이션의 사용자라면 예외가 언제 던져질지 모릅니다. 그래서 예외는 "발생하는" 것이라 생각하게 되죠. 그런데 우리는 애플리케이션을 만들고 있습니다. 언제 어떤 예외를 던질지 결정하는 건 우리죠. 그래서 이렇게 설명을 적게 됩니다.
detail 메소드는, id 가 null 이라면 TaskNotFoundException 예외를 던진다.
There was a problem hiding this comment.
우리가 애플리케이션의 사용자라면 예외가 언제 던져질지 모릅니다. 그래서 예외는 "발생하는" 것이라 생각하게 되죠.
우리는 애플리케이션을 만들고 있습니다.
말씀대로 애플리케이션을 만드는 사람이 던져주는 것인데 "발생한다"라고 무의식적으로 생각하게 되는 것 같습니다 ㅎㅎ
명심하겠습니다
…rollerNestedTest.java 예외가 발생 X , 예외를 던진다 O Co-authored-by: JohnGrib <johngrib82@gmail.com>
| @Nested | ||
| @DisplayName("Task가 존재하지 않는다면") | ||
| class Context_NotExistsTask{ | ||
| private final int EMPTY = 0; | ||
|
|
||
| @Test | ||
| @DisplayName("비어있는 List를 반환한다") | ||
| void It_ReturnEmptyList(){ | ||
| assertThat(controller.list()).hasSize(EMPTY); | ||
| } | ||
| } |
There was a problem hiding this comment.
우리는 지금 스코프가 있는 테스트 코드 스타일을 선택해서 작업을 하고 있어요. 이렇게 테스트별로 스코프가 있는 방식으로 작업할 때 열심히 고민해야 하는 주제는 바로 테스트 코드 스코프에서 논리적인 인과를 잘 마련해주는 것입니다.
여기에서 controller.list()를 호출하는 상황에 대해 생각해 보죠. 조금 올라가보면 @BeforeEach에서 new TaskController를 하고 있습니다. 그리고 이 테스트에서는 "자신 있게" "Task가 존재하지 않는다면"이라고 이야기하고 있죠. 이 자신감의 근거는 "TaskController를 new 했으니까 당연히 list 는 비어있을거야"라는 생각이라 할 수 있습니다. 하지만 이것은 일종의 상식에 근거한 비약입니다. 만약 컨트롤러를 새로 만들었을 때 디폴트 할 일이 리스트에 들어가있게 어느 동료가 수정한다면 이 테스트는 실패할 겁니다.
정말 논리적으로 튼튼한 테스트코드를 작성하려면 몇 가지 조치가 필요해요. 가장 쉬운 방법은 이 두 가지입니다.
- 방법 1.
"Task가 존재하지 않는다면"을새로 생성한 컨트롤러를 통해 호출한다면으로 수정한다. - 방법 2.
"Task가 존재하지 않는다면"컨텍스트 스코프에서 모든 할 일을 삭제한다.
위의 두 방법이 각자 어떤 장점을 갖고 있는지에 대해 생각해 보세요.
There was a problem hiding this comment.
하나의 사건 (원인)이 다른 사건 (결과)을 일으킬 때 둘의 관계 출처
스코프가 있는 테스트 코드 스타일의 논리적인 인과 기억하겠습니다 ㅎㅎ
방법 1. "Task가 존재하지 않는다면"을 "새로 생성한 컨트롤러를 통해 호출한다면"으로 수정한다.
글로 명시하는게 읽는 사람에겐 더 정확하게 전달될 것 같습니다. 하지만 "새로 생성한 컨트롤러"라는 글은 테스트 코드에 뭔가 맞지 않는 느낌이네요. 글로 명시 하여도 @BeforeEach가 수정되면 해당 테스트 부분도 신경 써줘야 할 것 같습니다
방법 2. "Task가 존재하지 않는다면" 컨텍스트 스코프에서 모든 할 일을 삭제한다.
Context단계에서 테스트에 대한 준비를 더 철저하게 해주는게 더 좋을 것 같습니다
전체적으로 다시 확인 해보겠습니다!
| private final Long givenId = null; | ||
|
|
||
| @Test | ||
| @DisplayName("TaskNotFoundException를 던진다") |
There was a problem hiding this comment.
잘 수정하셨습니다. 그런데 여기에서 한 가지 더 생각해볼만한 문제가 있습니다. 바로 TaskNotFoundException 클래스의 이름 변경입니다. 누가 Task 클래스의 이름을 Todo로 변경한다고 상상해 봅시다. IntelliJ 에서 rename 해서 클래스 이름을 변경하겠죠? 그리고 Task가 Todo로 바뀌었으니까, 작업한 사람은 TaskNotFoundException 클래스가 있는 걸 보고 "이것도 바꿔야겠다"라고 생각하고 TodoNotFoundException으로 이름을 바꿀 거에요. 그리고 Task가 들어간 다른 클래스들도 몇 개 더 바꿨다고 상상해 보세요.
아마 그 분은 이 테스트의 @DisplayName의 문자열도 수정해야 한다는 걸 놓칠 수도 있을 겁니다. IntelliJ가 문자열까지 rename해주지는 않으니까요. 그래서 rename하는 사람이 이 설명을 수정하지 않고 넘어갈 수 있겠죠. 거짓된 설명이 남게 되는 것입니다.
| @DisplayName("TaskNotFoundException를 던진다") | |
| @DisplayName("찾지 못했다는 내용의 예외를 던진다") |
하지만 위와 같이 설명을 작성해 두면 클래스 이름 변경에 취약했던 설명이 이름 변경에도 유연하게 대응하게 됩니다. 즉 클래스 이름에 테스트 설명이 강하게 의존하고 있었던 문제를 약하게 의존하게 만들어 해소한거죠. 잘 생각해보면 객체지향 원칙이 주석이나 설명에도 작동한다는 사실을 알 수 있습니다.
There was a problem hiding this comment.
아 중요한 걸 놓치고 있었네요..
개발을 할 때만 유연하게가 아니라 말씀하신 주석이나 설명 , 문서 작성에도 의존에 대한 고민이 필요하네요
감사합니다 ㅎㅎ
| @Nested | ||
| @DisplayName("존재하지 않는 id로 조회한다면") | ||
| class Context_SearchInvalidId{ | ||
| private final Long invalidId = 100L; |
There was a problem hiding this comment.
이 아이디가 정말로 존재하지 않는다는 논리적인 근거를 이 스코프에서 마련해 보세요. @BeforeEach에서 100L에 해당하는 할 일을 지워버리는 것도 방법이죠.
| @Test | ||
| @DisplayName("모든 Task를 조회한다.") | ||
| void list(){ | ||
| assertThat(controller.list()).hasSize(3); |
There was a problem hiding this comment.
여기에 3이 들어가기 때문에 변경에 취약할 뿐 아니라 논리적 근거가 부족한 테스트가 됐어요. 왜 3인가? 를 열심히 생각해 보세요. 저라면 이런 식으로 수정이 되도록 준비 작업을 해 둘 것 같습니다.
| assertThat(controller.list()).hasSize(3); | |
| assertThat(controller.list()).hasSize(allTaskCount); |
…rollerTest.java Co-authored-by: JohnGrib <johngrib82@gmail.com>
This reverts commit 7d25cc2.
| final StringBuilder sb = new StringBuilder("Task{"); | ||
| sb.append("id=").append(id); | ||
| sb.append(", title='").append(title).append('\''); | ||
| sb.append('}'); | ||
| return sb.toString(); |
There was a problem hiding this comment.
String.format을 사용해서 표현하는 건 어떨까요? 다음과 같이 표현할 수 있을거에요
return String.format("{ id = %s, title = %s }", id, title);더 좋은건 Template literal이지만 자바에서는 지원하질 않네요 ㅠ 만약 이런게 지원하는 코틀린을 사용했다면 다음과 같이 사용할 수 있을거에요
return "{ id = $id, title = $title }"|
|
||
| @BeforeEach | ||
| void setUp() throws JsonProcessingException { | ||
| service.clearTasks(); |
There was a problem hiding this comment.
테스트가 시작되기 전에 service를 초기화하려고 하신 것 같아요. 전체 테스트가 시작되기 전에 초기화를 하는 것은 어떨까요?
| @Test | ||
| @DisplayName("할 일들을 반환한다") | ||
| void It_GetTasks() throws JsonProcessingException { | ||
| String getContent = mapper.writeValueAsString(service.getTasks()); |
There was a problem hiding this comment.
getContent는 `content를 가져와라! 라고 명령하는 것 같이 보이네요
| @Nested | ||
| @DisplayName("할 일을 추가하는 메소드는") | ||
| class Context_Add{ |
There was a problem hiding this comment.
이 것은 테스트 대상인가요? 그렇다면 Describe라고 적어주어야겠네요
| @DisplayName("{id}에 해당하는 할 일이 없다면") | ||
| class Context_InvalidId{ | ||
|
|
||
| private final String invalidId = "1"; |
There was a problem hiding this comment.
invalid라고하면 아이디가 /아이디 이렇게 요청하는 것도 invalid라고 해석될 수 있을 것 같아요. 더 구체적으로 표현해 보면 좋겠네요
| @DisplayName("body에 담긴 제목으로 수정한다") | ||
| void It_UpdateTask() throws Exception { | ||
| String content = mapper.writeValueAsString(afterTask); | ||
| mockMvc.perform(put(RESOURCE +"/"+updateId) |
There was a problem hiding this comment.
| mockMvc.perform(put(RESOURCE +"/"+updateId) | |
| mockMvc.perform(put(RESOURCE + "/" + updateId) |
코드를 일관성있게 정리하면 더 좋겠네요
…rollerWebSpringDITest.java Co-authored-by: Yunseok <hannut91@gmail.com>
| @@ -121,7 +121,7 @@ void It_GetObject() throws JsonProcessingException { | |||
|
|
|||
| @Nested | |||
| @DisplayName("할 일을 추가하는 메소드는") | |||
| class Context_Add{ | |||
| class Describe_Add{ | |||
| mockMvc.perform(get(RESOURCE)) | ||
| .andExpect(status().isOk()); | ||
|
|
||
| verify(service).getTasks(); |
There was a problem hiding this comment.
이 라인은 이 테스트의 목적이 아닙니다. GET /tasks를 호출했는데 TaskService의 getTasks 메소드가 호출됐는지 검사하는 건 테스트가 구현에 의존하는 일이죠.
There was a problem hiding this comment.
아 감사합니다 verify를 언제 사용하여야 하는지 몰랐는데 ㅎㅎ Web테스트가 아니라 Controller테스트에서 사용했어야 하겠네요
| @@ -47,6 +47,11 @@ public Task deleteTask(Long id) { | |||
| return task; | |||
| } | |||
|
|
|||
| public void clearTasks(){ | |||
| newId = 0L; | |||
| tasks = new ArrayList<>(); | |||
There was a problem hiding this comment.
ArrayList를 사용하게 된 김에 DynamicArray 자료구조에 대해 학습해보는 것도 좋을 것 같습니다.
| assertThatThrownBy(() -> service.getTask(invalidId)) | ||
| .isInstanceOf(TaskNotFoundException.class); | ||
| assertThatThrownBy(() -> service.getTask(nullId)) | ||
| .isInstanceOf(TaskNotFoundException.class); |
There was a problem hiding this comment.
지금은 두 건 밖에 없지만, 검사할 케이스가 여러 건이 된다면 invalidId와 nullId를 리스트에 넣고 for 로 돌리면서 assert를 하는 방법도 사용할 수 있을 거에요.
| @DisplayName("null 참조 예외가 발생한다") | ||
| void It_Null(){ | ||
| assertThatThrownBy(() -> service.createTask(nullTask)) | ||
| .isInstanceOf(NullPointerException.class); |
There was a problem hiding this comment.
이건 NPE를 그대로 사용하기보다는 나름의 의미를 갖고 있는 별도의 예외를 정의해 사용하는 방향으로 고쳐 보세요
There was a problem hiding this comment.
네 다음부터는 예외를 그대로 사용하지 않고 커스텀 예외를 추가하도록 하겠습니다 ㅎㅎ
| } | ||
|
|
||
| @Nested | ||
| @DisplayName("비교할 객체와 주소가 같다면") |
There was a problem hiding this comment.
테스트 코드를 읽어보면 주소가 같을 때에만 true를 리턴하는 것처럼 보입니다. 그 외의 경우는 equals가 true를 리턴하는 경우가 없을까요?
테스트 코드를 처음 접해봐서 부족하지만 추가 과제나 키워드들 던져주시면 열심히 조사해보겠습니다!!
한 주간 잘 부탁드립니다 ㅎㅎ