[2단계 - 자동차 경주 리팩토링] 이프(송인봉) 미션 제출합니다. #425

sinb57 · 2022-02-20T04:58:50Z

안녕하세요! 이프입니다.

이미 2단계 미션이 머지되었지만
수정 사항이 있어서 다시 한번 PR 드립니다!!

지난 최종 제출 이후, 다시 한번 코드를 확인하는 과정에서 GameServiceInitTest 의
자동차 이름 길이 예외테스트와 자동차 이름 중복 예외테스트가 통과되지 않는 것을 확인했습니다.
해당 테스트코드가 성공하도록 코드를 수정했으며, 그 외 추가적인 변경 사항은
Custom Exception을 도메인별로 한단계 더 계층을 나눴다는 점이 있습니다.

sinb57 · 2022-02-20T05:02:20Z

src/test/java/racingcar/service/GameServiceInitTest.java

    @ParameterizedTest
    @MethodSource(PROVIDER_PATH + "provideForInitCarNamesDuplicatedExceptionTest")
    void initCarNamesDuplicatedExceptionTest(final List<String> carNames) {
-        final String errorMessage = CarNameExceptionStatus.NAME_IS_TOO_LONG.getMessage();


이름 길이 예외 테스트 과정에서 예외 메시지가 잘못 지정되어 있었습니다!ㅜ

sinb57 · 2022-02-20T05:03:06Z

src/test/java/racingcar/service/provider/GameServiceInitTestProvider.java

    public static Stream<Arguments> provideForInitCarNamesTooLongExceptionTest() {
        return Stream.of(
                Arguments.of(Arrays.asList("abcdef", "poby", "hanul", "hello")),
-                Arguments.of(Arrays.asList("hanul", "", "poby", "ififif"))


이름 길이 초과를 확인하기 위한 테스트 데이터인데, 잘못된 데이터 ("")가 존재했습니다.

ksy90101

이프 안녕하세요 :) 리뷰어 럿고입니다.
다시 보다 보니, 조금 더 해보면 좋을만한게 있어서 리뷰를 더 남겨봣어요 :)
확인해보시고 수정한번 해주시면 감사하겠습니다!

ksy90101 · 2022-02-20T07:02:14Z

src/test/java/racingcar/view/input/RoundInputTest.java

+                .isInstanceOf(WrongRoundCountException.class)
                .hasMessageContaining(RoundCountExceptionStatus.ROUND_IS_NOT_NUMERIC.getMessage());
    }



코드가 보이지 않아 아래에 남길게요.
pickNumberInRangeTest 이 테스트를 보면 20번을 돌리게 되는데, 혹시 20번을 검증하는 이유가 있을까요?

RandomUtilTest의 pickNumberInRangeTest 테스트를 통해 확인하고자 한 것은
RandomUtil 클래스의 pickNumberInRange 메서드의 정상 작동 여부입니다.
(지정된 범위 내의 숫자를 랜덤하게 선정하는 기능)

랜덤 값의 특성상 항상 동일한 값이 나오지 않으므로, 랜덤 값의 범위가 크면
그 만큼 실행횟수를 늘려야한다고 생각했고
무한정 실행횟수를 늘리기보다는 랜덤 범위를 축소해서 횟수 또한 줄이고자 했습니다.
그래서 해당 테스트에서 랜덤 값이 [0,1,2] 중에서 나오도록 지정했는데
횟수는 20번으로 지정한 것에 대해서는 별다른 이유가 없습니다.

100번이 될 수도 있고, 1000번이 될 수도 있는데
그 횟수만큼 테스트 실행에 성공했다고 해서
해당 테스트를 완벽히 신뢰할 수는 없다고 생각했습니다. 랜덤값 테스트니까요.

그래서 저는 RandomUtil의 pickNumberInRange 메서드에 대해서
범위 지정이 옳게 작동하는지만 확인하기 위해
실행 횟수를 늘리기보단 범위를 축소하는 방향으로 진행했습니다!!

네 이프의 의견도 좋지만, 저희가 실제로는 Random값을 내려주는것은 라이브러리를 사용하고 있습니다.
저는 라이브러리를 사용함에 따라, 라이브러리를 믿고 사용해야 한다고 생각해요.
말씀하신대로, 1000번 돌려도 문제가 될수 있지만, 그렇다면 라이브러리를 너무 못믿는게 아닐까 합니다 :)
지금은 테스트코드가 많지 않아서 괜찮지만, 차후 테스트코드가 많아진다면 어떤 문제가 있을지 고민해보세요.

참고로 저희 회사에 테스트 코드가 백엔드만 9천개가 있는데, 테스트 코드를 모두 돌릴떄 10분정도 걸려요 :) ㅎㅎ

앗!! 라이브러리를 안 믿는게 아니고.. RandomUtil 클래스에 정의된 pickNumberInRange가 문제였습니다!!
아래의 코드를 보시면, 랜덤값의 시작부분도 컨트롤하고 싶은 생각으로 작성했거든요.

만약 해당 메서드가 수정되는 상황에서, 개발자가 exclusive와 inclusive를 잘못 보고서
final int size = exclusiveEnd - inclusiveStart + 1; 이렇게 하거나
return random.nextInt(size) + inclusiveStart - 1; 이렇게 할 경우
의도와는 달리 범위 지정이 잘못되는 문제가 발생합니다.

final int size = inclusiveStart - exclusiveEnd; 이렇게 할 수도 있고요.
(저번 pr에서 이렇게 했다가 수정한 이력이 있거든요.)

private int pickNumber(final int inclusiveStart, final int exclusiveEnd) { final int size = exclusiveEnd - inclusiveStart; return random.nextInt(size) + inclusiveStart; }

그래서 제가 테스트하고자 한 것은, 범위 지정이 옳게 되었는가였습니다!!

오 넵 :) 해당 부분은 저도 인지하지 못했네요.
저도 배워 갑니다 :)
말씀하신대료, 이렇게 수정한다면 문제가 발생할 수 있을거 같네요. 그렇다고 해서 테스트를 계속 돌리는건 조금 무리가 있을듯 하네요.
https://stackoverflow.com/questions/14811014/writing-a-junit-test-for-a-random-number-generator
위의 링크를 보면 비슷한 방법이지만, 조금 다르게 동작할수도 있는데요. 위와 같은 방법도 고민해보면 좋을거 같네요!@

역시 프로그래밍은 어렵네요. 하지만 그래서 더 재밌는거 같습니다ㅎㅎ!!
비록 완벽한 정답은 아닐지라도, 최선의 답을 찾기 위해 노력하겠습니다!!
좋은 자료 알려주셔서 감사합니다!

ksy90101 · 2022-02-20T07:04:14Z

src/test/java/racingcar/view/input/RoundInputTest.java

+                .isInstanceOf(WrongRoundCountException.class)
                .hasMessageContaining(RoundCountExceptionStatus.ROUND_IS_NOT_NUMERIC.getMessage());
    }



현재 테스트를 돌리면 Paramterzied 테스트를 돌리고 확인하면 어떤 테스트 인지 알수가 조금 없는데요.
https://stackoverflow.com/questions/57892989/generating-display-names-for-parameterizedtest-in-junit-5
위의 링크를 참고해서 한번 보기 좋게 해볼까요?

와 세상에!! 이런 좋은 방법이 있었네요!! 감사합니다ㅜ

ksy90101 · 2022-02-20T07:06:53Z

src/test/java/racingcar/service/GameServiceInitTest.java

    @ParameterizedTest
    @MethodSource(PROVIDER_PATH + "provideForInitCarNamesDuplicatedExceptionTest")
    void initCarNamesDuplicatedExceptionTest(final List<String> carNames) {
-        final String errorMessage = CarNameExceptionStatus.NAME_IS_TOO_LONG.getMessage();


ksy90101 · 2022-02-20T07:07:01Z

src/test/java/racingcar/service/provider/GameServiceInitTestProvider.java

    public static Stream<Arguments> provideForInitCarNamesTooLongExceptionTest() {
        return Stream.of(
                Arguments.of(Arrays.asList("abcdef", "poby", "hanul", "hello")),
-                Arguments.of(Arrays.asList("hanul", "", "poby", "ififif"))


ksy90101 · 2022-02-20T07:07:53Z

src/main/java/racingcar/exception/round/WrongRoundCountException.java

+import racingcar.exception.ExceptionStatus;
+import racingcar.exception.WrongArgumentException;
+
+public class WrongRoundCountException extends WrongArgumentException {


커스텀 예외가 무조건 좋은건 아닙니다 :)
아이템72. 표준예외를 사용하라.
위의 글을 읽어보시고, 한번 이프만의 커스텀 예외 생성 이유를 찾아보면 좋을거 같아요 :)

커스텀 예외에 대해서 생각이 많아지는데.. 이부분에 연연하는 것보다
저번 리뷰에 적어주신대로 현재 미션에서 학습해야할 부분에 초점을 맞추도록 하겠습니다ㅜ
이후에 이 부분을 다룰 기회가 있으리라 믿습니다!!

넵 좋습니다 :) 너무 연연하지 않아도 됩니다 :)
제가 말씀드린것처럼, 이프만의 커스텀 예외 생성 이유를 찾아보라는것입니다.
사실 저는 커스텀 예외에 대해서는 정답이 따로 있지 않다고 생각해요.
그래서 충분히 미션을 하면서 경험을 하다 보면 이프만의 생각이 생길거라고 생각합니다 :)

아.. 지금껏 커스텀 예외를 어떻게 써야하지?에 집중해서 생각을 하고 있었네요ㅜ
말씀하신대로 커스텀 예외를 꼭 써야 하는가?에 대해서는 저 스스로의 확고한 생각을 갖도록 하겠습니다!!
긴 시간동안 코드리뷰를 해주시고 수많은 피드백을 남겨주셔서 정말 감사합니다!!

이프가 말씀하신 내용에서 너무 공감되네요.
저도 예전에는 이걸 어떻게 쓰지? 라는 어떻게에 많은 초점을 맞추었었는데요.
우테코를 하면서 포비가 왜라는거에 집중해보면 좋겠다 라는 말에 왜에 집중 하다보니, 내가 지금 당장 필요한게 무엇인지 내가 제대로된 길을 가고 있는지, 이걸 여기에 써도 되는 확신을 갖게 되는거 같아요.
👍

ksy90101

이프 안녕하세요 :) 리뷰어 럿고 입니다.
마지막까지 너무 고생 많으셨습니다~
다음 미션도 파이팅 입니다!

언제든지 질문이 있으시면 슬랙으로 연락주세요~

sinb57 added 3 commits February 20, 2022 13:51

test: GameServiceInitTest, 이름 길이 예외 테스트 실패 해결

e4f8578

test: GameServiceInitTest, 이름 중복 예외 테스트 실패 해결

eb608ba

refactor: Custom Exception, 각 도메인별 상위 클래스 생성

cfed5c3

sinb57 commented Feb 20, 2022

View reviewed changes

ksy90101 requested changes Feb 20, 2022

View reviewed changes

test: ParameterizedTest 테스트 메서드 name 세부 명시

a15f9c4

ksy90101 approved these changes Feb 21, 2022

View reviewed changes

ksy90101 merged commit 97b627f into woowacourse:sinb57 Feb 21, 2022

[2단계 - 자동차 경주 리팩토링] 이프(송인봉) 미션 제출합니다. #425

[2단계 - 자동차 경주 리팩토링] 이프(송인봉) 미션 제출합니다. #425

Uh oh!

Conversation

sinb57 commented Feb 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ksy90101 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sinb57 Feb 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ksy90101 Feb 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ksy90101 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sinb57 Feb 20, 2022 •

edited

Loading

ksy90101 Feb 20, 2022 •

edited

Loading