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

@Disabled 된 테스트 검토 및 사용하지 않는 메서드 제거 #991

Merged
merged 9 commits into from
Dec 27, 2024

Conversation

kyum-q
Copy link
Contributor

@kyum-q kyum-q commented Dec 24, 2024

⚡️ 관련 이슈

#948

📍주요 변경 사항

주요 변경 사항에 대해 작성해주세요.

  • 사용하지 않는 메서드 제거 ( COMMIT )
  • @Disabled 테스트 확인 후 수정 혹은 제거
    • 수정 혹은 제거한 이유에 대해서는 이슈에 작성
  • 소스코드 순서 검증 시 현재까지는 순서가 뒤죽박죽이면 예외 발생하는 방식을 변경
    • 소스 코드 순서 검증 시 정렬하여 1부터 시작해 빠지는 값이 없다면 예외 발생하지 않게 수정
    • 이유 : 이전 코드는 소스코드 업데이트 시 무조건 업데이트 소스코드가 생성 소스코드보다 앞선 순서여야하기 때문. 추후 소스코드 순서 변경을 고려해서 구현한 코드인 만큼 생성 소스코드가 수정 소스코드보다 클 수 있어야한다고 생각했습니다.

이해를 돕기 위해 코드를 가져오면 소스코드 순서를 확인하기 위해 사용했던 순서를 알아내는 코드는 다음과 같았습니다.

    public List<Integer> extractSourceCodesOrdinal() {
        return Stream.concat(
                        updateSourceCodes.stream().map(UpdateSourceCodeRequest::ordinal),
                        createSourceCodes.stream().map(CreateSourceCodeRequest::ordinal)
                )
                .toList();
    }
  • @jminkkk 몰리가 태그 동시성 구현하면서 @Disabled로 만들었던 테스트를 제거했습니다. 이유는 사용하지 않는 이유로 제가 제거한 메서드인 templateRepository.findByMemberId을 사용해서 입니다 ! 혹시 다른 의견이 있거나, 해당 메서드를 사용하지 않고 테스트 코드를 두는 방식이 있다면 알려주세요 ~

  • @zeus6768 , @zangsu 제우스가 만든 voc의 @disabled 테스트와 짱수가 만든 RandomSaltGeneratorTest의 난수 확인 @disabled 테스트의 추후 발전 가능성이 있는지 궁금합니다! 사용하지 않고 발전 가능성이 없다면 지우는 방향은 어떨까요? 동의하신다면 해당 PR에서 제거하고 싶습니다~

🎸기타

고려해야 하는 내용을 작성해 주세요.

코드를 확인할 때 Commit 단위로 보면 더 편하실 것 같아요 ~
뒤로 갈수록 그렇게 예쁘게 분리하진 못했지만 그래도 그냥 보는 것보다는 편할 듯 싶습니다~

🍗 PR 첫 리뷰 마감 기한

12/27 20:00

@kyum-q kyum-q added refactor 요구사항이 바뀌지 않은 변경사항 BE 백엔드 labels Dec 24, 2024
@kyum-q kyum-q added this to the 7차 스프린트 💭 milestone Dec 24, 2024
@kyum-q kyum-q self-assigned this Dec 24, 2024
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.

숙변이 내려가는 기분이에요 👍👍👍👍👍👍👍👍👍👍

@zeus6768 , @zangsu 제우스가 만든 voc의 @disabled 테스트와 짱수가 만든 RandomSaltGeneratorTest의 난수 확인 @disabled 테스트의 추후 발전 가능성이 있는지 궁금합니다! 사용하지 않고 발전 가능성이 없다면 지우는 방향은 어떨까요? 동의하신다면 해당 PR에서 제거하고 싶습니다~

voc의 create_real_api() 테스트 메서드 지워도 무방합니다~!

@zangsu
Copy link
Contributor

zangsu commented Dec 27, 2024

짱수가 만든 RandomSaltGeneratorTest의 난수 확인 @disabled 테스트의 추후 발전 가능성이 있는지 궁금합니다! 사용하지 않고 발전 가능성이 없다면 지우는 방향은 어떨까요?

좋습니다~~!

Copy link
Contributor

@jminkkk jminkkk left a comment

Choose a reason for hiding this comment

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

image
수고했어요 켬미!!!!!!!!!! 최고야

@jminkkk 몰리가 태그 동시성 구현하면서 @disabled로 만들었던 테스트를 제거했습니다. 이유는 사용하지 않는 이유로 제가 제거한 메서드인 templateRepository.findByMemberId을 사용해서 입니다 ! 혹시 다른 의견이 있거나, 해당 메서드를 사용하지 않고 테스트 코드를 두는 방식이 있다면 알려주세요 ~

문제가 해결되었으니 제거해도 될 듯합니다~!


@Override
public List<Integer> extractSourceCodesOrdinal() {
return sourceCodes.stream()
.map(CreateSourceCodeRequest::ordinal)
.sorted()
Copy link
Contributor

Choose a reason for hiding this comment

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

👍👍👍👍

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.

아우 너무 깔끔해~~~!!
고생하셨어요 켬미~~!

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.

켬미 체거자나~~~

@HoeSeong123 HoeSeong123 merged commit e70bbf0 into dev/be Dec 27, 2024
6 checks passed
@HoeSeong123 HoeSeong123 deleted the fix/948-disable-test-check branch December 27, 2024 04: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