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

[김대겸] Step1 PR #651

Merged
merged 13 commits into from Dec 10, 2022
Merged

[김대겸] Step1 PR #651

merged 13 commits into from Dec 10, 2022

Conversation

Gyeom
Copy link

@Gyeom Gyeom commented Dec 10, 2022

안녕하세요 리뷰어님,

1단계 - 테스트를 통한 코드 보호 리뷰 요청 드립니다.

잘 부탁드립니다 :)

Copy link
Member

@limwoobin limwoobin 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 +9 to +20
@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT)
@Sql("/sql/truncate.sql")
public class AcceptanceTest {

@LocalServerPort
int port;

@BeforeEach
public void setUp() {
RestAssured.port = port;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

인수테스트 정말 꼼꼼하게 잘 작성해주셧네요 ㅎㅎ
호출, 검증기능에 대해서도 잘 분리되어있으니 보기 편하네요!

Comment on lines +21 to +23
@DisplayName("메뉴 그룹 서비스 테스트")
@ExtendWith(MockitoExtension.class)
public class MenuGroupServiceTest {
Copy link
Member

Choose a reason for hiding this comment

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

서비스에 대한 단위 테스트도 정말 잘 작성해주셨습니다 ㅎㅎ
단위테스트에서 public 접근제어자는 없어도 될것같아요!

Copy link
Author

Choose a reason for hiding this comment

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

접근제어자는 없어도 될것 같네요 ㅎㅎ 수정하도록 하겠습니다~!

Comment on lines +105 to +154
@DisplayName("가격이 0원 미만인 메뉴를 생성하면 IllegalArgumentException을 반환한다.")
@Test
void createWithException() {
// given
Menu menu = createMenu(1L, "짜장면_탕수육_1인_메뉴_세트", BigDecimal.valueOf(-1000L),
짜장면_탕수육_1인_메뉴_세트.getId(), Arrays.asList(짜장면상품, 탕수육상품, 단무지상품));

// when & then
assertThatIllegalArgumentException().isThrownBy(() -> menuService.create(menu));
}

@DisplayName("존재하지 않는 메뉴그룹이 포함된 메뉴를 생성하면 IllegalArgumentException을 반환한다.")
@Test
void createWithException2() {
// given
Menu menu = createMenu(1L, "짜장면_탕수육_1인_메뉴_세트", BigDecimal.valueOf(20000L), 10L,
Arrays.asList(짜장면상품, 탕수육상품, 단무지상품));
when(menuGroupDao.existsById(10L)).thenReturn(false);

// when & then
assertThatIllegalArgumentException().isThrownBy(() -> menuService.create(menu));
}

@DisplayName("존재하지 않는 상품이 포함된 메뉴를 생성하면 IllegalArgumentException을 반환한다.")
@Test
void createWithException3() {
// given
Menu menu = createMenu(1L, "짜장면_탕수육_1인_메뉴_세트", BigDecimal.valueOf(20000L), 중국집_1인_메뉴_세트.getId(),
singletonList(짜장면상품));
when(menuGroupDao.existsById(짜장면_탕수육_1인_메뉴_세트.getMenuGroupId())).thenReturn(true);
when(productDao.findById(짜장면상품.getProductId())).thenReturn(Optional.empty());

// when & then
assertThatIllegalArgumentException().isThrownBy(() -> menuService.create(menu));
}

@DisplayName("메뉴의 가격이 메뉴 상품들의 가격의 합보다 큰 메뉴를 생성하면 IllegalArgumentException을 반환한다.")
@Test
void createWithException4() {
// given
Menu menu = createMenu(1L, "짜장면_탕수육_1인_메뉴_세트", BigDecimal.valueOf(21000L), 중국집_1인_메뉴_세트.getId(),
Arrays.asList(짜장면상품, 탕수육상품, 단무지상품));
when(menuGroupDao.existsById(짜장면_탕수육_1인_메뉴_세트.getMenuGroupId())).thenReturn(true);
when(productDao.findById(짜장면상품.getProductId())).thenReturn(Optional.of(짜장면));
when(productDao.findById(탕수육상품.getProductId())).thenReturn(Optional.of(탕수육));
when(productDao.findById(단무지상품.getProductId())).thenReturn(Optional.of(단무지));

// when & then
assertThatIllegalArgumentException().isThrownBy(() -> menuService.create(menu));
}
Copy link
Member

Choose a reason for hiding this comment

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

예외케이스에 대해서도 정말 잘 작성해주셧네요 ㅎㅎ

Comment on lines +75 to +91
중국집_1인_메뉴_세트 = createMenuGroup(1L, "중국집_1인_메뉴_세트");
짜장면 = createProduct(1L, "짜장면", BigDecimal.valueOf(8000L));
탕수육 = createProduct(2L, "탕수육", BigDecimal.valueOf(12000L));
짬뽕 = createProduct(3L, "짬뽕", BigDecimal.valueOf(9000L));
단무지 = createProduct(4L, "단무지", BigDecimal.valueOf(0L));
짜장면상품 = createMenuProduct(1L, null, 짜장면.getId(), 1L);
탕수육상품 = createMenuProduct(2L, null, 탕수육.getId(), 1L);
짬뽕상품 = createMenuProduct(3L, null, 짬뽕.getId(), 1L);
단무지상품 = createMenuProduct(4L, null, 단무지.getId(), 1L);
짜장면_탕수육_1인_메뉴_세트 = createMenu(1L, "짜장면_탕수육_1인_메뉴_세트", BigDecimal.valueOf(20000L), 중국집_1인_메뉴_세트.getId(), Arrays.asList(짜장면상품, 탕수육상품, 단무지상품));
짬뽕_탕수육_1인_메뉴_세트 = createMenu(2L, "짬뽕_탕수육_1인_메뉴_세트", BigDecimal.valueOf(21000L), 중국집_1인_메뉴_세트.getId(), Arrays.asList(짬뽕상품, 탕수육상품, 단무지상품));
주문테이블1 = createOrderTable(1L, null, 10, false);
주문테이블2 = createOrderTable(2L, null, 20, false);
짜장면_탕수육_1인_메뉴_세트주문 = createOrderLineItem(1L, null, 짜장면_탕수육_1인_메뉴_세트.getId(), 1);
짬뽕_탕수육_1인_메뉴_세트주문 = createOrderLineItem(2L, null, 짬뽕_탕수육_1인_메뉴_세트.getId(), 1);
주문1 = createOrder(주문테이블1.getId(), null, null, Arrays.asList(짜장면_탕수육_1인_메뉴_세트주문, 짬뽕_탕수육_1인_메뉴_세트주문));
주문2 = createOrder(주문테이블2.getId(), null, null, singletonList(짜장면_탕수육_1인_메뉴_세트주문));
Copy link
Member

Choose a reason for hiding this comment

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

setUp에 대해 반복적으로 fixture로 객체를 생성하는 부분이 보여지는데요 ㅎㅎ
재사용이 빈번한 객체에 대해서는 객체 자체를 fixture로 올려놓는것도 고려해보면 좋을것같아요 :)

Copy link
Author

Choose a reason for hiding this comment

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

재사용이 빈번한 객체는 fixture로 올려놓으면 더 관리하기가 용이하겠네요 👍

Comment on lines +24 to +47
public static void 주문_목록_응답됨(ExtractableResponse<Response> response) {
assertThat(response.statusCode()).isEqualTo(HttpStatus.OK.value());
}

public static void 주문_목록_포함됨(ExtractableResponse<Response> response, List<ExtractableResponse<Response>> createdResponses) {
List<Long> expectedOrderIds = createdResponses.stream()
.map(it -> Long.parseLong(it.header("Location").split("/")[3]))
.collect(Collectors.toList());

List<Long> resultOrderIds = response.jsonPath().getList(".", Order.class).stream()
.map(Order::getId)
.collect(Collectors.toList());

assertThat(resultOrderIds).containsAll(expectedOrderIds);
}

public static void 주문_상태_변경됨(ExtractableResponse<Response> response, String expectOrderStatus) {
String actualOrderStatus = response.jsonPath().getString("orderStatus");

assertAll(
() -> assertThat(response.statusCode()).isEqualTo(HttpStatus.OK.value()),
() -> assertThat(actualOrderStatus).isEqualTo(expectOrderStatus)
);
}
Copy link
Member

Choose a reason for hiding this comment

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

해당 메소드는 fixture 클래스 내에 있는 이유가 혹시 있을까요??
OrderAcceptanceStep 클래스에 있어야할것같아서 여쭤봅니다!

Copy link
Author

Choose a reason for hiding this comment

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

OrderAcceptanceTest.java 클래스에서 먼저 작성하고 리팩토링 하는 과정에서 잘못 옮긴것같네요 😢
이 부분도 수정하도록 하겠습니다 :)

@limwoobin limwoobin merged commit 317ee5e into next-step:gyeom Dec 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants