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

충남대 FE_강병현 5주차 과제 Step1 #93

Open
wants to merge 7 commits into
base: kang-kibong
Choose a base branch
from

Conversation

kang-kibong
Copy link

No description provided.

@taehwanno taehwanno self-requested a review July 28, 2024 13:40
Copy link

@taehwanno taehwanno left a comment

Choose a reason for hiding this comment

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

피드백 드려요!

return <FormProvider {...methods}>{children}</FormProvider>;
}

test('현금영수증 Checkbox가 체크되지 않은 경우 현금영수증 필드 렌더링 테스트 ', async () => {

Choose a reason for hiding this comment

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

Suggested change
test('현금영수증 Checkbox가 체크되지 않은 경우 현금영수증 필드 렌더링 테스트 ', async () => {
test('현금영수증 신청을 하지 않았다면 번호 입력과 발급 유형 선택은 보여주지 않아야 한다.', async () => {

테스트 케이스를 통해 기대하는 결과가 무엇인지 명확하도록 테스트 명세를 적어주시는게 중요합니다. 그래야 미래의 내가 코드를 수정했을 때 혹은 동료 개발자가 깨진 테스트 코드를 보고 테스트 코드를 수정해야하는지, 변경한 코드를 개선해야하는지 쉽게 판단할 수 있거든요.

Comment on lines +53 to +58
await waitFor(() => {
expect(cashReceiptCheckbox).toBeChecked();
expect(screen.getByPlaceholderText('(-없이) 숫자만 입력해주세요.')).toBeInTheDocument();
expect(screen.getByText('개인소득공제')).toBeInTheDocument();
expect(screen.getByText('사업자증빙용')).toBeInTheDocument();
});

Choose a reason for hiding this comment

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

Suggested change
await waitFor(() => {
expect(cashReceiptCheckbox).toBeChecked();
expect(screen.getByPlaceholderText('(-없이) 숫자만 입력해주세요.')).toBeInTheDocument();
expect(screen.getByText('개인소득공제')).toBeInTheDocument();
expect(screen.getByText('사업자증빙용')).toBeInTheDocument();
});
expect(cashReceiptCheckbox).toBeChecked();
expect(screen.getByPlaceholderText('(-없이) 숫자만 입력해주세요.')).toBeInTheDocument();
expect(screen.getByText('개인소득공제')).toBeInTheDocument();
expect(screen.getByText('사업자증빙용')).toBeInTheDocument();

컴포넌트에서 비동기 요청을 통해 상태 변화가 발생하는건 없어서 waitFor를 사용해주지 않아도 테스트는 통과하는거 같아요. waitFor처럼 특정 조건을 기다리는 테스트 관련 API는 꼭 필요한 상황이 아니라면 지양하는게 좋습니다. 이런 코드는 테스트 성능에 영향을 미치거든요.

// Then 유효성 검사 함수가 호출되고 에러 메시지가 표시되어야 한다
await waitFor(() => {
expect(mockValidatePayment).toHaveBeenCalledWith('', true, '');
expect(window.alert).toHaveBeenCalledWith('Error: Cash receipt number is required');

Choose a reason for hiding this comment

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

테스트 코드가 테스트하는 코드의 내부구현에 결합되어 의존하기 때문에 validatePayment를 mocking 해서 기대하는 값으로 호출했는지 확인하는건 불필요하다고 봅니다. 최종적으로 저희가 확인하면 되는건 UI가 최종적으로 어떻게 표현되는지 인데 이는 window.alert의 호출 여부로 검증할 수 있으니까요.

오히려 해당 방향처럼 테스트 코드가 작성되고 나서 내부 구현이 변경됐지만 validatePayment mocking 코드는 그대로 라면 의도치 않게 테스트가 깨져야 하는데 깨지지 않거나, 깨지지 않아야 하는데 깨지는 상황이 발생할 수 있습니다.

다른 멘티분들에게 드린 코멘트와 참조된 블로그 글들 읽어보시면 좋을 거 같아요.

  1. 전남대 FE_정서윤 5주차 과제 Step3 #78 (comment)
  2. 충남대 FE_김지안_5주차 과제 #53 (comment)

expect(image).toHaveAttribute(
'src',
'https://st.kakaocdn.net/product/gift/product/20240215083306_8e1db057580145829542463a84971ae3.png',
);

Choose a reason for hiding this comment

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

waitFor를 크게 쓰기보다는 꼭 써야하는 요소에 한해서 좁은 범위에 쓰시는게 추후 비동기로 변화한 UI가 어떤 상호작용까지 영향을 미치는지 쉽게 파악할 수 있어요.

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.

2 participants