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] hook 테스트 추가 #182

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

[Test] hook 테스트 추가 #182

wants to merge 10 commits into from

Conversation

novice0840
Copy link
Contributor

@novice0840 novice0840 commented Aug 15, 2024

Issue Number

#152

As-Is

기능 구현에 급급해서 테스트 코드가 부실했던 상황

To-Be

useMakeOrEnterRoom
useGameStart
useGetRoomInfo

이렇게 3개의 hook에 대한 테스트가 추가되었다.

Check List

  • 테스트가 전부 통과되었나요?
  • 모든 commit이 push 되었나요?
  • merge할 branch를 확인했나요?
  • Assignee를 지정했나요?
  • Label을 지정했나요?

Test Screenshot

image

(Optional) Additional Description

mocking이 너무 많아 실질적으로 테스트하는 부분이 적어 테스트가 추가된 것은 좋으나 효용성이 있는지에 대한
의문이 들긴했다.

🌸 Storybook 배포 주소

https://woowacourse-teams.github.io/2024-ddangkong/storybook/

@novice0840 novice0840 added ✅ test 테스트 관련 🫧 FE front end labels Aug 15, 2024
@novice0840 novice0840 added this to the FE Sprint4 milestone Aug 15, 2024
@novice0840 novice0840 self-assigned this Aug 15, 2024
@novice0840 novice0840 linked an issue Aug 15, 2024 that may be closed by this pull request
3 tasks
@novice0840 novice0840 changed the title Test/#152 [Test] hook 테스트 추가 Aug 16, 2024
Copy link
Contributor

@rbgksqkr rbgksqkr left a comment

Choose a reason for hiding this comment

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

테스트 코드 작성하느라 고생해써요~~~!!!! 테스트 코드를 모킹하느라 고생이 많았을 게 예상이 되네요 ㅎㅎ 근데 팀에서 정했던 테스트 코드 목적과 범위랑 살짝 달랐던 것 같아서 코멘트 살짝 남겨봤습니다!~!

@@ -30,6 +30,7 @@ const enableMocking = async () => {
enableMocking().then(() => {
ReactDOM.createRoot(document.getElementById('root')!).render(
<QueryClientProvider client={queryClient}>
{process.env.NODE_ENV === 'development' && <ReactQueryDevtools initialIsOpen={true} />}
Copy link
Contributor

Choose a reason for hiding this comment

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

💭 질문 💭

해당 분기처리를 한 이유가 궁금합니다! ReactQueryDevtools는 프로덕션 빌드에서 제거되기 때문에 분기처리를 할 필요가 없다고 생각했습니다.

image

https://tanstack.com/query/latest/docs/framework/react/devtools#devtools-in-production

const mockEnterRoom = jest.fn();

beforeEach(() => {
jest.clearAllMocks();
Copy link
Contributor

@rbgksqkr rbgksqkr Aug 16, 2024

Choose a reason for hiding this comment

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

🙏 제안 🙏

jest.config에서 clearMocks:true를 설정해서 clearAllMocks를 별도로 적용하지 않아도 될 것 같아요!

https://jestjs.io/docs/configuration#clearmocks-boolean


// Then
expect(mockCreateRoom).toHaveBeenCalledTimes(1);
expect(mockEnterRoom).not.toHaveBeenCalled();
Copy link
Contributor

Choose a reason for hiding this comment

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

🙏 제안 🙏

모든 함수를 모킹해서 이 테스트 코드가 작성이 된 것 같은데, PR에도 작성해준 것처럼 효용을 느끼기 어려운 것 같아요!
아니면 user가 방 생성 버튼을 클릭하고, mutation에서 반환하는 데이터를 확인하는 건 어떨까요??

it('멤버가 확인 버튼을 누른 경우 방에 참여한다.', () => {
// Given
(useRecoilState as jest.Mock).mockReturnValue([{ isMaster: false }, jest.fn()]);
const { result } = renderHook(() => useMakeOrEnterRoom(), {
Copy link
Contributor

Choose a reason for hiding this comment

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

💭 질문 💭

useMakeOrEnterRoom 훅이 이름부터 두 가지 역할을 한다고 생각이 들었는데 테스트 코드를 보니 더 그게 느껴지네요. 다른 테스트 케이스의 다른 입력값인데 테스트 수행 과정이 같다는 게 어색한 것 같아요 어떻게 생각하시나요??

jest.mock('@tanstack/react-query', () => ({
...jest.requireActual('@tanstack/react-query'),
useMutation: jest.fn(),
}));
Copy link
Contributor

Choose a reason for hiding this comment

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

🙏 제안 🙏

mutate를 jest로 모킹하는 것보단 현재 msw를 사용하고 있으니까 API 모킹을 msw로 활용해보는 건 어떨까요??

jest.mock('recoil', () => ({
...jest.requireActual('recoil'),
useRecoilValue: jest.fn(),
}));
Copy link
Contributor

Choose a reason for hiding this comment

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

🌸 칭찬 🌸

모킹을 야무지게 잘하셨네요 굿굿 근데 recoil로 관리하던 유저 정보를 쿠키로 관리하도록 바뀌면 테스트코드를 다 다시 작성해야할 우려가 있긴 하네용

}));

describe('useGetRoomInfo', () => {
it('게임이 시작되면 모든 유저는 게임화면으로 이동한다.', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

🙏 제안 🙏

이런 테스트 케이스가 애매하다고 느껴졌는데, 페이지를 이동하는 부분은 cypress의 역할로 양도하는 건 어떨까요? 모킹하느라 힘들었을 것 같은데 테스트 명세에 작성된 "게임화면으로 이동한다"에 비해, 화면으로 이동되는 것을 해당 테스트 코드에서 확인하기 어려운 것 같아요!

Copy link
Contributor

@useon useon left a comment

Choose a reason for hiding this comment

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

저도 어제 테스트를 작성하느라 하루 종일 테스트 생각 밖에 안 한 것 같아요. 저도 이번 테스트 작성에 recoil을 모킹했지만, 과연 모킹이 많은 테스트가 좋은 테스트일까 하는 근본적인 고민부터 들었어요 방장 여부에 따른 구현이 다르기 때문에 어쩔 수 없다고 판단했지만요 !🥹 포메도 해당 작업들에 대한 테스트 코드를 작성하면서 많은 고민을 한 것 같네요. 지금 저희가 이제 recoil에서 cookie로 변경이 되면서 테스트 코드도 많이 수정될 것 같은 예감이 .. ^ ^ 따라서 지금 테스트 작업에는 최소한의 리소스를 들이고 P1 기능 개발로 얼른 나아갑시다 .. ! 수고 많았어요 폼에 ^. ^

(useRecoilValue as jest.Mock).mockReturnValue({ isMaster: false });
const { result } = renderHook(() => useGameStart(), {
wrapper,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

🙏 제안 🙏

테스트마다 반복되는 코드들이 있는데, 제가 작성한 방식이긴 이런 식으로 중복 코드를 수정해볼 수도 있을 것 같아요 ! 하지만 테스트마다 해당 작업을 보고 싶다면 포메처럼 매 테스트 작성하는 것도 방법일 수 있겠네요 !! 해당 방법에 대해 포메는 어떻게 생각하시나요 ? 🤓

import { renderHook, act } from '@testing-library/react-hooks';
import { useRecoilValue } from 'recoil';
import { useGameStart } from './useGameStart';
import { customRender } from '@/test-utils';
import { memberInfoState } from '@/recoil/atom';
import { useMutation } from '@tanstack/react-query';

jest.mock('@tanstack/react-query', () => ({
  ...jest.requireActual('@tanstack/react-query'),
  useMutation: jest.fn(),
}));

jest.mock('recoil', () => ({
  ...jest.requireActual('recoil'),
  useRecoilValue: jest.fn(),
}));

describe('useGameStart hook 테스트', () => {
  const mockStartGameMutation = jest.fn();

  const renderWithRecoilState = (isMaster: boolean) => {
    (useRecoilValue as jest.Mock).mockReturnValue({ isMaster });

    return renderHook(() => useGameStart(), {
      wrapper: ({ children }) => (
        customRender(
          <RecoilRoot
            initializeState={(snap) => {
              snap.set(memberInfoState, { memberId: 1, nickname: 'Test User', isMaster });
            }}
          >
            {children}
          </RecoilRoot>
        )
      ),
    });
  };

  beforeEach(() => {
    jest.clearAllMocks();
    (useMutation as jest.Mock).mockImplementation(() => ({
      mutate: mockStartGameMutation,
    }));
  });

  it('방장이 게임 시작 버튼을 누른 경우 게임이 시작된다', () => {
    // Given
    const { result } = renderWithRecoilState(true);

    // When
    act(() => {
      result.current.handleGameStart();
    });

    // Then
    expect(mockStartGameMutation).toHaveBeenCalledTimes(1);
  });

  it('멤버가 게임 시작 버튼을 누른 경우에는 게임이 시작되지 않는다.', () => {
    // Given
    const { result } = renderWithRecoilState(false);

    // When
    act(() => {
      result.current.handleGameStart();
    });

    // Then
    expect(mockStartGameMutation).not.toHaveBeenCalled();
  });
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🫧 FE front end ✅ test 테스트 관련
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

[TEST] hook 테스트 추가
3 participants