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

Test/#2 server chat module 테스트 코드 추가 #5

Merged
merged 11 commits into from
Jan 9, 2025
Merged

Conversation

swkim12345
Copy link
Collaborator

closes #2

📂 작업 내용

  • chat 모듈 테스트
    • gateway
    • repository
    • service

💡 자세한 설명

server 쪽 chat module 내부 로직에 대해 테스트를 진행했습니다.
전반적으로 내부 로직을 검사하지 않는 블랙박스 방식으로 진행했고, 내부 로직을 검사할 때에는 함수 호출 여부만 확인하는 toHaveBeenCalled를 이용해 테스트를 작성했습니다.
현재 테스트만 작성되어 있는 상태이고, 부수적인 로직 변경과 같은 것은 진행되어 있지 않습니다. 이런 부분은 먼저 이슈로 발행한 다음 수정할 생각입니다.

// 예시
async sendMessage(roomId: string, playerId: string, message: string) {
    if (!message?.trim()) {
      throw new BadRequestException('Message cannot be empty');
    }

    const player = await this.chatRepository.getPlayer(roomId, playerId);
    if (!player) throw new PlayerNotFoundException();

    // return 타입에 대한 DTO 부재
    return {
      playerId,
      nickname: player.nickname,
      message,
      createdAt: new Date(),
    };
  }

📗 참고 자료 & 구현 결과 (선택)

스크린샷 2025-01-09 오전 10 00 00 스크린샷 2025-01-09 오전 10 07 21

🙋 질문

  1. module을 테스트하고 있는 데, 약간 고민중에 있습니다. 테스트하는 보람이 있을 지에 대해 어떻게 생각하시나요?

🚩 후속 작업 (선택)

리턴되는 값 type으로 선언 후 사용

✅ 셀프 체크리스트

  • PR 제목을 형식에 맞게 작성했나요?
  • 브랜치 전략에 맞는 브랜치에 PR을 올리고 있나요? (main이 아닙니다.)
  • 이슈는 close 했나요?
  • Reviewers, Labels를 등록했나요?
  • 작업 도중 문서 수정이 필요한 경우 잘 수정했나요?
  • 테스트는 잘 통과했나요?
  • 불필요한 코드는 제거했나요?

@swkim12345 swkim12345 added the ✅ test test 관련 label Jan 9, 2025
@swkim12345 swkim12345 requested a review from uuuo3o January 9, 2025 01:09
Copy link
Collaborator

@uuuo3o uuuo3o left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!

module을 테스트하고 있는 데, 약간 고민중에 있습니다. 테스트하는 보람이 있을 지에 대해 어떻게 생각하시나요?

이 부분에 대해서는... 솔직히 테스트하는 보람이 있는지는 잘 모르겠습니다. 성환님이 써주신 것처럼 모듈이 잘 정의되었고, 그에 해당하는 것들의 의존성 주입이 잘 됐는지만 확인하는 거라 테스트 코드 자체는 정말 아무 생각 없이 쓸 것도 같습니다.

물론 저는 가끔씩 추가했던 걸 까먹어서 모듈에 적지 않았거나 하는 경험이 있지만 글쎄요.. 모듈에 쓰는 것도 까먹었는데 테스트 코드에 추가하는 건 안까먹을까 싶기도 하고...ㅎㅎ
쓴다면 정말 단순한 코드가 되니까 부담없이 쓸 수는 있겠지만 굳이라는 생각을 떨칠 수는 없겠네요..


describe('ChatService', () => {
let chatService: ChatService;
let chatRepository: ChatRepository;

const mockChatRepository = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

이렇게 미리 정의해두고 아래 providers에 사용하니 훨씬 깔끔하고 보기 좋네요 ㅎㅎ

});

afterEach(() => {
jest.clearAllMocks();
Copy link
Collaborator

Choose a reason for hiding this comment

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

저는 테스트할 때마다 계속 덮어씌우고 해서 따로 처리하지 않았네요! 앞으로는 이걸 넣어야겠어요

profileImage: null,
score: 10,
};
mockChatRepository.getPlayer.mockResolvedValue(player);
Copy link
Collaborator

Choose a reason for hiding this comment

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

이런 식으로 쓰니 훨씬 깔끔하네요.
제 코드 상으로는 jest.spyOn(mockChatRepository, 'getPlayer').mockResolvedValue(player) 이런 식으로 썼을 텐데 말이죠


describe('getPlayer 테스트', () => {
it('플레이어 데이터가 없을 때 null을 리턴', async () => {
mockRedisService.hgetall.mockResolvedValue(null);
Copy link
Collaborator

Choose a reason for hiding this comment

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

없는 데이터에 대해서 hgetall 을 하면 null 이 안나올텐데.. 이런 생각을 했었는데 이제보니 코드에서 length가 0이면 null이 나오게 했었군요! 덕분에 코드 한번씩 더 읽고 갑니다...

@@ -27,18 +27,18 @@ describe('ChatService', () => {
});

describe('sendMessage 테스트', async () => {
it('메시지가 공백일 때', async () => {
it('메시지가 공백일 때 BadRequestException 발생', async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

이렇게 어떤 에러가 뜨는지 적는게 훨씬 보기 좋네요!

it('roomId가 null일 때 BadRequestException을 발생', () => {
mockSocket.handshake.auth = { roomId: null };

expect(() => gateway.handleConnection(mockSocket as Socket)).toThrow(BadRequestException);
Copy link
Collaborator

Choose a reason for hiding this comment

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

저처럼 any를 쓰는 것보다는 괜찮아보이지만 이렇게 모든 mockSocket에 대해서 다시 as Socket을 붙여야 하니... 정말 뭔가뭔가... 아쉽네요

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

맞아요.. 이거는 좀 아쉬운 거 같아요. 더 좋은 방법을 찾으면 업데이트할게요!

@@ -62,4 +63,40 @@ describe('ChatService', () => {
expect(mockChatRepository.getPlayer).toHaveBeenCalled();
});
});

describe('existsRoom 테스트', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기가 제가 말했던 중복된 부분이잖아요. existsRoom이랑 existsPlayer
형태는 살짝은 다르긴하지만(mocking 하는 방식 등) 테스트 코드 자체도 중복되어보이는데 이런 부분은 합쳐서 공통된 부분으로 변경하는 것이 좋을까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이건 약간 고민하는 게 좋을 거 같아요. 방을 찾고, 플레이어를 찾는 로직상 다른 역할을 하는 함수 두개가 있는 데, 이를 하나의 테스트 코드를 가지고 임계값 체크를 하면 unit test의 역할에서 벗어날 수 있을 거 같아요.
제 생각은 통합테스트에서 이 부분을 통합해서 테스트하고, 이후 e2e 테스트에서 추가적으로 진행하는 게 더 좋을 거 같습니다.

@swkim12345 swkim12345 merged commit 89341cb into dev-be Jan 9, 2025
@swkim12345 swkim12345 deleted the test/#2 branch January 9, 2025 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✅ test test 관련
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants