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

SourceCodeService 테스트 추가 #653

Merged
merged 12 commits into from
Sep 20, 2024
Merged

Conversation

jminkkk
Copy link
Contributor

@jminkkk jminkkk commented Sep 16, 2024

⚡️ 관련 이슈

close #645

📍주요 변경 사항

  1. 생성, 삭제, 조회, 수정 등에 대한 테스트 작성

🎸기타

  1. 테스트를 하면서 로직이 이상한 경우 또는 검증이 되어있지 않는 경우가 다수 있었습니다. 해당 pr은 테스트에 대한 것이기 때문에 main 로직의 수정이 필요한 경우에 대해서는 우선 작성하고 @disable 처리를 해놓았습니다.

이슈 #650 에 여러가지 문제 사항들을 정리를 해두었으니 확인 부탁드립니다.

  1. 또 현재 소스 코드 수정 로직이 복잡합니다. (매우)

열심히 작성했지만 놓친 엣지 케이스나 잘못 작성된 부분이 있다면 알려주세요~

@jminkkk jminkkk self-assigned this Sep 16, 2024
@jminkkk jminkkk added refactor 요구사항이 바뀌지 않은 변경사항 BE 백엔드 labels Sep 16, 2024
@jminkkk jminkkk added this to the 5차 스프린트🍗 milestone Sep 16, 2024
Copy link
Contributor

@zangsu zangsu left a comment

Choose a reason for hiding this comment

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

예외 상황들을 정말 꼼꼼하게 잘 추가해 주셨네요~~ 👍
몇가지 제안 사항 남겨 두었으니 확인해 주세요~~

@jminkkk jminkkk changed the title TemplateTagService 테스트 추가 SourceCodeService 테스트 추가 Sep 18, 2024
Copy link
Contributor

@kyum-q kyum-q left a comment

Choose a reason for hiding this comment

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

안녕하세요 몰리 ~ 즐거운 추석 보냈나요 ~
다양한 실패 케이스까지 추가한 거 완전 굿 👍 수고하셨어요 😊

junit 사용에 관한 코멘트 남겼어요 확인해주세요. 그리고 개행 처리가 자동으로 예쁘게 안되죠..ㅜㅜ 스타일을 다시 건들던가 해야겠네요 ! 개행 처리에 관한 코멘트도 몇가지 남겨뒀습니다.

Copy link
Contributor

@HoeSeong123 HoeSeong123 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다 몰리!!
소스 코드가 우리 서비스의 중심이 되는 부분이라 테스트할 부분이 굉장히 많았는데 정말 잘해주셨네요.
역시 꼼꼼몰리
꼼몰 최고!!
아주 간단한 피드백 몇 개 남겼습니다. 놓칠 수도 있을까봐 보이는 부분마다 집어놔서 반복되는 피드백들도 많습니다ㅎㅎ
고생하셨습니다.


@Nested
@DisplayName("소스 코드 생성")
class CreateSourceCodes {
Copy link
Contributor

Choose a reason for hiding this comment

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

소스 코드의 순서가 올바른지에 대한 검증도 필요할 것 같습니다!
예를 들면 1, 3, 4 이런 식으로 오는 경우에 대한 예외처리입니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wowowowo 꼼초;;;;;

생각하지도 못했네요 🔥
추가했습니다 심지어 검증 코드가 없나봐요 대박~
이슈에도 추가해놓을게요

// when & then
assertAll(
() -> assertThat(sourceCodeService.getByTemplateAndOrdinal(template, 2)).isEqualTo(sourceCode2),
() -> assertThat(sourceCodeService.getByTemplateAndOrdinal(template, 1)).isEqualTo(sourceCode1));
Copy link
Contributor

Choose a reason for hiding this comment

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

일부러 순서 뒤집어서 검사하는 꼼꼼함 👍
꼼꼼몰리
꼼몰


@Nested
@DisplayName("템플릿과 순서에 해당하는 소스 코드 조회")
class getByTemplateAndOrdinal {
Copy link
Contributor

Choose a reason for hiding this comment

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

(고민) 저희 코드 로직상 같은 순서의 소스코드가 테이블에 추가되는 경우(템플릿 업데이트시)가 있습니다.
물론 추가됐다가 바로 사라지기는 하지만 중복된 순서에 대한 검증도 필요하지 않을까요?
fetchByTemplateAndOrdinal를 할 때는 반드시 모든 소스코드가 중복되지 않은 순서를 가지는 것을 보장해야 할 것 같습니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
class getByTemplateAndOrdinal {
class GetByTemplateAndOrdinal {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fetchByTemplateAndOrdinal를 할 때는 반드시 모든 소스코드가 중복되지 않은 순서를 가지는 것을 보장해야 할 것 같습니다.

혹시 이 부분이 조금 이해가 안되는데 어떤 경우가 있을지 설명해줄 수 있나요~?
특정 Template의 모든 소스코드를 fetchByTemplateAndOrdinal 해서 Ordinal 를 체크해야 한다는 것일까요?

Copy link
Contributor

@HoeSeong123 HoeSeong123 Sep 20, 2024

Choose a reason for hiding this comment

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

제가 생각했던 것은 fetchByTemplateAndOrdinal이 단 하나의 값을 반환한다는 보장이 없으니 List로 반환한 후에 해당 리스트에 값이 하나만 있는지를 검증하는 로직을 추가하는 느낌이었습니다.
그런데 생각하면 생각할수록 별로네요..


@Nested
@DisplayName("템플릿에 해당하는 소스 코드 조회")
class findSourceCodesByTemplate {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
class findSourceCodesByTemplate {
class FindSourceCodesByTemplate {


@Nested
@DisplayName("소스 코드 수정")
class updateSourceCodes {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
class updateSourceCodes {
class UpdateSourceCodes {


@Nested
@DisplayName("id에 해당하는 모든 소스 코드 삭제")
class deleteByIds {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
class deleteByIds {
class DeleteByIds {

Comment on lines 228 to 230
() -> assertThat(
sourceCodeRepository.fetchByTemplateAndOrdinal(template, 2).getFilename()).isEqualTo(
"새로운 제목1"),
Copy link
Contributor

Choose a reason for hiding this comment

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

줄바꿈이 조금 어색한 것 같아요. 이런 줄바꿈은 어떤가요? 사실 둘 다 좀 어색한 거 같아서 이건 몰리 편한대로 정해주세요~~

Suggested change
() -> assertThat(
sourceCodeRepository.fetchByTemplateAndOrdinal(template, 2).getFilename()).isEqualTo(
"새로운 제목1"),
() -> assertThat(sourceCodeRepository.fetchByTemplateAndOrdinal(template, 2).getFilename())
.isEqualTo("새로운 제목1"),

Comment on lines 296 to 298
() -> assertThat(
sourceCodeRepository.fetchByTemplateAndOrdinal(template, 1).getFilename()).isEqualTo(
"새로운 제목3")
Copy link
Contributor

Choose a reason for hiding this comment

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

똑같은 줄바꿈 피드백인데 놓칠까봐 잡아놓습니다! 마찬가지로 선택은 몰리가 편한대로!!

Suggested change
() -> assertThat(
sourceCodeRepository.fetchByTemplateAndOrdinal(template, 1).getFilename()).isEqualTo(
"새로운 제목3")
() -> assertThat(sourceCodeRepository.fetchByTemplateAndOrdinal(template, 1).getFilename())
.isEqualTo("새로운 제목3")

Copy link
Contributor

@zeus6768 zeus6768 left a comment

Choose a reason for hiding this comment

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

고생 많았어요 몰리~!! 꼼몰짱

@DisplayName 전반에 대해 다음의 피드백을 적용해주세요 😁

image

@jminkkk
Copy link
Contributor Author

jminkkk commented Sep 20, 2024

고생 많았어요 몰리~!! 꼼몰짱

@DisplayName 전반에 대해 다음의 피드백을 적용해주세요 😁

image

안녕하세요 제우스
직전 레포지토리 테스트 때의 코멘트군요.
당시 논의 때 테스트 코드에서 @nested 사용 시 @DisplayName의 문체나, @nest의 DisplayName을 중복으로 작성할 것인지는 자율로 하기로 결정했었던 걸로 기억합니다.
형식은 "테스트 이름 성공(or 실패): 이유 또는 추가적으로 설명이 필요한 내용"만 지키면 되구요.

이 pr을 포함해 다른 pr에도 비슷하게 작성되어 있어서 저 이미지와 완전하게 동일한 형식으로 가려면 다시 논의를 해야할 것 같아요.

Copy link
Contributor

@zeus6768 zeus6768 left a comment

Choose a reason for hiding this comment

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

몰리 정말 고생 많았어요! 👍👍

DisplayName 컨벤션은 크루들과 한 번 더 가볍게 얘기하면 좋겠네요!

Copy link
Contributor

@HoeSeong123 HoeSeong123 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

@kyum-q kyum-q left a comment

Choose a reason for hiding this comment

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

몰리 짱 수고하셨어요 !!

@zangsu
Copy link
Contributor

zangsu commented Sep 20, 2024

고생 많으셨습니다~! 20000 머지 할게요~

@zangsu zangsu merged commit 42280db into dev/be Sep 20, 2024
7 checks passed
@zangsu zangsu deleted the test/645-template-tag-service branch September 20, 2024 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BE 백엔드 refactor 요구사항이 바뀌지 않은 변경사항
Projects
Status: Weekend Done
Development

Successfully merging this pull request may close these issues.

5 participants