[TEST] 출석 관련 단위 테스트 추가 및 행사 출석 로직 수정 #254

KWY0218 · 2024-04-09T00:01:59Z

Related Issue 🚀

closed [TEST] 출석 관련 단위 테스트 추가 #253

Work Description ✏️

이번에 출석 관련 로직이 수정됨에 따라서 추후에 또 수정할 일이 있을 때 빠르게 검사하기 위해서 테스트 코드를 추가했습니다.
단위 테스트시 도메인을 생성하기 위해서 실제 비즈니스 로직에 영향이 안가도록 도메인 내에 protected 생성자를 만든 후에 도메인 객체를 상속 받는 Dummy 객체를 만들었습니다.
Attendance 도메인 내에 존재하는 getScore 함수와 getStatus 함수의 단위 테스트를 작성했습니다.
행사도 지각시 0.5 점을 부여하기 때문에 도메인 로직을 수정했습니다.

PR Point 📸

@DisplayName 테스트명이 괜찮은지 궁금합니다

height · 2024-04-09T00:02:02Z

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

yummygyudon

단위 테스트 야무지네요!!

개인적인 의견이 몇 가지 있어서 제안드려봅니다!!
또한 출석 상태 값 연산 부분도 반영 부탁드려요!!

yummygyudon · 2024-04-09T08:53:55Z

operation-domain/build.gradle

+    // test
+    testImplementation 'org.springframework.boot:spring-boot-starter-test'


우와 진짜 메이커스에서 테스트하는 것 감격...!!

yummygyudon · 2024-04-09T08:55:53Z

operation-domain/src/main/java/org/sopt/makers/operation/attendance/domain/Attendance.java

+    protected Attendance(Long id, Member member, Lecture lecture, AttendanceStatus status) {
+        this.id = id;
+        setMember(member);
+        setLecture(lecture);
+        this.status = status;
+    }
+


protected 접근제어자와 id 직접 주입하는 것으로 미뤄보아
테스트를 위한 생성자인 듯 한데 맞을까요?!

@yummygyudon
넵 맞습니다!

yummygyudon · 2024-04-09T09:01:08Z

operation-domain/src/main/java/org/sopt/makers/operation/attendance/domain/Attendance.java

@@ -103,7 +110,7 @@ public float getScore() {
 					yield 0f;
 				}
 			}
-			case EVENT -> this.status.equals(ATTENDANCE) ? 0.5f : 0f;
+			case EVENT -> this.status.equals(ABSENT) ? 0f : 0.5f;


제가 행사 점수 연산 방식을 전달 드리기 전에 올리신 PR인 것 같습니다!
행사 또한 마찬가지로 1차 출석 / 2차 출석 여부 상관 없이
출석 완료 1회일 경우 동일한 0.5점인 것은 확인했습니다!!

Score 조회 메서드는 원용씨가 하신대로 가면될 듯 하구
상단에 있는 AttendanceStatus getStatus() 메서드 내 로직도 반영 부탁드려요!!

현재는 2차 출석값만 확인하는 로직인 듯 합니다!!

as-is

return switch (this.lecture.getAttribute()) { case SEMINAR -> { if (first.getStatus().equals(ATTENDANCE) && second.getStatus().equals(ATTENDANCE)) { yield ATTENDANCE; } yield first.getStatus().equals(ABSENT) && second.getStatus().equals(ABSENT) ? ABSENT : TARDY; } case EVENT -> second.getStatus().equals(ATTENDANCE) ? ATTENDANCE : ABSENT; case ETC -> second.getStatus().equals(ATTENDANCE) ? PARTICIPATE : NOT_PARTICIPATE; };

to-be
이번에 변경된 세미나(SEMINAR)의 상태 값 연산과 동일하게 가면 될 것 같아요!

점수 반영은 "출석"과 "지각" 모두 +0.5 이긴 하나 상태는 달라야 합니다!

return switch (this.lecture.getAttribute()) { case SEMINAR -> { if (first.getStatus().equals(ATTENDANCE) && second.getStatus().equals(ATTENDANCE)) { yield ATTENDANCE; } yield first.getStatus().equals(ABSENT) && second.getStatus().equals(ABSENT) ? ABSENT : TARDY; } case EVENT -> { if (first.getStatus().equals(ATTENDANCE) && second.getStatus().equals(ATTENDANCE)) { yield ATTENDANCE; } yield first.getStatus().equals(ABSENT) && second.getStatus().equals(ABSENT) ? ABSENT : TARDY; case ETC -> second.getStatus().equals(ATTENDANCE) ? PARTICIPATE : NOT_PARTICIPATE; };

메서드 길이가 길어지는 문제와 함께 중복되는 코드가 발생하는 문제가 존재할 것 같은데
두 switch case 에 두 요소(SEMINAR & EVENT)를 합쳐서 처리해도 괜찮을 것 같아요!!
원용씨 의견은 어떤가요?!

@yummygyudon
넵 고려해서 행사 관련한 getStatus 테스트도 만들어서 반영하고 로직 수정해보겠습니다!
감사합니다!

8abf1d4 , e25492f

코드 중복이라고 생각하여 EVENT, SEMINAR 를 같은 CASE에 두는 방식으로 적용해봤습니다!

행사도 연산 점수 로직 바뀐거 확인했습니다 ! 동규님 꼼꼼하게 체크해주시고 바로 반영해준 원용님 감사합니다 ~!

yummygyudon · 2024-04-09T15:36:09Z

...domain/src/test/java/org/sopt/makers/operation/attendance/domain/AttendanceGetScoreTest.java

+    }
+
+    @Nested
+    @DisplayName("세미나 출석 관련 테스트")


대분류(상위 클래스) 이름 @DisplayName은 [] 등으로 단어로 보여주는 것은 어떨까요?!
또한 Seminar는 Status 테스트와 Score 점수를 분리해서 명시한 다음 진행하는 것은 어떨지 제안드려봅니다!

ex. [세미나 출석 점수 테스트] , [세미나 출석 상태 테스트] 등

@yummygyudon
의견 반영해서 좀 더 명시적으로 @DisplayName 설정해보겠습니다. 감사합니다!

7b90765 반영했습니다!

yummygyudon · 2024-04-09T15:39:33Z

...omain/src/test/java/org/sopt/makers/operation/attendance/domain/AttendanceGetStatusTest.java

+        subLecture1 = new SubLectureDummy(1L, lecture, 1, LocalDateTime.now(), "000001");
+        subLecture2 = new SubLectureDummy(2L, lecture, 2, LocalDateTime.now(), "000002");
+
+        subLectures = new ArrayList<>();
+        subLectures.add(subLecture1);
+        subLectures.add(subLecture2);
+
+        attendances = new ArrayList<>();
+        attendance = new AttendanceDummy(1L, member, lecture, AttendanceStatus.ABSENT);
+        attendances.add(attendance);


조금 setUp() 메서드가 길고 유연성이 부족한 것처럼 보여요!!

Dummy Data를 만드는 것에 대해서는 행위를 나타내는 함수로 분리하여 setUp() 메서드 내에서는
"Dummy Data를 만드는 행위"를 나타내는 방식은 어떨까요?!

추후에 특정 Dummy Data의 스펙을 수정할 때도 관리하기 용이할 것 같아요!!

List<SubLectureDummy> dummySubLectures = makeDummySubLecture(2); List<AttendanceDummy> dummyAttendances = makeDummyAttendance(2); private List<SubLectureDummy> makeDummySubLecture(int quantity) { // for문 혹은 Stream 을 통해 quantity 값을 활용하여 id, round, code 등을 만들어서 반환할 수 있을 듯! ... } private List<AttendanceDummy> makeDummyAttendance(int quantity) { // for문 혹은 Stream 을 통해 quantity 값을 활용하여 id 주입하고 Random 함수를 통해 AttendanceStatus를 주입할 수 있을듯!! ... }

@yummygyudon

긴 setUp 에 머리 아팠었는데, 메서드로 분리하면 setUp 과정도 좀 더 명시적으로 변할 것 같아서 좋을 것 같습니다!
이 부분 고려해서 반영하겠습니다!

@yummygyudon

해당 부분을 함수화 해서 변경하는 방식으로 생각해 보다가 아래 사항들이 맘에 걸려 기존 상태를 유지했습니다.

set up 함수는 실제 테스트를 위한 준비 동작이므로 크게 중요하지 않다고 판단했고, 코드가 유연하지 않고 메서드 길이가 길어도 된다고 생각합니다.

dummy 객체를 인스턴스화 하는 메서드 만든다면 유지 보수를 해야 하는 범위가 커진다고 생각합니다.
추후 set up에서 테스트 로직을 수정해야 할 때 지금 상태라면, 값을 넣는 아규먼트만 수정하면 되는데
함수화를 한다면 함수를 수정하는 등 추가 작업이 수반된다고 생각하여서 테스트를 설정하는 데 드는 노력이 추가가 될 것 같다는 생각이 듭니다..!

이에 대해 어떻게 생각하시는지 궁금합니다..!

set up 함수는 실제 테스트를 위한 준비 동작이므로 크게 중요하지 않다고 판단했고, 코드가 유연하지 않고 메서드 길이가 길어도 된다고 생각합니다.

테스트 여부, 준비 동작 여부와는 상관없이 코드는 유연하고 가독성이 중요하다고 생각합니다!!
단순 테스트를 했다에 의의를 두고자한다면 상관없겠지만
테스트 또한 개발의 일부라고 생각한다면 나름대로의 의미를 가지는 것이라고 생각해요!

dummy 객체를 인스턴스화 하는 메서드 만든다면 유지 보수를 해야 하는 범위가 커진다고 생각합니다.
추후 set up에서 테스트 로직을 수정해야 할 때 지금 상태라면, 값을 넣는 아규먼트만 수정하면 되는데
함수화를 한다면 함수를 수정하는 등 추가 작업이 수반된다고 생각하여서 테스트를 설정하는 데 드는 노력이 추가가 될 것 같다는 생각이 듭니다..!

해당 테스트에 대해서만 생각한다면 본 의견에 대해 동의합니다!!

다만 첫 리뷰에서는 언급드리지 않은 내용인데
해당 테스트에 국한하지 않고 향후 추가적인 테스트 작성을 고려했을 때,
서로 다른 테스트 클래스들이더라도 @BeforeEach로 Dummy Data를 만드는 것은 동일할 것입니다.

이 때, 각 테스트 클래스 내에는 동일한 코드가 반복되어 들어갈 것이라고 생각했고
그럴 경우, 팩토리 메서드로 Dummy Data를 만드는 메서드를 구현해놓으면
서로 다른 테스트 클래스 내에서 재사용할 수 있고 필요에 따라 DummyData의 갯수, 속성 등을 편집할 수 있다는 장점이 있을 것이라고 생각했어요!!

해당 의견에 대한 원용이 의견도 궁금합니다!!

@yummygyudon

코드는 유연하고 가독성이 중요하다고 생각합니다!!

이 부분은 굉장히 동의합니다!

하지만 저는 각 테스트 케이스가 좀 더 독립적으로 실행되기 위해선 서로 다른 테스트 클래스에서 생성 메서드를 사용하는 것은 지양하고 싶다는 생각이 듭니다..!

왜냐하면 해당 생성 메서드를 통해서 테스트를 하려다가 추가 내용이 필요해서 메서드를 수정하려는 순간
다른 테스트 케이스에서 해당 메서드를 사용하는 것도 고려하면서 코드를 작성해야 하기 때문에
테스트 케이스가 많아질수록 더욱 관리가 힘들어 질 것이라 생각합니다..!

저는 테스트 코드는 가능한 빠르게 작성 되는 것이 중요하다고 생각합니다.
현재 set up 함수는 길고, 값을 일일이 넣는 시간이 걸리지만 이는 첫 테스트를 작성할 때만 시간이 걸리지
이후 더미 객체를 생성할 땐 간단히 복사 붙여놓기만 하면 되기 때문에 더 빠르게 작성이 가능하다고 생각합니다.

이로 인해 각각의 테스트 케이스는 좀 더 독립적인 테스트 케이스를 만들 수 있지 않을까란 생각이 들기 때문에
생성자를 이용해서 set up을 하는 것이 좋을 것 같다고 생각합니다..!

독립적인 테스트 케이스 활용을 위한다는 마음으로 고려해보니 원용이의 의견에 매우 공감되는 바입니다!!

yeseul106

정확한 로직 계산이 필요했었는데 테스트 코드 작성으로 인해서 추후 유지보수가 원활하게 될 것 같네요 !!! 감사합니다 !

제가 늦게봐서 이미 동규님도 좋은 리뷰 많이 주시고 리팩 모두 잘 되어있는 거 같네요 ! 죄송합니다 ㅠㅠ 다음부터는 빠르게 확인 하겠습니다 ! 고생하셨어요 ~~~

yeseul106 · 2024-04-12T03:54:18Z

operation-domain/src/main/java/org/sopt/makers/operation/attendance/domain/Attendance.java

@@ -103,7 +110,7 @@ public float getScore() {
 					yield 0f;
 				}
 			}
-			case EVENT -> this.status.equals(ATTENDANCE) ? 0.5f : 0f;
+			case EVENT -> this.status.equals(ABSENT) ? 0f : 0.5f;


행사도 연산 점수 로직 바뀐거 확인했습니다 ! 동규님 꼼꼼하게 체크해주시고 바로 반영해준 원용님 감사합니다 ~!

KWY0218 added 6 commits April 7, 2024 23:42

[CHORE] 도메인 모듈 내 test 의존성 추가

ffa2c20

[FEAT] Dummy 도메인 객체를 위한 생성자 구현

8f67dca

[FEAT] 각 도메인 별로 Dummy 도메인 객체 생성

ef9cff4

[TEST] Attendance 도메인 내 getStatus 테스트 로직 구현

246e287

[TEST] Attendance 도메인 내 getScore 단위테스트 작성

939e385

[REFACTOR] 행사 지각 시 0.5 점이 반환되도록 로직 수정

9ecbe65

KWY0218 requested review from yeseul106 and yummygyudon April 9, 2024 00:02

KWY0218 self-assigned this Apr 9, 2024

pull-request-size bot added the size/L label Apr 9, 2024

yummygyudon requested changes Apr 9, 2024

View reviewed changes

KWY0218 added 3 commits April 10, 2024 14:58

[CHORE] 행사 출석 점수 반환 단위 테스트명 명시적으로 수정

7b90765

[TEST] 행사 출석 상태 반환 단위 테스트 추가

8abf1d4

[REFACTOR] 행사일 때 출석 상태 반환 로직 수정

e25492f

yummygyudon approved these changes Apr 10, 2024

View reviewed changes

yeseul106 approved these changes Apr 12, 2024

View reviewed changes

KWY0218 added this pull request to the merge queue Apr 12, 2024

Merged via the queue into develop with commit a75c418 Apr 12, 2024
1 check passed

KWY0218 deleted the kwy_#253 branch April 12, 2024 04:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TEST] 출석 관련 단위 테스트 추가 및 행사 출석 로직 수정 #254

[TEST] 출석 관련 단위 테스트 추가 및 행사 출석 로직 수정 #254

KWY0218 commented Apr 9, 2024

height bot commented Apr 9, 2024

yummygyudon left a comment

yummygyudon Apr 9, 2024

yummygyudon Apr 9, 2024

KWY0218 Apr 10, 2024

yummygyudon Apr 9, 2024

yummygyudon Apr 9, 2024

KWY0218 Apr 10, 2024

KWY0218 Apr 10, 2024

yeseul106 Apr 12, 2024

yummygyudon Apr 9, 2024

KWY0218 Apr 10, 2024

KWY0218 Apr 10, 2024

yummygyudon Apr 9, 2024

yummygyudon Apr 9, 2024

KWY0218 Apr 10, 2024

KWY0218 Apr 10, 2024

yummygyudon Apr 10, 2024

KWY0218 Apr 10, 2024

yummygyudon Apr 10, 2024

yeseul106 left a comment

yeseul106 Apr 12, 2024

		// test
		testImplementation 'org.springframework.boot:spring-boot-starter-test'

[TEST] 출석 관련 단위 테스트 추가 및 행사 출석 로직 수정 #254

[TEST] 출석 관련 단위 테스트 추가 및 행사 출석 로직 수정 #254

Conversation

KWY0218 commented Apr 9, 2024

Related Issue 🚀

Work Description ✏️

PR Point 📸

height bot commented Apr 9, 2024

yummygyudon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeseul106 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment