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

SO1S-482 ABN 테스트 개발 #65

Merged
merged 10 commits into from
Nov 20, 2022
Merged

SO1S-482 ABN 테스트 개발 #65

merged 10 commits into from
Nov 20, 2022

Conversation

DPS0340
Copy link
Member

@DPS0340 DPS0340 commented Nov 19, 2022

ABN 테스트 개발

Tasks

  • 기존의 코드를 tests/v1 package로 마이그레이션
  • tests/v2 package 내부에 ABN 테스트 관련 코드 작성
  • v2 API 버저닝을 통한 ABN 테스트 엔드포인트 제공
  • 성공하는 단순한 테스트 코드 작성

Discussion

  • 버저닝을 제공해서 기존 v1 AB 테스트와는 다르게 새로운 패키지 구조를 작성하면서 개발했기 때문에 기존의 API가 보존되었지만, 완전히 새로운 도메인을 개발하는 수준으로 코드를 많이 작업했습니다. 추후 프론트엔드 연동이 완료된 후에도 v1 코드와 API를 보존하는게 맞을까요? 의견 부탁드립니다!
  • 서비스가 아닌 컨트롤러 메소드상에서 핵심 로직이 연동되어 서비스 단위 테스트가 아닌 컨트롤러를 DI로 가져와서 서비스 메소드를 호출하는 방법으로 진행했는데 기존의 MockMVC보다는 코드가 간결하고, 서비스 테스트보다는 커버리지가 높은 느낌을 받아 좋았습니다. 추후에 컨트롤러에서 테스트해야하는 까다로운 부분은 이렇게 작업하면 좋을 것 같아요 ㅎㅎ
  • 코드 리뷰 부탁드립니다~

Jira

  • SO1S-482

@DPS0340 DPS0340 requested a review from shinilseop November 19, 2022 15:29
@DPS0340 DPS0340 added the API API 기능이 추가됐습니다. label Nov 19, 2022
@DPS0340 DPS0340 self-assigned this Nov 19, 2022
Copy link
Contributor

@shinilseop shinilseop left a comment

Choose a reason for hiding this comment

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

고생하셨습니다~ 👍
테스트도 꼼꼼하게 작성해주신 것 같습니다~

Comment on lines +1 to +11
package io.so1s.backend.domain.test.v2.service.internal;

import io.so1s.backend.domain.test.v2.entity.ABNTest;

public interface ABNTestKubernetesService {

boolean deployABNTest(ABNTest abTest);

boolean deleteABNTest(ABNTest abTest);

}
Copy link
Contributor

Choose a reason for hiding this comment

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

쿠버네티스 서비스에서 별도의 내부 서비스로 빼셨는데 리소스 생성때 반복적인 코드의 생성이랑 번거로움 때문에 변경하신건가요 ??
저도 예전에 Deployment 생성이나 Job 생성당시 굳이 나눠야 하나 싶긴 했었는데 그래도 쿠버네티스 리소스 관리는 한곳에서 해야 나중에 편할것 같아서 따로 빼지는 않았었거든요.

Copy link
Member Author

@DPS0340 DPS0340 Nov 20, 2022

Choose a reason for hiding this comment

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

반복적인 코드 생성과 번거로움은 사실 큰 문제는 되지 않았는데, 500라인 이상으로 KubernetesService 클래스가 너무 커지니까 부분적으로 도메인에서 필요한 쿠버네티스 로직만 분리해서 기존 클래스의 코드를 조금 줄였습니다!
god class 패턴이 되어가는 것 같아서 조금 쪼갰어요 ㅎㅎ

지금 당장 리팩토링하기에는 시간이 모자라지만 나중에 리팩토링할 때는 Gateway같은 스테레오타입 객체 생성이 반복되는 부분은 외부 Builder 클래스를 만들어서 쪼개도 될 것 같은데, 이 부분은 어떻게 생각하시나요?

Copy link
Contributor

Choose a reason for hiding this comment

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

아하 그렇군요
저도 그정도로 길다면 별도로 쪼개는거 좋은거같습니다! 👍
나중에 별도로 빼서 만들어 놓으면 코드 가독성이나 개발할 때 더 편리하고 좋을 것 같습니다!

@DPS0340 DPS0340 merged commit f8ea702 into main Nov 20, 2022
@DPS0340 DPS0340 deleted the SO1S-482 branch November 20, 2022 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API API 기능이 추가됐습니다.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants