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

리더보드 페이지가 실제 데이터를 사용하도록 수정 #79

Merged
merged 4 commits into from
Nov 23, 2024

Conversation

DaleSeo
Copy link
Contributor

@DaleSeo DaleSeo commented Nov 22, 2024

리더보드 페이지가 임시로 하드코딩된 데이터를 사용하는 useMembers 훅 함수를 통해서 GitHub API를 호출한 실제 데이터를 사용하도록 수정하였습니다. 미팅 때 논의했던 것처럼 다른 페이지 컴포넌트를 수정하실 때 레퍼런스가 될 수 있었으면 합니다.

Shot 2024-11-21 at 19 20 30

체크리스트

@DaleSeo DaleSeo self-assigned this Nov 22, 2024
@@ -56,7 +56,8 @@
"typescript": "^5.6.3",
"typescript-eslint": "^8.10.0",
"vite": "^5.4.9",
"vitest": "^2.1.3"
"vitest": "^2.1.3",
"vitest-mock-extended": "^2.0.2"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

#28 (comment) 에서도 제안드렸던 테스팅 유틸리티인데요. 타입 안전한 모킹을 하기 위해서 들어가는 시간과 노력을 최소화하는데 도움이 됩니다. 사용법은 아래 테스트 코드를 참고하세요.

@@ -94,7 +94,7 @@ export default function Footer() {
];

return (
<footer className={styles.footer}>
<footer className={styles.footer} aria-label="Site Footer">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

footer 요소는 하나의 페이지에서 여러 군데 사용될 수 있기 때문에, 사이트 수준 footer 요소에 레이블을 달아주었습니다. 이렇게 해주지 않으면, 테스트 코드에서 screen.getByRole("contentinfo")을 할 때 의도치 않게 여러 요소가 선택되어 문제가 될 수 있습니다.

Copy link
Member

Choose a reason for hiding this comment

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

이전에는 막연히 footer 요소가 웹 페이지에서 한 군데서만 사용하는 것으로 여겨왔는데, 여러 곳에서 쓰일 때 생길 수 있는 문제가 있었군요!

Comment on lines +53 to +58
mock<Member>(),
mock<Member>(),
mock<Member>(),
mock<Member>(),
mock<Member>(),
mock<Member>(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

vitest-mock-extended 덕분에 모킹이 이렇게 쉬워집니다! 이 테스트에서는 각 멤버가 어떤 속성을 갖는지는 별로 중요하기 않기 때문에 모두 생략할 수 있었습니다.

Copy link
Member

Choose a reason for hiding this comment

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

대박! 타입만 전달하면 이렇게 되는군요? ts-mockito가 떠올랐습니다...

Comment on lines -40 to -44
members.forEach((member, index) => {
const memberItem = memberItems[index];
expect(memberItem).toHaveTextContent(`등급: ${member.rank}`);
expect(memberItem).toHaveTextContent(`진행 상황: ${member.solved}`);
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

이렇게 디테일한 부분은 카드 컴포넌트의 테스트에서 커버될 거라고 가정하고 페이지 레벨에서는 중복해서 테스트하지 않도록 할께요.

Comment on lines +11 to +13
vi.mocked(useMembers).mockReturnValue(
mock({ isLoading: true, error: null, members: [] }),
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

테스트가 실행될 때 GitHub API와 네트워크 통신이 일어나지 않도록 데이터를 호출하는 훅 함수를 모킹하였습니다.

const [members, setMembers] = useState<Member[] | null>(null);
const [members, setMembers] = useState<Member[]>([]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sounmind 컴포넌트 단에서 불필요한 null 체크를 피하기 위해서 members의 기본값을 null에서 []로 변경하였습니다. 아직 데이터 조회가 되지 않아서 빈 배열인지, 데이터가 정말 없어서 빈 배열인지는, isLoading을 통해서 판단이 가능하기 때문에, null로 굳이 이중으로 표현할 필요 없다고 생각합니다.

Copy link
Member

Choose a reason for hiding this comment

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

그랬군요. 의도를 설명해주셔서 감사합니다!

  • 데이터 조회를 성공했지만 빈 배열: isLoading이 false -> true -> false.
  • 아예 조회조차 하지 않고 빈 배열: isLoading이 계속 false.

현재 요구사항에서는 이 방식으로 판단해도 전혀 문제 없을 것 같네요 😄

@DaleSeo DaleSeo marked this pull request as ready for review November 22, 2024 00:43
@DaleSeo DaleSeo requested a review from a team as a code owner November 22, 2024 00:43
@sounmind
Copy link
Member

deps: add vitest-mock-extended as dep depedency
@DaleSeo 커밋 이름에서 dep는 devDependencies를 의도하신 것 맞을까용?

@DaleSeo DaleSeo force-pushed the leaderboard-use-members branch from 518c9b4 to 69f62a2 Compare November 22, 2024 13:50
@DaleSeo DaleSeo force-pushed the leaderboard-use-members branch from 69f62a2 to d5b6a35 Compare November 22, 2024 13:51
@DaleSeo
Copy link
Contributor Author

DaleSeo commented Nov 22, 2024

커밋 이름에서 dep는 devDependencies를 의도하신 것 맞을까용?

오타를 냈네요 ㅋㅋ 정정하겠습니다.

@DaleSeo DaleSeo merged commit 7affc1a into main Nov 23, 2024
1 check passed
@DaleSeo DaleSeo deleted the leaderboard-use-members branch November 23, 2024 00:08
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.

Entire Leaderboard Page
2 participants