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

[3, 4 단계 - 체스] 져니(이지원) 미션 제출합니다. #604

Merged
merged 76 commits into from Mar 31, 2023

Conversation

Cl8D
Copy link

@Cl8D Cl8D commented Mar 27, 2023

안녕하세요, 서브웨이! 져니입니다 ☃️
3, 4단계는 시간에 쫓겨 급하게 구현하느라 내부 로직이 다소 엉망인 것 같네요... 🥲
궁금한 점은 코드 커멘트로 달아두겠습니다!
감사합니다 🙇‍♀️

Cl8D added 30 commits March 24, 2023 00:33
@@ -9,6 +9,8 @@ repositories {
}

dependencies {
runtimeOnly 'mysql:mysql-connector-java:8.0.28'
implementation 'org.yaml:snakeyaml:2.0'
Copy link
Author

Choose a reason for hiding this comment

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

프로퍼티의 경우 yml로 관리하기 위해서 별도의 라이브러리를 설치하여 진행하였는데, 혹시 구현 요구사항에 위반되는 내용이라면 변경하겠습니다! 🙇‍♀️

Choose a reason for hiding this comment

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

아닙니다. 저는 사용해도 무방하다고 생각합니다!

Choose a reason for hiding this comment

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

runtimeOnly, implementation 차이를 알고 사용하신걸까욥?

Copy link
Author

Choose a reason for hiding this comment

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

runtimeOnly는 runtime 때 해당 dependency를 불러오는 것이고, implementation은 컴파일 시점부터 rumtime 때까지 해당 dependency가 유지되는 것이라고 생각했습니다! DB 관련 depdency의 경우 컴파일 시점에서는 해당 의존성이 필요없고 실제로 동작했을 때 DB 관련 커넥션들을 맺으면서 필요하기 때문에 주로 runtimeOnly로 선언하는 것으로 알고 있습니다 🫠...

Choose a reason for hiding this comment

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

혹여나, 그냥 복붙해서 넣으신건가 해서 한번 여쭤봤어요ㅎㅎ.. 무엇이든 파악하고 쓰셨으면 해서요ㅎㅎ👍

src/main/java/chess/service/Service.java Outdated Show resolved Hide resolved
src/main/java/chess/service/ServiceManager.java Outdated Show resolved Hide resolved
src/main/java/chess/controller/ChessHandler.java Outdated Show resolved Hide resolved
src/main/java/chess/domain/piece/Score.java Show resolved Hide resolved
src/main/java/chess/entity/PieceEntity.java Outdated Show resolved Hide resolved
Comment on lines +12 to +15
public class MockChessGameDao implements ChessGameDao {
private final Map<Long, ChessGameEntity> STORAGE = new ConcurrentHashMap<>();
private final AtomicLong pk = new AtomicLong(0L);

Copy link
Author

Choose a reason for hiding this comment

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

DB 연결에 대한 테스트 코드를 작성하는 것은 낭비라고 생각이 되어서,
해당 메서드가 어떤 행위를 할 것인지에 대한 테스트 코드를 작성하기 위해 Map을 통한 임시 스토리지를 생성해두었습니다.
각 메서드가 '~~한 행위를 하고, ~~한 결과를 반환해야 한다'라는 의미로 테스트를 진행하였는데, DB 연결을 위한 테스트 코드도 작성을 하는 것이 맞을까요?

Copy link

@joseph415 joseph415 Mar 28, 2023

Choose a reason for hiding this comment

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

db와 연결하여 통합테스트를 작성하려는 의도가 아니고, 유닛테스트를 작성하시려고 하는 의도라면 이렇게 작성하셔도 무방하다고 생각합니다!

Choose a reason for hiding this comment

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

혹시 단위테스트와 통합테스트의 개념이 헷갈리시면 한번 찾아보시는것도 좋을 것 같습니다.

Copy link
Author

@Cl8D Cl8D Mar 30, 2023

Choose a reason for hiding this comment

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

말씀해주신 것을 보고 다음 레퍼런스를 보고 나름의 정리를 해보았는데요!

✔️ 단위 테스트 - 가장 작은 실행 단위, 클래스 혹은 메서드 단위가 예상대로 동작하는지 확인 (객체지향의 관점에서 보았을 때 하나의 객체를 단위라고도 볼 수 있다.)

✔️ 통합 테스트 - 여러 모듈을 모아서 이들이 제대로 협력하여 동작하는지 확인

위의 레퍼런스에서는 명시적으로 'DB에 접근하거나, 다양한 환경이 제대로 동작하는지' 확인할 때 통합 테스트를 이용한다고 했어요. 저는 단순히 DB를 접근하는 메서드 하나하나도 기능 단위로 봐서 단위 테스트라고 생각했었는데, 애플리케이션 단위가 아니라 DB를 하나의 외부 모듈로 보아서 해당 모듈과 협력하여 나의 프로그램이 잘 동작하는지 확인하는 게 통합 테스트라는 것이라고 이해했어요 🫠

지금까지는 기능들이 모인 게 통합 테스트라고 생각했었는데, 기능들이 모인 것 외에도 아예 모듈 간의 화합을 보는 게 통합 테스트라는 것을 이해했습니다... 🤔 특히 이 글에서도 말한대로 데이터베이스 테스트는 모두 통합 테스트라고 말을 해주시네요. 외부 리소스가 들어간 이상 통합 테스트라는 것을 이해했어요!


💬 그러면 한 가지 궁금한 게 있는데, 만약 외부 api (ex. 소셜 로그인) 을 활용한 테스트 작성은 외부 리소스를 사용하는 것이라고 보면 될까요? 그렇다면 해당 기능을 통합 테스트를 한다고 보는 것이 맞을까요?
-> 근데 보통 외부 api 통신의 경우 테스트 코드에서 실제로 api 요청을 보낸다기보다는, mocking을 통해서 해당 메서드가 호출되었다고 가정하고 제가 작성한 비즈니스 로직을 테스트한다고 생각하는데... (ex. kotlin에서의 every {} just runs 같은 구문 사용 시} 그렇다면 이런 관점에서는 단위 테스트라고 보면 될까요?


💬 추가적으로, 통합 테스트는 실제 리소스를 사용하거나 외부 api를 통신하는 것이라면, 웹 서비스에서 repository 테스트를 진행하였을 때 h2 같은 in-memory database를 활용하여 디비 관련 테스트를 진행했다면 해당 테스트는 통합 테스트라고 봐야 할까요?!


💬 실제 실무에서는 통합 테스트를 많이 작성하나요? 애플리케이션이 커진다면 통합 테스트를 하기에는 비용이 엄청 클 것 같다는 생각이 들었어요!

Copy link

@joseph415 joseph415 Mar 31, 2023

Choose a reason for hiding this comment

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

  1. 저는 통합테스트, 단위테스트를 어떤 관점에서 보는지 먼저 말씀드릴게요.

    • 가장작은 단위로 다른 협력객체 없이 테스트를 한다면, 단위테스트
    • 테스트를 진행할 시 협력객체를 통해 테스트를 진행하면 통합테스트

    라고 생각합니다. 테스트하려는 대상이외에는 어떻게 생각하면 모두 외부의 리소스라고 생각할 수 있으니까요ㅎㅎ

    Integration testing -- also known as integration and testing (I&T) -- is a type of software testing in which the different units, modules or components of a software application are tested as a combined entity. However, these modules may be coded by different programmers.

    일단 저는 위의 관점으로 답변을 드릴게요.

    외부 리소스를 사용해야하는데, mocking을 한 상태이다. 따라서 결국에는 mockd으로 인해 테스트 대상이 하나가 되면, 단위테스트라고 할 수 있을 것 같습니다. mocking을 통해서 테스트 하려는 대상만을 테스트하게 되는 것이니까요.

  2. 네 통합테스트로 봐야할 것 같습니다. 대상 코드들이 제대로 동작하는지 해당객체뿐아니라 모든 디비관련 협력객체들까지 테스트하는것이니까요. 하지만, 디비연결없이 정말 insert 로 작성한 레포지토리 코드들이 잘 동작하는지만 테스트하게 되면 단위테스트가 되겠쬬?

그리고 외부 api를 사용해서 테스트를 진행하는것은 잘못된것 같습니다. 외부 api는 내가 통제할 수 없는 곳이기 때문에, mocking을 통해서 성공,실패를 정해두고 테스트하는게 맞는것 같아요~

  1. 팀마다 다를텐데요. 테스트를 강제하는 팀이라면 모든 테스트를 다 작성할 것 같습니다. 테스트는 다다익선으로 많이 작성하면 좋겠지만, 과제 일정을 맞추기 위한 시간압박이 많기 때문에, 개발자 재량으로 두는 곳도 있을 겁니다.

Copy link

@joseph415 joseph415 left a comment

Choose a reason for hiding this comment

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

안녕하세요 져니

3,4 단계 잘 구현해주셨네요💯 코멘트로 질문해주신 부분은 답글로 달아두었습니다.
답글, 리뷰 코멘트 확인해주세요~!

src/test/java/chess/service/UserServiceTest.java Outdated Show resolved Hide resolved
src/test/java/chess/service/ChessGameServiceTest.java Outdated Show resolved Hide resolved
src/test/java/chess/service/ChessBoardServiceTest.java Outdated Show resolved Hide resolved
src/test/java/chess/domain/piece/PieceTest.java Outdated Show resolved Hide resolved
src/test/java/chess/dao/user/MockUserDao.java Show resolved Hide resolved
KNIGHT(Score.create(2.5)),
PAWN(Score.create(1)),
BISHOP(Score.create(3)),
KING(Score.create(0));

Choose a reason for hiding this comment

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

킹 관련 점수를 0 점이 아닌 무한대로 잡으면 안될까요? 의미상 0점보다 INT_MAX 같은 값이 더 와닿는것 같아서 의견드려요

Copy link
Author

Choose a reason for hiding this comment

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

점수를 계산할 때 존재하는 모든 기물의 정보를 가져와서 Score의 값들을 그대로 처리하기 때문에, MAX로 들어가면 해당 값에 대해서는 제거해주는 부분이 필요해서 0점으로 해두는 것이 조금 더 깔끔할 것 같습니다 🥲

Choose a reason for hiding this comment

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

이 부분은 제가 파악하지 못해서 모든 리뷰이에게 같은 리뷰를 드렸었네요ㅠㅠ

King이 잡혔을 때 게임을 종료해야 한다. 로 어차피 점수 계산이 의미가 없을 것 같아서 의견 드렸는데,

요구사항에 5(rook) + 3(bishop) + 9(queen) + 3(pawn) + 0(king) = 20점 다음과 같은 예시가 나와있네요ㅎㅎ;

src/main/java/chess/domain/chess/vo/ScoreVO.java Outdated Show resolved Hide resolved
src/main/java/chess/domain/chess/ChessGame.java Outdated Show resolved Hide resolved
@@ -9,6 +9,8 @@ repositories {
}

dependencies {
runtimeOnly 'mysql:mysql-connector-java:8.0.28'
implementation 'org.yaml:snakeyaml:2.0'

Choose a reason for hiding this comment

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

runtimeOnly, implementation 차이를 알고 사용하신걸까욥?

src/main/java/chess/controller/status/MoveController.java Outdated Show resolved Hide resolved
@Cl8D
Copy link
Author

Cl8D commented Mar 30, 2023

안녕하세요, 서브웨이! 레벨 인터뷰 준비로 인해 리뷰 요청이 늦어졌네요 ㅠㅠ
서브웨이가 말씀해 주신 방향대로 수정하면서 확실히 컨트롤러의 책임이 덜어지고, 서비스에게 비즈니스 로직을 줄 수 있게 되어서 이전보다 더 깔끔한 코드가 된 것 같아요!
또한, 아무 생각 없이 습관적으로 작성하던 관습들 (Long, 네이밍, equals 재정의 등등...)에 대해 다시 한 번 생각해 볼 수 있었어서 좋은 경험이 되었습니다.
개인적으로 테스트 관련해서 궁금한 점을 커멘트로 달아두었는데, 해당 부분 확인 부탁드립니다!
감사합니다 🙇‍♀️ 곧 방학인데 마지막까지 잘 부탁드려요!!

Copy link

@joseph415 joseph415 left a comment

Choose a reason for hiding this comment

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

안녕하세요 져니!

마지막 단계 리뷰 잘 적용해주셨네요💯
엔티티를 id로 관리되도록하고, 서비스로 비지니스 로직흐름이 다 이동한것 같아서, 책임 분리가 잘 된것 같습니다.

추가로 리뷰드릴것이 없어서 이만 머지하겠습니다! 체스 미션 끝까지 고생하셨습니다!
간단한 커멘트 하나 달아두었습니다. 확인한번 해주세요!

고생하셨습니다🙇‍♂️

@@ -13,7 +14,7 @@

public final class ChessBoardMapper {

public static Map<Position, Piece> from(final List<PieceEntity> pieceEntities) {
public static ChessBoard from(final List<PieceEntity> pieceEntities) {

Choose a reason for hiding this comment

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

ChessBaord 리턴하도록 한 것 👍

@@ -14,10 +14,10 @@

public final class MoveController implements Controller {

private final Long userId;
private final long userId;

Choose a reason for hiding this comment

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

이전 리뷰 댓글에 답변으로 달아 둔 것이 있는데, #604 (comment)

userId를 controller 가 갖고있으면 안될 것 같아서 리뷰드립니다!

@@ -72,19 +72,19 @@ private void play(final List<String> commands, final ChessGame chessGame) {
}

private void savePlayInfo(final Position source, final Position target, final ChessGame chessGame) {
final Long chessGameId = chessGameService.getChessGameId(userId);
final long chessGameId = chessGameService.getChessGameId(userId);

Choose a reason for hiding this comment

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

근데 저장되지 않은 chess는 null을 갖고 있을 텐데, 이렇게 long을 받도록 하는게 나은것인지 모르곘네요.

근데, null 일 경우에는 익센션 처리가 다 잘되있고, null 리턴이 아님이 보장되면 사용해도 무방할 것 같기도 합니다ㅎㅎ

@joseph415 joseph415 merged commit ea2eee0 into woowacourse:cl8d Mar 31, 2023
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