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

test:댓글API 테스트 작성 #50

Merged
merged 5 commits into from
Aug 23, 2024
Merged

test:댓글API 테스트 작성 #50

merged 5 commits into from
Aug 23, 2024

Conversation

s-hwan
Copy link
Contributor

@s-hwan s-hwan commented Aug 21, 2024

📌 Related Issue

#42

🚀 Description

-댓글 수정 test코드를 작성해봤습니다

📢 Review Point

  • 테스트 코드 너무 어려워요 .. 맞는지 모르겠네여

📚Etc (선택)

  • 테스트 통과는 하는데.. 이게 테스트 코드 로직이 이상해서 그냥 늘 통과하는 건지 그런거는 어떻게 검증하나요..?

@s-hwan s-hwan added the test 테스트 코드 추가 label Aug 21, 2024
Copy link
Contributor

@rladmstn rladmstn left a comment

Choose a reason for hiding this comment

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

사실 테스트 코드는.. 공백으로 내도 통과하기 때문에 본인이 꼼꼼하게 짜는 수밖에 없다고 개인적으로 생각합니다 모든 경우의 수를 쪼개보고 꼼꼼하게 테코 만드는 연습을 합시다! 코멘트 드린 곳들에 검증 코드 추가 해주세요!

그리고 전에 댓글 수정 API PR 올리셨을 때 제가 createdAt을 수정하는 대신 updatedAt 필드 만들어서 수정 날짜를 갱신하는 건 어떻겠냐는 의견을 드렸었는데 혹시 반영이 안된걸까요?? 어떻게 되는건지 알려주세요!


// then
verify(commentRepository).findById(request.commentId());
assertEquals("Updated content", comment.getContent());
Copy link
Contributor

Choose a reason for hiding this comment

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

저희 댓글 수정할 때 댓글 생성 시간? 수정시간도 업데이트 되는 걸로 알고 있습니다 그 부분 검증이 없네요! 변경되는 모든 필드에 대해서는 검증을 붙여주시는 게 좋아요

@s-hwan
Copy link
Contributor Author

s-hwan commented Aug 21, 2024

사실 테스트 코드는.. 공백으로 내도 통과하기 때문에 본인이 꼼꼼하게 짜는 수밖에 없다고 개인적으로 생각합니다 모든 경우의 수를 쪼개보고 꼼꼼하게 테코 만드는 연습을 합시다! 코멘트 드린 곳들에 검증 코드 추가 해주세요!

그리고 전에 댓글 수정 API PR 올리셨을 때 제가 createdAt을 수정하는 대신 updatedAt 필드 만들어서 수정 날짜를 갱신하는 건 어떻겠냐는 의견을 드렸었는데 혹시 반영이 안된걸까요?? 어떻게 되는건지 알려주세요!

아 그 updateAt 이 메소드에서 매개변수로 받는데 사용은 안한다고 했을 때 그냥 updateAt이 필요하지 않다는 줄 이해해서 만들지 않았는데 지금 다시 글을 읽어보니 이해가 됐습니다 다시 반영하도록하겠습니다!

@s-hwan s-hwan requested a review from rladmstn August 21, 2024 13:59
@s-hwan
Copy link
Contributor Author

s-hwan commented Aug 21, 2024

@s-hwan
Copy link
Contributor Author

s-hwan commented Aug 21, 2024

시간 검증을 할 때 equals로 하면 안되는군요..

@hwangjokim
Copy link
Contributor

LocalDateTime의 정밀도는 마이크로초입니다. 그래서 Assert에서 updateAt과 LocalDateTime.now()로 비교하는 건 굉장히 나이브하고, 타이밍 이슈가 생길 가능성이 높습니다.

@s-hwan
Copy link
Contributor Author

s-hwan commented Aug 21, 2024

LocalDateTime의 정밀도는 마이크로초입니다. 그래서 Assert에서 updateAt과 LocalDateTime.now()로 비교하는 건 굉장히 나이브하고, 타이밍 이슈가 생길 가능성이 높습니다.

isCloseTo로 비교하는 방법도 문제가 생길 가능성이 높다는 말인가요?!

@hwangjokim
Copy link
Contributor

hwangjokim commented Aug 21, 2024

옙. pc 리소스를 많이 탑니다. �나이브한건 마찬가지여서, LocalDateTime.now()를 모킹해야 할 것 같아요
생각해보니 그냥 업데이트 이전 작성 시간을 변수에 담아두고, 신규 시간이 더 이후인지 검사하는게 좋을것같네요

Copy link
Contributor

@hwangjokim hwangjokim left a comment

Choose a reason for hiding this comment

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

사실 테스트코드 + 바쁨 이슈로 제대로 못봄 ㅈㅅ

@@ -61,7 +61,7 @@ public ResponseEntity<Object> deleteComment(@AuthedUser User user, @RequestParam
@PutMapping
@Operation(summary = "댓글 수정 API")
public ResponseEntity<String> modifyComment(@AuthedUser User user,
@Valid @RequestBody ModifyCommentRequest request, Errors errors) {
@Valid @RequestBody UpdateCommentRequest request, Errors errors) {
Copy link
Contributor

Choose a reason for hiding this comment

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

이 diff는 왜 생긴거예요? PR이랑 무관해보이는데..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아 Modify 이름이 짜쳐서 바꿨읍니닫..

verify(commentRepository).findById(request.commentId());
assertEquals("Updated content", comment.getContent());
assertThat(comment.getUpdatedAt()).isCloseTo(LocalDateTime.now(), within(1, ChronoUnit.SECONDS));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

여기 시간검증 말한것처럼 나이브한것 고치기 위해
LocalDateTime.now()자체를 mockStatic 써서 모킹하거나 하는 등 방법으로 수정해주세요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이게 지금 보이네요 다시 해보겠습니다

@s-hwan
Copy link
Contributor Author

s-hwan commented Aug 22, 2024

괜찮습니다.. 테코는 근데 진짜로 정신 나갈거같아요 너무 어려움 ..ㅠㅠㅠ

@s-hwan
Copy link
Contributor Author

s-hwan commented Aug 22, 2024

황조님 피드백을 반영해봤읍니다..

@s-hwan
Copy link
Contributor Author

s-hwan commented Aug 22, 2024

하하 테스트 통과를 못하네요 ^^

@hwangjo-v6x
Copy link

fail 났을 때 details 누르면 자세한 내용 볼 수 있어요
그리고 테스트코드 작업시에는 커밋/푸시 전 직접 한번 테스트 돌려보고 올리면 좀 더 좋구요
image

@s-hwan s-hwan requested review from rladmstn and hwangjokim August 22, 2024 16:30
@s-hwan
Copy link
Contributor Author

s-hwan commented Aug 22, 2024

테스트 코드 싫다..

@hwangjokim
Copy link
Contributor

어렵고 모를땐 톡방이나 이슈에 멘션걸고 그냥 무지성 질문 ㄱㄱ하시죠
아니면 그냥 어려운 내용을 이슈에 기록해도 좋구요
죽겠다고 티내야 주변에서 많이 붙어서 도와줍니다!

@s-hwan
Copy link
Contributor Author

s-hwan commented Aug 22, 2024

어렵고 모를땐 톡방이나 이슈에 멘션걸고 그냥 무지성 질문 ㄱㄱ하시죠 아니면 그냥 어려운 내용을 이슈에 기록해도 좋구요 죽겠다고 티내야 주변에서 많이 붙어서 도와줍니다!

ㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋ든든합니다 무지성 질문 가겠습니다

@hwangjokim hwangjokim linked an issue Aug 22, 2024 that may be closed by this pull request
1 task
Copy link
Contributor

@rladmstn rladmstn left a comment

Choose a reason for hiding this comment

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

테코와의 사투.. 수고하셨습니다

Copy link
Contributor

@hwangjokim hwangjokim left a comment

Choose a reason for hiding this comment

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

다른 두 분이 리뷰 잘 해줬을것이라 믿고 어프로브 합니다~

@hwangjokim hwangjokim merged commit d7aeb80 into develop Aug 23, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test 테스트 코드 추가
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test:댓글 수정API 테스트 코드 추가
5 participants