[BE] 테스트 코드 리팩토링 #730

woosung1223 · 2023-11-13T14:42:15Z

구현 기능 및 변경 사항

테스트 코드 스타일을 정리했습니다.
불필요한 의존성을 제거하고 코드를 재배치했습니다.
로깅용으로 남아있던 코드를 제거했습니다.
Disabled된 테스트를 복구했습니다. (인수 테스트 포함)

Repository 테스트는 일단 남겨뒀는데, 요 PR 닫히기 전에 논의까지 해서 결정내리면 좋을 듯 합니다. 저는 개인적으로 findById와 같은 기본적인 Spring Data JPA의 기능은 테스트 할 필요가 없다는 생각이 듭니다!

@Query 로 지정된 메소드만 테스트하는게 나을 것 같은데 다들 어떻게 생각하시나요? 불필요한 테스트는 결국 관리 비용을 높이는 원인처럼 느껴져서요.. 주석처리 되어 있는 Repository 테스트도 있더라고요 🥲

리뷰하실 때에는 커밋 단위로 보시면 보기 편하실 것 같습니다!!

스크린샷(선택)

…록 변경

jaehee329

서로 이해하기 어려운 수준의 테스트는 없다 생각해서
적당히 자율성이 있어도 괜찮을 것 같은데 또 쌓아놓고 보니 중구난방이었네요 ㅎㅎㅎ
고생하셨습니다~

제가 올린 PR에서 jwt관련이 삭제되어 머지하면 conflict가 날 것 같은데 시점을 조율해보죠~
코멘트 남긴 부분들은 다같이 의견 공유하면 좋겠어요!

jaehee329 · 2023-11-15T15:40:19Z

backend/src/test/java/harustudy/backend/acceptance/AcceptanceTest.java

-        System.out.println(locationHeader);
-        return Long.parseLong(parsed[3]);
+
+        return Long.parseLong(parsed[4]);


이 부분은 /api/v2/studies/1를 파싱하는 부분이어서 parsed[4]로 바뀐거니 다른 분들 보실때 참고하세요~
이런 부분은 정말 유지보수성이 떨어지는 코드인거 같은데 당장 뾰족한 수가 떠오르진 않네요 ㅋㅋㅋ;

Suggested change

return Long.parseLong(parsed[4]);

return Long.parseLong(parsed[parsed.length-1]);

코테풀 때 사용하는 코드 스타일이기는 한데 지금처럼 하드 코딩하는 것보단 나을 것 같아서 제안드려봐요...!

jaehee329 · 2023-11-15T15:45:15Z

backend/src/test/java/harustudy/backend/acceptance/AcceptanceTest.java

+        assertThatCode(() -> objectMapper.readValue(response, StudyResponse.class))
+                .doesNotThrowAnyException();


저번에 마코랑은 얘기했었던 것 같은데 assertThatCode() + doesNotThrowAnyException()을 쓰는 이유가 있나요?
저는 assertDoesNotThrow()가 둘을 합친 것이라 생각해 가독성도 좋고 간결하다고 생각해서요

요 부분은 지속적으로 팀 내에서 언급이 되는 사항이라 여겨지는데 이번 PR을 기점으로 컨벤션을 잡는 것이 좋을 것 같습니다. 이번 PR이 테스트 코드 리팩토링이기도 하구요!

스타일 차이이긴 한데 저는 assertThatCode() 취향이기는 합니다.
다른 부분도 assertThat() + isEqualTo() 이런식으로 사용하니까 예외 검증하는 부분도 assertThatCode() + doesNotThrowxxx() 이런식으로 하는게 일관적이고 개인적으로는 읽기 편하다고 느낍니다.
하지만 큰 차이는 아니라서 팀이 결정한다면 어느 쪽이든 좋아요:)

저도 둘 다 상관은 없으나 이참에 컨벤션 정하는 게 나을 것 같네요 !

=> 팀 회의를 통해 둘 다 사용하는 방향으로 결정했습니다.

jaehee329 · 2023-11-15T15:47:46Z

backend/src/test/java/harustudy/backend/integration/AuthIntegrationTest.java


 @SuppressWarnings("NonAsciiCharacters")
-@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)


SuppressWarnings는 여기에 있고 DisplayNameGeneration은 IntegrationTest 클래스에 있어서 오히려 헷갈리네요 ㅋㅋㅋ
삭제가 안된다면 @DisplayNameGeneration(ReplaceUnderscores.class) 으로 바뀌어야 할 것 같고...

저는 익숙해져서 저희거 테스트 짤 때는 둘을 그냥 깔고 가는데 다들 익숙하시다면
IntegrationTest의 DisplayNameGeneration을 제거하고 각 테스트마다 두 어노테이션을 직접 붙이는건 어떤가요?

저희가 SuppressWarnings와 DisplayNameGeneration을 붙여주는데 익숙해지긴 했지만 IntegrationTest에 붙여서 공통적인 부분을 분리할 수 있는 건 분리하는게 좋다고 생각해요. SuppressWarnings의 경우 commit 하기 전에 경고를 확인하고 수정할 수 있지만, 테스트 명은 비교적으로 까먹기 쉽다고도 생각해서요! (개인적인 의견입니다 ㅎㅎ...) 요 부분은 의견 한 번 맞춰보시죠~

저도 공통부분을 관리한다는 입장에서 IntegrationTest에 두는 게 나을 것 같아 올렸습니다!

사실 SupressWarnings도 공통 부모에서 관리되면 좋긴 한데, 왜 상속이 안되는지 아쉽네요 🥲

jaehee329 · 2023-11-15T15:49:02Z

backend/src/test/java/harustudy/backend/acceptance/AcceptanceTest.java

@@ -1,6 +1,7 @@
 package harustudy.backend.acceptance;


이 클래스에 안 쓰게 된 import문들 정리해줘야 할 것 같아요

👍 반영하겠습니다!!

jaehee329 · 2023-11-15T15:57:36Z

backend/src/test/java/harustudy/backend/integration/StudyIntegrationTest.java

-            Assertions.assertThat(location.split("/")).hasSize(4);
+            Assertions.assertThat(location.split("/")).hasSize(5);


수정하는 김에 제안하는 것인데
이 클래스에서 softly.assertThat... 이런 식으로 softassertions 내부에서 assertion을 하고 있거든요,
모두 static import된 assertThat()을 사용하도록 통일하면 깔끔할 것 같아요.
테스트마다 이런 부분도 있는데 여기는 그렇지 않아서요

assertSoftly() 내에서 softly.assertThat()을 사용해서 검증하지 않으면 저희가 예상하는 softAssertion을 수행할 수 없을 것 같아요. 저도 이번 리뷰를 하며 다시 찾아보고 실험해봤는데 지금 코드는 softlyAssertion이 되지 않더라구요! 여기 부분은 수정되어야 할 것 같습니다.

오 너무 좋은 지적! softly.assertThat()으로 통일해야겠네요!
softly라는 단어보다도 더 짧은 것으로 통일해서 간단하게 쓰도록 하면 좋곘네요

jaehee329 · 2023-11-15T16:03:35Z

backend/src/test/java/harustudy/backend/testutils/EntityManagerUtils.java

+
+import jakarta.persistence.EntityManager;
+
+public class EntityManagerUtils {


이 친구.. 계속 가는건가요? ㅋㅋㅋㅋ
정말 간단한 util클래스인데 차라리 영속화하려는 모든 엔티티를 가변인자로 받아서 다 merge하고 마무리로 flush clear까지 해준다면 유틸 클래스로서 조금 의미가 있을 것 같은데
당장은 em.persist는 persist대로 하면서 flush, clear만 이 유틸을 통해서 한다는게 추상화 레벨이 맞지 않는 것 같고, 책임을 분리할 만한 복잡한 일도 아니라고 느껴져요.

추가로 모듈 분리까지 고려했을 때 테스트 패키지는 공유가 안되지 않나요? 그 점에서도 관리가 어려울 것 같고요.
저는 이 정도의 역할이라면 쓰지 않는 편이 낫다고 느껴집니다.
다른 분들 생각은 어떠신가요?

오프라인에서 얘기를 나눴던 것처럼 수료까지 얼마 남지 않은 시점에서 좀 더 우선순위가 높은 태스크를 집중해서 처리하는 것도 좋아서 일단 미뤄둬도 저는 괜찮을 것 같아요~

MoonJeWoong

테스트도 이젠 전부 되살아나니 든든하네요~ 고생 많으셨습니다.
softAssertion 관련된 부분은 수정되어야 할 것 같아 RC 드렸습니다. 확인 부탁드려요~ 😄

MoonJeWoong · 2023-11-16T01:01:18Z

backend/src/test/java/harustudy/backend/acceptance/AcceptanceTest.java

-        System.out.println(locationHeader);
-        return Long.parseLong(parsed[3]);
+
+        return Long.parseLong(parsed[4]);


Suggested change

return Long.parseLong(parsed[4]);

return Long.parseLong(parsed[parsed.length-1]);

코테풀 때 사용하는 코드 스타일이기는 한데 지금처럼 하드 코딩하는 것보단 나을 것 같아서 제안드려봐요...!

MoonJeWoong · 2023-11-16T01:06:33Z

backend/src/test/java/harustudy/backend/acceptance/AcceptanceTest.java

이거 버저닝 걷어내고 살렸으면 작업량이 줄어드셨을텐데... 고생 많으셨어요 😭

MoonJeWoong · 2023-11-16T01:15:52Z

backend/src/test/java/harustudy/backend/acceptance/AcceptanceTest.java

@@ -94,9 +87,9 @@ void setUp() {

    @Test
    void 회원으로_진행했었던_스터디_목록을_조회한다() throws Exception {
+        LoginResponse 로그인_정보 = 구글_로그인을_진행한다();


엇 혹시 이 부분 순서를 바꿔주신 이유가 있으신지 궁금합니다!
제가 예전에 인수 테스트를 작성했을 때 회원으로 진행해둔 기존 스터디 목록을 조회한다는 시나리오를 순차대로 표현하고 싶어서 회원으로_스터디를_진행한다() 로직을 먼저 수행하고 로그인-스터디 목록 조회 테스트를 수행하도록 구현했었거든요. 변경해주신 다른 이유가 있다면 말씀 부탁드립니다~ 🙇

오프라인에서 말씀드렸다시피 '로그인을 안 했는데 어떻게 스터디를 했지?' 라는 생각이 들어 순서를 조정하게 되었습니다!

그런데 말씀 들어보니 그럴 수 밖에 없는 구조인 것 같아서, 다시 복구하도록 하겠습니다 👍

추후 인수 테스트용 DSL을 하나 만드는 것도 좋아보이네요!

MoonJeWoong · 2023-11-16T01:17:49Z

backend/src/test/java/harustudy/backend/acceptance/AcceptanceTest.java

+        assertThatCode(() -> objectMapper.readValue(response, StudyResponse.class))
+                .doesNotThrowAnyException();


요 부분은 지속적으로 팀 내에서 언급이 되는 사항이라 여겨지는데 이번 PR을 기점으로 컨벤션을 잡는 것이 좋을 것 같습니다. 이번 PR이 테스트 코드 리팩토링이기도 하구요!

MoonJeWoong · 2023-11-16T01:27:58Z

backend/src/test/java/harustudy/backend/integration/AuthIntegrationTest.java


 @SuppressWarnings("NonAsciiCharacters")
-@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)


저희가 SuppressWarnings와 DisplayNameGeneration을 붙여주는데 익숙해지긴 했지만 IntegrationTest에 붙여서 공통적인 부분을 분리할 수 있는 건 분리하는게 좋다고 생각해요. SuppressWarnings의 경우 commit 하기 전에 경고를 확인하고 수정할 수 있지만, 테스트 명은 비교적으로 까먹기 쉽다고도 생각해서요! (개인적인 의견입니다 ㅎㅎ...) 요 부분은 의견 한 번 맞춰보시죠~

MoonJeWoong · 2023-11-16T01:35:54Z

backend/src/test/java/harustudy/backend/integration/IntegrationTest.java

-    void setUp() {
-        this.mockMvc = MockMvcBuilders.webAppContextSetup(webApplicationContext).build();
-    }
-
-    protected void setMockMvc() {
-        this.mockMvc = MockMvcBuilders.webAppContextSetup(webApplicationContext).build();
-    }
-


꼼꼼한 리팩토링 👍

MoonJeWoong · 2023-11-16T01:46:14Z

backend/src/test/java/harustudy/backend/integration/IntegrationTest.java

+    @Autowired
+    protected GenerationStrategy generationStrategy;
+


요 GenerationStrategy를 다른 통합테스트에서도 participantCode를 생성하는데 사용했다면 IntegrationTest로 올려도 좋을 것 같습니다. 하지만 지금 저희 통합 테스트에서는 StudyIntegrationTest에서만 사용하는 것 같아 StudyIntegrationTest에서 관리하는 것이 괜찮을 것 같은데 어떻게 생각하시는지 궁금합니다 😃

오 저도 이 부분 고민했었는데,

현재는 IntegrationTest에서 모든 의존성이 드러나고 있어서 GenerationStrategy도 올리는게 낫지 않을까 생각을 했었습니다!
(매번 통합 테스트에서 어떤 의존성을 어디서 선언할지 고민하지 않아도 되고요)

그런데 이 부분은 저도 고민 많이 했던 내용이라, 팀 논의를 먼저 해보면 좋을 것 같아요!

MoonJeWoong · 2023-11-16T02:30:36Z

backend/src/test/java/harustudy/backend/integration/StudyIntegrationTest.java

-            Assertions.assertThat(location.split("/")).hasSize(4);
+            Assertions.assertThat(location.split("/")).hasSize(5);


assertSoftly() 내에서 softly.assertThat()을 사용해서 검증하지 않으면 저희가 예상하는 softAssertion을 수행할 수 없을 것 같아요. 저도 이번 리뷰를 하며 다시 찾아보고 실험해봤는데 지금 코드는 softlyAssertion이 되지 않더라구요! 여기 부분은 수정되어야 할 것 같습니다.

MoonJeWoong · 2023-11-16T02:47:38Z

backend/src/test/java/harustudy/backend/testutils/EntityManagerUtils.java

+
+import jakarta.persistence.EntityManager;
+
+public class EntityManagerUtils {


오프라인에서 얘기를 나눴던 것처럼 수료까지 얼마 남지 않은 시점에서 좀 더 우선순위가 높은 태스크를 집중해서 처리하는 것도 좋아서 일단 미뤄둬도 저는 괜찮을 것 같아요~

aak2075

고생 많으셨습니다!
아무래도 마이그레이션때문에 우선순위가 밀리다 보니 마이그레이션 끝나면 머지하면 될 것 같네요!

aak2075 · 2023-11-15T00:12:28Z

backend/src/test/java/harustudy/backend/integration/IntegrationTest.java

@@ -32,7 +33,7 @@
 import org.springframework.transaction.annotation.Transactional;
 import org.springframework.web.context.WebApplicationContext;

-@SuppressWarnings("NonAsciiCharacters")
+


섬세하군요👍

aak2075 · 2023-11-20T05:29:52Z

backend/src/test/java/harustudy/backend/acceptance/AcceptanceTest.java

+        assertThatCode(() -> objectMapper.readValue(response, StudyResponse.class))
+                .doesNotThrowAnyException();


스타일 차이이긴 한데 저는 assertThatCode() 취향이기는 합니다.
다른 부분도 assertThat() + isEqualTo() 이런식으로 사용하니까 예외 검증하는 부분도 assertThatCode() + doesNotThrowxxx() 이런식으로 하는게 일관적이고 개인적으로는 읽기 편하다고 느낍니다.
하지만 큰 차이는 아니라서 팀이 결정한다면 어느 쪽이든 좋아요:)

woosung1223 added 8 commits November 13, 2023 23:04

refactor: 인수 테스트 복구

bc56f07

chore: 불필요한 코드 제거, 스타일 조정

c209ebd

chore: 불필요한 어노테이션 삭제

1fa27ea

refactor: @AutoConfigureMockMvc를 사용하고 있으므로 setUp 메소드 삭제

3a2aca5

chore: GenerationStrategy를 IntegrationTest로 이동, 불필요한 의존성 제거

9c296bc

refactor: 인증 관련 메소드를 상위 클래스에 두지 않고 AuthIntegrationTest로 이동

4b08854

chore: @Disabled 처리된 테스트 복구

431ad83

chore: 메소드명 변경, 모든 테스트가 TestEntityManager가 아닌 EntityManager를 사용하도…

81f6419

…록 변경

woosung1223 added BE 백엔드 작업 refactor labels Nov 13, 2023

woosung1223 self-assigned this Nov 13, 2023

woosung1223 linked an issue Nov 13, 2023 that may be closed by this pull request

[BE] 테스트 코드 리팩토링 #729

Open

1 task

jaehee329 approved these changes Nov 15, 2023

View reviewed changes

MoonJeWoong requested changes Nov 16, 2023

View reviewed changes

aak2075 approved these changes Nov 20, 2023

View reviewed changes

woosung1223 added 4 commits November 21, 2023 16:58

chore: unused import 제거

13bb5a6

chore: softly 선언이 되고 있지 않던 문제 해결

bc6e3bf

chore: 매직 넘버 제거

2a90fdf

chore: 인수 테스트 로그인 순서 복구

5d0cb9a

MoonJeWoong approved these changes Nov 22, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BE] 테스트 코드 리팩토링 #730

[BE] 테스트 코드 리팩토링 #730

woosung1223 commented Nov 13, 2023 •

edited

Loading

jaehee329 left a comment

jaehee329 Nov 15, 2023

MoonJeWoong Nov 16, 2023

jaehee329 Nov 15, 2023

MoonJeWoong Nov 16, 2023

aak2075 Nov 20, 2023

woosung1223 Nov 21, 2023

jaehee329 Nov 22, 2023

jaehee329 Nov 15, 2023

MoonJeWoong Nov 16, 2023

woosung1223 Nov 21, 2023

jaehee329 Nov 15, 2023

woosung1223 Nov 21, 2023

jaehee329 Nov 15, 2023

MoonJeWoong Nov 16, 2023

jaehee329 Nov 16, 2023

jaehee329 Nov 15, 2023

MoonJeWoong Nov 16, 2023

MoonJeWoong left a comment •

edited

Loading

MoonJeWoong Nov 16, 2023

MoonJeWoong Nov 16, 2023

MoonJeWoong Nov 16, 2023

woosung1223 Nov 21, 2023

MoonJeWoong Nov 16, 2023

MoonJeWoong Nov 16, 2023

MoonJeWoong Nov 16, 2023

MoonJeWoong Nov 16, 2023

woosung1223 Nov 21, 2023

MoonJeWoong Nov 16, 2023

MoonJeWoong Nov 16, 2023

aak2075 left a comment

aak2075 Nov 15, 2023

aak2075 Nov 20, 2023

	return Long.parseLong(parsed[4]);
	return Long.parseLong(parsed[parsed.length-1]);

		assertThatCode(() -> objectMapper.readValue(response, StudyResponse.class))
		.doesNotThrowAnyException();


		@SuppressWarnings("NonAsciiCharacters")
		@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)

		Assertions.assertThat(location.split("/")).hasSize(4);
		Assertions.assertThat(location.split("/")).hasSize(5);


		import jakarta.persistence.EntityManager;

		public class EntityManagerUtils {

[BE] 테스트 코드 리팩토링 #730

Are you sure you want to change the base?

[BE] 테스트 코드 리팩토링 #730

Conversation

woosung1223 commented Nov 13, 2023 • edited Loading

관련 이슈

구현 기능 및 변경 사항

스크린샷(선택)

jaehee329 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoonJeWoong 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

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

aak2075 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woosung1223 commented Nov 13, 2023 •

edited

Loading

MoonJeWoong left a comment •

edited

Loading