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] 테스트 코드 리팩토링 #730

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from
Open
Changes from 1 commit
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
Copy link
Collaborator

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
@@ -1,6 +1,7 @@
package harustudy.backend.acceptance;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍 반영하겠습니다!!


import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatCode;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.BDDMockito.given;
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
Expand Down Expand Up @@ -55,29 +56,21 @@
@Transactional
class AcceptanceTest {

@MockBean
private OauthClients oauthClients;
@Autowired
private MockMvc mockMvc;

@Autowired
private ObjectMapper objectMapper;

@Autowired
private MockMvc mockMvc;
@MockBean
private OauthClients oauthClients;

@Autowired
private JwtTokenProvider jwtTokenProvider;

@Autowired
private TokenConfig tokenConfig;

@Autowired
private WebApplicationContext webApplicationContext;

@BeforeEach
void setUp() {
this.mockMvc = MockMvcBuilders.webAppContextSetup(webApplicationContext).build();
}

@Test
void 회원으로_스터디를_진행한다() throws Exception {
LoginResponse 로그인_정보 = 구글_로그인을_진행한다();
Expand All @@ -94,9 +87,9 @@ void setUp() {

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

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

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

회원으로_스터디를_진행한다();
회원으로_스터디를_진행한다();
LoginResponse 로그인_정보 = 구글_로그인을_진행한다();
List<StudyResponse> 회원으로_완료한_스터디_목록 = 회원으로_진행했던_모든_스터디_목록을_조회한다(로그인_정보);
for (StudyResponse 스터디_정보 : 회원으로_완료한_스터디_목록) {
스터디_종료_후_결과_조회(로그인_정보, 스터디_정보.studyId());
Expand Down Expand Up @@ -124,10 +117,9 @@ void setUp() {
참여_코드로_스터디_아이디를_얻는다(로그인_정보, 참여_코드);
}

private List<StudyResponse> 회원으로_진행했던_모든_스터디_목록을_조회한다(LoginResponse 로그인_정보)
throws Exception {
long memberId = Long.parseLong(jwtTokenProvider
.parseSubject(로그인_정보.tokenResponse().accessToken(), tokenConfig.secretKey()));
private List<StudyResponse> 회원으로_진행했던_모든_스터디_목록을_조회한다(LoginResponse 로그인_정보) throws Exception {
long memberId = Long.parseLong(
jwtTokenProvider.parseSubject(로그인_정보.tokenResponse().accessToken(), tokenConfig.secretKey()));

MvcResult result = mockMvc.perform(
get("/api/v2/studies")
Expand All @@ -137,8 +129,7 @@ void setUp() {
.andReturn();

String jsonResponse = result.getResponse().getContentAsString(StandardCharsets.UTF_8);
StudiesResponse StudiesResponse = objectMapper.readValue(jsonResponse,
StudiesResponse.class);
StudiesResponse StudiesResponse = objectMapper.readValue(jsonResponse, StudiesResponse.class);

return StudiesResponse.studies();
}
Expand All @@ -150,6 +141,7 @@ void setUp() {

String jsonResponse = result.getResponse().getContentAsString(StandardCharsets.UTF_8);
TokenResponse tokenResponse = objectMapper.readValue(jsonResponse, TokenResponse.class);

return new LoginResponse(tokenResponse, null);
}

Expand All @@ -174,6 +166,7 @@ void setUp() {
String jsonResponse = result.getResponse().getContentAsString(StandardCharsets.UTF_8);
Cookie refreshToken = result.getResponse().getCookie("refreshToken");
TokenResponse tokenResponse = objectMapper.readValue(jsonResponse, TokenResponse.class);

return new LoginResponse(tokenResponse, refreshToken);
}

Expand All @@ -187,10 +180,10 @@ void setUp() {
.header(HttpHeaders.AUTHORIZATION, 로그인_정보.createAuthorizationHeader()))
.andExpect(status().isCreated())
.andReturn();
String locationHeader = result.getResponse().getHeader(HttpHeaders.LOCATION);

String locationHeader = result.getResponse().getHeader(HttpHeaders.LOCATION);
String[] parsed = locationHeader.split("/");
System.out.println(locationHeader);

return Long.parseLong(parsed[4]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

}

Expand Down Expand Up @@ -226,8 +219,7 @@ void setUp() {
.andExpect(status().isOk());
}

private void 스터디_상태를_다음_단계로_넘긴다(LoginResponse 로그인_정보, Long 스터디_아이디)
throws Exception {
private void 스터디_상태를_다음_단계로_넘긴다(LoginResponse 로그인_정보, Long 스터디_아이디) throws Exception {
mockMvc.perform(post("/api/v2/studies/{studyId}/next-step", 스터디_아이디)
.header(HttpHeaders.AUTHORIZATION, 로그인_정보.createAuthorizationHeader()))
.andExpect(status().isNoContent());
Expand All @@ -238,8 +230,7 @@ void setUp() {
Map.of("retrospect", "test"));
String jsonRequest = objectMapper.writeValueAsString(request);

mockMvc.perform(post("/api/v2/studies/{studyId}/contents/write-retrospect",
스터디_아이디)
mockMvc.perform(post("/api/v2/studies/{studyId}/contents/write-retrospect", 스터디_아이디)
.contentType(MediaType.APPLICATION_JSON)
.content(jsonRequest)
.header(HttpHeaders.AUTHORIZATION, 로그인_정보.createAuthorizationHeader()))
Expand Down Expand Up @@ -275,8 +266,7 @@ void setUp() {
.andReturn();

String response = result.getResponse().getContentAsString(StandardCharsets.UTF_8);
ParticipantCodeResponse jsonResponse = objectMapper.readValue(response,
ParticipantCodeResponse.class);
ParticipantCodeResponse jsonResponse = objectMapper.readValue(response, ParticipantCodeResponse.class);

assertThat(jsonResponse.participantCode()).hasSize(6);
return jsonResponse.participantCode();
Expand All @@ -289,8 +279,10 @@ void setUp() {
.header(HttpHeaders.AUTHORIZATION, 로그인_정보.createAuthorizationHeader()))
.andExpect(status().isOk())
.andReturn();

String response = result.getResponse().getContentAsString(StandardCharsets.UTF_8);
Assertions.assertDoesNotThrow(() -> objectMapper.readValue(response,
StudyResponse.class));

assertThatCode(() -> objectMapper.readValue(response, StudyResponse.class))
.doesNotThrowAnyException();
Comment on lines +280 to +281
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

}
}