[4 - 9단계 - 방탈출 예약 관리] 구름(김민수) 미션 제출합니다. #108

alstn113 · 2024-04-22T11:20:41Z

안녕하세요 범블비! 구름 ⛅️ 입니다!

4-9 미션 구현은 오랜시간이 걸리지 않았던 것 같습니다.
거의 시간의 80퍼를 테스트를 어떻게 작성하면 좋을지 고민했던 것 같습니다.

데이터베이스와 함께 테스트를 해야했기 때문에 다음의 전략을 사용했습니다.

repository 부분은 @SpringBootTest와 @Transactional의 조합을 사용했습니다.
service 부분은 mockito를 이용해서 repository를 mocking해서 테스트했습니다.
controller부분은 BaseControllerTest에서 truncate query를 만들어서 실행했습니다.

어느 부분이 문제이고, 어떤 방법들이 있는지 메타인지가 되지 않아서 다양한 코드들을 찾아보면서 학습하고 있습니다.

다음으로 service 계층에서 dto를 리턴해야할 지, entity를 리턴해야할 지 모르겠습니다.
제 결론은 그냥 dto를 default로 하되, 부분적으로 entity를 쓰는게 맞다고 생각합니다. 왜냐하면 service에서 service를 사용하는 경우도 있기 때문입니다.
아니면 controller는 webDto를, service는 ServiceDto를 사용하는 방법도 있습니다. 그러나 이것은 너무 복잡해서 사용하지 않을 것 같습니다.

미리 리뷰 감사합니다. 범블비!

ddaaac

안녕하세요 구름 👋

구현 잘 해주셨네요.

몇 가지 코멘트 남겼으니 확인해주세요.

ddaaac · 2024-04-23T00:28:00Z

src/main/java/roomescape/controller/ReservationController.java

 import org.springframework.web.bind.annotation.RestController;
 import roomescape.dto.ReservationRequest;
 import roomescape.dto.ReservationResponse;
 import roomescape.service.ReservationService;

 @RestController
-@RequestMapping("/reservations")


@RequestMapping을 제거해주신 이유가 있을까요?

너무 당연하게 공통된 부분을 @RequestMapping으로 분리했었습니다.
그러나 전체 URL을 검색할 때 불편함이 있지 않겠냐는 말에 설득된 것 같습니다.

ddaaac · 2024-04-23T00:28:02Z

src/main/java/roomescape/controller/ReservationTimeController.java

+    public ReservationTimeController(ReservationTimeService reservationTimeService) {
+        this.reservationTimeService = reservationTimeService;
+    }


알고 계실 수 있겠지만 생성자 주입에 대한 글을 읽어봐도 좋을 것 같네요.

단일 책임의 원칙

DI Container와의 낮은 결합도

필드의 불변성 보장

순환 의존 방지

다시 한 번 짚고 넘어 갑니다!

ddaaac · 2024-04-23T00:28:07Z

src/main/java/roomescape/repository/JdbcReservationRepository.java

+    public List<Reservation> findAll() {
+        String sql = """
+                    SELECT
+                        r.id as reservation_id,
+                        r.name,
+                        r.date,
+                        t.id as time_id,
+                        t.start_at as time_value
+                    FROM reservation as r
+                    inner join reservation_time as t
+                    on r.time_id = t.id
+                """;
+
+        return jdbcTemplate.query(sql, rowMapper);


ReservationTimeRepository도 존재하는 상황에서 ReservationRepository에서 findAll을 수행하면 ReservTime까지 join할거라는 사실을 클라이언트에서 쉽게 알 수 있을까요?

ReservationTime이 Reservation의 속성이라서 굳이 알 필요가 없다고 생각합니다.
만약에 진짜 바꿔야한다면 findAllWithTime으로 메서드 명을 변경할 것 같습니다!

이번 미션에서 자세히 다룰 내용은 아니라 간단하게 코멘트 남겨보면
책임과 예측 가능성 측면서에서 한 번쯤 더 고민해보셔도 좋을 것 같네요.

ddaaac · 2024-04-23T00:28:09Z

src/main/java/roomescape/repository/JdbcReservationTimeRepository.java

+        try {
+            ReservationTime reservationTime = jdbcTemplate.queryForObject(sql, rowMapper, id);
+            return Optional.of(reservationTime);
+        } catch (EmptyResultDataAccessException e) {


처리 꼼꼼하게 잘 해주셨네요 👍

ddaaac · 2024-04-23T00:28:11Z

src/main/java/roomescape/repository/ReservationRepository.java

@@ -4,6 +4,7 @@
 import roomescape.domain.Reservation;

 public interface ReservationRepository {


Repository를 인터페이스로 분리해서 얻은 장점이 있나요?

확장성이 좋습니다.
실제로 1-3단계에서 4-9단계로 쉽게 변경할 수 있었습니다.

확장성이 좋다는게 구체적으로 어떤 의미일까요?
만약 ReservationRepository가 인터페이스가 아니고 클래스였을 때와 지금처럼 인터페이스일 때와는 어떤 차이가 있을까요?

ddaaac · 2024-04-23T00:28:15Z

src/main/java/roomescape/service/ReservationTimeService.java

+import roomescape.repository.ReservationTimeRepository;
+
+@Service
+@Transactional(readOnly = true)


@Transactional(readOnly = true)를 사용했을 때는 어떤 장점이 있을까요?

읽기 전용 트랙잭션임을 명시합니다.
데이터베이스에서 읽기 작업만 수행하고 데이터를 변경하지 않습니다.
이로 인해 성능 향상이 있습니다.

ddaaac · 2024-04-23T00:28:25Z

src/test/java/roomescape/controller/BaseControllerTest.java

 import roomescape.BaseTest;

+@Sql(scripts = "classpath:truncate.sql", executionPhase = ExecutionPhase.BEFORE_TEST_METHOD)


테스트에서 DB 초기화 로직을 호출하는 건 위험합니다. 잘 와닿지 않을 수도 있는 얘기지만 테스트 환경에도 데이터들이 존재하는데요, 이런 데이터들이 테스트 한 번 돌린다고 모두 사라지면 곤란하거든요. 테스트로 생성된 데이터만 날리게 구현해주시면 좋을 것 같습니다.

초기 데이터들이 많을 경우에는 값을 세팅하는데 시간이 많이 들 것 같습니다.
이런 경우에는 truncate 코드를 지우고 @AfterEach를 이용해서 @BeforeEach를 통해 생성했던 값들을 지울 것 같습니다.

하지만 이번 미션같은 경우에는 truncate로 모든 값을 밀어버리는게 제일 단순해서 사용했던 것 같습니다.

ddaaac · 2024-04-23T00:28:30Z

src/test/java/roomescape/repository/BaseRepositoryTest.java

+@Transactional
+public abstract class BaseRepositoryTest extends BaseTest {
+}


테스트의 계층이 복잡해지면 어떤 문제가 생길지도 고민해보시면 좋을 것 같아요

같은 계층끼리 테스트에서 비슷한 코드가 생길 것 같다고 짐작해서 만들어봤는데 당장은 필요가 없을 것 같네요

ddaaac · 2024-04-23T00:28:34Z

src/test/java/roomescape/service/ReservationServiceTest.java

+class ReservationServiceTest extends BaseServiceTest {
+
+    @Mock


mocking과 transactional은 같이 쓰일 수 있을까요? 런던파 vs 고전파에 대한 글을 읽고 구름이 만든 테스트는 어떤 테스트를 하고 싶은건지 고민해보시면 좋을 것 같습니다

repository는 @Transactional을 이용한 테스트 간의 격리는 고전파를 의도한 것 같습니다.
service는 Mock을 이용한 테스트 간의 격리는 런던파를 의도한 것 같습니다.

@Transactional을 잘못 붙인 것 같네요. 감사합니다!

ddaaac · 2024-04-23T00:28:38Z

src/test/resources/application.yml

+logging:
+  level:
+    org.springframework:
+      jdbc: DEBUG


테스트 코드에만 debug log를 사용하게 만들어주셨군요 😀

alstn113 · 2024-04-23T07:29:11Z

컨트롤러에서는 @WebMvcTest를 이용해서 테스트를 진행했습니다.
인수테스트에서 다이나믹 테스트를 할려고 했는데 딱히 시나리오가 떠오르지 않네요.

ddaaac

안녕하세요 구름 👋

리뷰 반영 잘 해주셨습니다.

몇 가지 코멘트 남겼으니 확인해주세요.

ddaaac · 2024-04-23T07:56:57Z

src/main/java/roomescape/dto/ReservationTimeResponse.java

+import roomescape.domain.ReservationTime;
+
+public record ReservationTimeResponse(
+        Long id,


프런트 코드를 자세히 보지는 않았는데 time의 id를 사용하는 곳이 있나요?

삭제 할 때 필요해요 :)

ddaaac · 2024-04-23T07:59:53Z

src/main/java/roomescape/repository/ReservationRepository.java

@@ -4,6 +4,7 @@
 import roomescape.domain.Reservation;

 public interface ReservationRepository {


확장성이 좋다는게 구체적으로 어떤 의미일까요?
만약 ReservationRepository가 인터페이스가 아니고 클래스였을 때와 지금처럼 인터페이스일 때와는 어떤 차이가 있을까요?

ddaaac · 2024-04-23T08:43:27Z

src/test/java/roomescape/repository/JdbcReservationTimeRepositoryTest.java

+import roomescape.fixture.Fixture;
+
+@SpringBootTest
+@Transactional


테스트에 사용하는 @Tx가 적당한가에는 여러 논의 들이 있습니다. 한 번쯤 고민해보셔도 좋을 것 같아요.

확장성이 좋다는게 구체적으로 어떤 의미일까요?
만약 ReservationRepository가 인터페이스가 아니고 클래스였을 때와 지금처럼 인터페이스일 때와는 어떤 차이가 있을까요?

예를 들어, 인터페이스를 만들고 InMemoryRepository, JdbcRepository를 만들고 bean으로 등록해서 상황에 따라 다르게 사용할 수 있습니다.
클래스와는 다르게 다형성을 활용해 유연하게 전환할 수 있습니다.

테스트에 사용하는 @tx가 적당한가에는 여러 논의 들이 있습니다. 한 번쯤 고민해보셔도 좋을 것 같아요.

저 글에서 결론은 일반적인 트랜잭션 사용 코드가 아닌 트랜잭션과 관련된 코드에서는 주의가 필요하다는 것이겠군요.
다양한 방법들에 대해서 알았습니다. 감사합니다!

ddaaac · 2024-04-23T08:45:40Z

src/test/java/roomescape/controller/ReservationControllerTest.java

+        BDDMockito.given(reservationService.getAllReservations())
+                .willReturn(List.of(ReservationResponse.from(reservation)));


일반 mockito 대신 BDDMockito를 사용해주신 이유가 있을까요?

given - when - then을 통해 테스트의 가독성을 높여 더 의도를 명확히 하고 싶었습니다!

ddaaac · 2024-04-23T08:53:18Z

src/test/java/roomescape/controller/ReservationTimeControllerTest.java

+import roomescape.fixture.Fixture;
+import roomescape.service.ReservationTimeService;
+
+@WebMvcTest(ReservationTimeController.class)


@SpringBootTest와 @WebMvcTest는 어떤 점이 다를까요?

@SpringBootTest는 스프링 부트 전체 컨텍스트를 로드합니다. 그래서 실제 애플리케이션을 시작하고, 실제 데이터베이스와 통합하여 테스트할 수 있습니다.

@WebMvcTest는 웹 계층과 컨트롤러와 관련된 빈들만 로드합니다.
컨트롤러의 요청과 응답을 테스트하고, 컨트롤러에서 사용하는 서비스나 다른 컴포넌트들을 목킹하여 테스트할 수 있습니다.

ddaaac · 2024-04-23T08:58:17Z

src/test/java/roomescape/acceptance/AdminViewAcceptanceTest.java

+    @Test
+    @DisplayName("어드민 페이지를 조회한다.")
+    void adminPage() {
+        ExtractableResponse<Response> response = RestAssured.given().log().all()


RestAssured와 mockMvc의 차이도 가볍게 정리하고 넘어가면 좋을 것 같습니다.

사용목적, 의존성, 속도, 목적 등의 차이를 다시 한 번 짚고 넘어갑니다!

ddaaac · 2024-04-23T09:21:48Z

src/test/java/roomescape/acceptance/BaseAcceptanceTest.java

+    @BeforeEach
+    void environmentSetUp() {
+        RestAssured.port = port;
+    }
+
+    @AfterEach
+    public void environmentAfterEach() {
+        List<String> truncateQueries = getTruncateQueries(jdbcTemplate);
+        truncateTables(jdbcTemplate, truncateQueries);
+    }


상속된 테스트에서 @BeforeEach @AfterEach는 어떻게 동작할까도 한 번쯤 체크해보시면 좋을 것 같습니다.

상속관계에서 @BeforeEach와 @AfterEach는 다음의 순서로 실행되네요.

부모 클래스 @BeforeEach
자식 클래스 @BeforeEach
자식 클래스 @AfterEach
부모 클래스 @AfterEach

ddaaac · 2024-04-23T09:22:39Z

src/test/java/roomescape/acceptance/ReservationAcceptanceTest.java

+    @Autowired
+    private ObjectMapper objectMapper;


스프링에서 주입시켜주는 ObjectMapper가 어디서 오는지도 가볍게 체크해보시면 좋을 것 같아요. 이 리뷰는 넘어가도 좋습니다.

ddaaac

안녕하세요 구름 👋

구현 잘 해주셨습니다.
이번 단계는 머지해도 될 것 같네요.

추가적으로 궁금한 부분이 있으면 DM 주세요!

ddaaac · 2024-04-24T08:55:22Z

src/main/java/roomescape/controller/ReservationController.java

+    @GetMapping("/reservations")
+    public ResponseEntity<List<ReservationResponse>> getAllReservations() {
+        List<ReservationResponse> reservationResponses = reservationService.getAllReservations();

        return ResponseEntity.ok(reservationResponses);


@RestController를 사용하고 있다면 꼭 통일성 부여를 위해 ResponseEntity를 사용할 필요는 없다고 생각해요. 200 상태코드를 사용하거나 헤더를 붙여줄 필요가 없다면 바로 객체를 반환하는게 보기 좋지 않을까도 고민해보시면 좋겠네요.

ddaaac · 2024-04-24T08:55:28Z

src/main/java/roomescape/repository/JdbcReservationRepository.java

+    public List<Reservation> findAll() {
+        String sql = """
+                    SELECT
+                        r.id as reservation_id,
+                        r.name,
+                        r.date,
+                        t.id as time_id,
+                        t.start_at as time_value
+                    FROM reservation as r
+                    inner join reservation_time as t
+                    on r.time_id = t.id
+                """;
+
+        return jdbcTemplate.query(sql, rowMapper);


이번 미션에서 자세히 다룰 내용은 아니라 간단하게 코멘트 남겨보면
책임과 예측 가능성 측면서에서 한 번쯤 더 고민해보셔도 좋을 것 같네요.

ddaaac · 2024-04-24T08:55:54Z

src/test/java/roomescape/repository/JdbcReservationRepositoryTest.java

+@SpringBootTest
+@Transactional
+class JdbcReservationRepositoryTest {


db 레이어를 테스트할 수 있는 다른 방법도 알아볼까요?

Jbdc로 해봐야겠네요 ㅎㅎ

alstn113 added 24 commits April 19, 2024 16:47

feat: 데이터베이스 설정 및 스키마 예약

9672876

refactor: 데이터베이스를 이용한 조회로 변경

8b8f82f

refactor: 데이터베이스를 이용한 추가/삭제로 변경

11f78f1

feat: ReservationTime 도메인 구현

22df57f

feat: 시간 관리 기능 구현

4b5e428

refactor: 메서드 명 수정 및 Transactional 추가

a3417d6

refactor: 시간 요청에 대한 성공 상태코드 변경

8d859b2

refactor: 예약 페이지 파일 경로 변경

323dd96

refactor: 정해진 시간으로만 예약으로 변경

f01622c

refactor: 예약 응답에서 예약 시간 응답 관련 수정

43bab55

refactor: 시간 검증 분리

b245438

refactor: 도메인 빈 생성장 제거

ddc2b44

refactor: request mapping 제거

ea99c8e

refactor: findById 로직 변경

cdd1750

refactor: repository 이름 수정

d013918

feat: Repository 테스트 작성

0c5b7f3

docs: api 명세서 작성

bdc4a8a

refactor: reservation time_id not null로 변경

2a66f1f

refactor: 테스트 코드 분리

7e3b3c1

fix: mockito 테스트 수정

fa764de

refactor: 코드 포맷팅

0f869d9

refactor: truncate 코드 이동

6475681

refactor: response 파싱 추가

5721f8b

chore: 빈 라인 제거

5e89018

ddaaac requested changes Apr 23, 2024

View reviewed changes

alstn113 added 5 commits April 23, 2024 10:47

refactor: 서비스 테스트에서 @transactional 제거

d8ade42

refactor: 추상 클래스 제거

2832bb2

refactor: Response에서 ExtractableResponse로 타입 변경

e9bfd77

refactor: cotroller test WebMvcTest로 변경

fab062e

refactor: 인수테스트 분리

b3a4652

ddaaac requested changes Apr 23, 2024

View reviewed changes

ddaaac mentioned this pull request Apr 23, 2024

[4 - 9단계 - 방탈출 예약 관리] 클로버(이은정) 미션 제출합니다. #125

Merged

ddaaac approved these changes Apr 24, 2024

View reviewed changes

ddaaac merged commit 4f445dd into woowacourse:alstn113 Apr 24, 2024

		@@ -4,6 +4,7 @@
		import roomescape.domain.Reservation;

		public interface ReservationRepository {

		import roomescape.BaseTest;

		@Sql(scripts = "classpath:truncate.sql", executionPhase = ExecutionPhase.BEFORE_TEST_METHOD)

		BDDMockito.given(reservationService.getAllReservations())
		.willReturn(List.of(ReservationResponse.from(reservation)));

[4 - 9단계 - 방탈출 예약 관리] 구름(김민수) 미션 제출합니다. #108

[4 - 9단계 - 방탈출 예약 관리] 구름(김민수) 미션 제출합니다. #108

Conversation

alstn113 commented Apr 22, 2024 • edited Loading

ddaaac 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

alstn113 Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alstn113 commented Apr 23, 2024

ddaaac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alstn113 Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alstn113 Apr 23, 2024 • edited Loading

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

alstn113 Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddaaac 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

alstn113 commented Apr 22, 2024 •

edited

Loading

alstn113 Apr 23, 2024 •

edited

Loading

alstn113 Apr 23, 2024 •

edited

Loading

alstn113 Apr 23, 2024 •

edited

Loading

alstn113 Apr 24, 2024 •

edited

Loading