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 #837

Merged
merged 3 commits into from Nov 8, 2022
Merged

[김대겸] Step1 PR #837

merged 3 commits into from Nov 8, 2022

Conversation

Gyeom
Copy link

@Gyeom Gyeom commented Nov 7, 2022

안녕하세요 리뷰어님,

1단계 지하철역의 인수 테스트 작성 PR입니다.

잘 부탁드립니다 :)

Copy link

@devyonghee devyonghee left a comment

Choose a reason for hiding this comment

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

안녕하세요 김대겸님!! 👋👋
세번째 인수 테스트 주도 개발 미션 함께 하게 된 한용희라고 합니다! 🙇‍♂️🙇‍♂️🙇‍♂️

1단계 RestAssured 를 활용하여 인수테스트 잘 구현해주신 것 같아요👍👍👍
다음 단계를 진행하시면서 테스트 코드의 가독성을 위해
인수테스트의 동작들을 분리해보는 방법은 어떨까 고민해보시면 좋을 것 같아요 🤔🤔
추가적으로 소소한 코멘트들도 남겨드렸으니 참고해주세요 😆

이번 단계는 잘 진행해주셔서 바로 머지하도록 하겠습니다 👍
그럼 다음 단계도 파이팅입니다!! 🚀💪

Comment on lines +117 to +118
givenCreateStation(Map.of("name", station1));
givenCreateStation(Map.of("name", station2));

Choose a reason for hiding this comment

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

gradle 의 자바 버전과 프로젝트의 자바 버전 설정 확인 한번만 부탁드리겠습니다. 🙏🙇‍♂️
gradle 에서는 자바 8로 설정되어 있을텐데
사용해주신 Map.of 는 자바 9 이상에서 사용할 수 있는 메소드 같아서요!

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 +43 to +44
//@formatter:off

Choose a reason for hiding this comment

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

매 테스트마다 이 설정이 들어간 것 같은데요 🤔
만약 정렬할 때 메서드 체이닝하는 부분이 인라인으로 자동 정렬되는 것이라면
Preferences -> Code style -> Formatter -> Reformat again to remove custom line breaks
체크가 제거되어 있는지 확인 한번만 부탁드릴게요 🙏

코드에 주석으로 설정을 추가해주는 것보다
설정된 코드 스타일을 export 하여 설정 파일을 공유하는 방향은 어떻게 생각하시나요?? 🤔🤔
Screenshot 2022-11-08 at 4 52 47 PM

Copy link
Author

@Gyeom Gyeom Nov 8, 2022

Choose a reason for hiding this comment

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

체크가 제거되어 있는 상태라 메서드 체이닝 부분은 인라인으로 자동 정렬이되지는 않습니다.

image

다만 개인적으로 위에 첨부한 코드처럼 가독성이 너무 떨어진다고 판단하였고, 번거로움을 해소하기 위해서 Live Templates을 활용하고 있습니다.

image

Choose a reason for hiding this comment

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

넵 감사합니다!
대겸님 의도에 대해서 이해했습니다!

Comment on lines +144 to +147
// given
String station1 = "강남역";
givenCreateStation(Map.of("name", station1));

Choose a reason for hiding this comment

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

같은 데이터를 계속 추가해주고 있는데
다른 테스트에 영향이 없는지 확인 한번만 부탁드리겠습니다.🙇‍♂️🙏
제 개인 환경에서는 전체 테스트가 중간에 깨지게 되네요 😅

Copy link
Author

Choose a reason for hiding this comment

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

넵 알겠습니다~! ㅎㅎ

@devyonghee devyonghee merged commit 974faae into next-step:gyeom Nov 8, 2022
@Gyeom
Copy link
Author

Gyeom commented Nov 8, 2022

1단계 RestAssured 를 활용하여 인수테스트 잘 구현해주신 것 같아요👍👍👍 다음 단계를 진행하시면서 테스트 코드의 가독성을 위해 인수테스트의 동작들을 분리해보는 방법은 어떨까 고민해보시면 좋을 것 같아요 🤔🤔

안녕하세요 리뷰어님~ 먼저 피드백 감사드립니다 ㅎㅎ

혹시 말씀하시는게 아래와 같은 방식으로 분리해보는 것을 의미하실까요~~?
image

@devyonghee
Copy link

devyonghee commented Nov 8, 2022

테스트를 진행하시는데 RestAssured 의 구문이 길어지면 재사용도 어렵고 테스트 의도를 파악하기 어려워질 것 같아 드린 의견이었습니다.
참고해보시면 좋을 것 같은 코드 첨부드리겠습니다.
저도 현재 이러한 구조를 많이 참고하여 인수테스트를 작성하고 있습니다 😆

https://github.com/boorownie/atdd-subway-path/blob/master/src/test/java/nextstep/subway/station/acceptance/StationAcceptanceTest.java

@Gyeom
Copy link
Author

Gyeom commented Nov 8, 2022

테스트를 진행하시는데 RestAssured 의 구문이 길어지면 재사용도 어렵고 테스트 의도를 파악하기 어려워질 것 같아 드린 의견이었습니다. 참고해보시면 좋을 것 같은 코드 첨부드리겠습니다. 저도 현재 이러한 구조를 많이 참고하여 인수테스트를 작성하고 있습니다 😆

https://github.com/boorownie/atdd-subway-path/blob/master/src/test/java/nextstep/subway/station/acceptance/StationAcceptanceTest.java

평소 Spring Rest Docs만 사용해봐서 잘 몰랐는데ㅠㅠ 감사드립니다~! 👍 👍 👍 👍

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