Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughApply 관리 페이지 관련 테스트가 추가되었습니다: ApplyAdminClientPage 통합 테스트, Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
일반 문자로 처리했을 경우 라벨UI와 폼의 숫자를 알려주는 ui의 역할이 달라 단순 텍스트가 아닌 역할별로 분류하여 코드를 다시 작성했습니다.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/test/apply/apply-admin.data.ts`:
- Around line 29-37: mockSingleForm duplicates data from mockForms[0]; change it
to derive from the single source to avoid drift by assigning it from
mockForms[0] (use a deep copy to prevent shared mutation) instead of hardcoding
values. Update the declaration of mockSingleForm to construct its element from
mockForms[0] (e.g., create a deep clone via structuredClone or JSON
parse/stringify) so future updates to mockForms automatically reflect in
mockSingleForm.
In `@src/test/apply/ApplyAdminPage.test.tsx`:
- Around line 132-142: The test fails because the regexes for filter buttons
assume a space in labels (e.g., /진행 중/) but rendered labels may be without
spaces; update the matchers in ApplyAdminPage.test (the getFilter usage in the
'필터별 개수가 정확하게 표시된다.' test) to use whitespace-tolerant regexes (e.g., /진행\s*중/
and /진행\s*전/) so buttons like "진행중" or "진행전" are matched reliably when rendering
ApplyAdminClientPage.
In `@src/test/apply/useFormFilter.test.ts`:
- Around line 62-87: The two tests duplicate behavior for the "진행 중" filter;
consolidate them by keeping one spec that asserts both the formFilter state and
the filteredForms contents/length, or replace them with a single parameterized
test using it.each. Update the test that uses useFormFilter(mockForms) and
handleFilterChange(FORM_STATUS_FILTER.IN_PROGRESS) to assert formFilter ===
FORM_STATUS_FILTER.IN_PROGRESS, filteredForms has the expected length and every
item has formStatus === '진행 중' (or use a table-driven it.each over status +
expected results) to remove duplication while still verifying filteredForms and
formFilter together.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/test/apply/ApplyAdminPage.test.tsx`:
- Around line 178-180: The test is asserting implementation detail by checking
emptyMessage.tagName === 'P'; remove that assertion and only verify the
user-visible behavior (e.g., presence/text of emptyMessage). Locate the
assertions in ApplyAdminPage.test.tsx that reference emptyMessage and delete the
expect(emptyMessage.tagName).toBe('P') line, or replace it with a content-based
check such as verifying the message text or role if needed (keep
expect(emptyMessage).toBeInTheDocument()).
- Line 195: The assertion is using mockForms[0].title while the test sets its
query data with mockSingleForm, causing inconsistent test data references;
update the assertion in ApplyAdminPage.test.tsx to reference
mockSingleForm.title (or make the query use mockForms) so the test consistently
uses the same mock object (search for the expect(...queryAllByText(...)) line
and replace mockForms[0].title with mockSingleForm.title).
keemsebin
left a comment
keemsebin
left a comment
There was a problem hiding this comment.
코멘트 남겼습니다! 테스트 짜시느라 수고하셨습니당~!
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| describe('렌더링 테스트', () => { |
There was a problem hiding this comment.
렌더링 테스트 보다는 지원서 페이지에 생성 버튼, 필터링 옵션, 지원서 목록 등이 존재하는지 확인한다. 처럼 의미를 명확히 하는건 어떤가요??
| expect(screen.getByText(/종료/)).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('지원서 목록이 표시된다.', () => { |
There was a problem hiding this comment.
지원서가 존재하는 경우, 목록에 제목, 기간을 담고 있는 카드가 노출된다 처럼 의미를 명확히 명시해주면 좋을 것 같아요~
| }); | ||
| }); | ||
|
|
||
| it('각 지원서에 기간이 표시된다.', () => { |
There was a problem hiding this comment.
기간, 상태 배지의 경우 하나의 카드 뷰 내에 함께 위치하는 정보라 it 구문으로 구분하지 않고 하나로 노출시키는 것이 좋을 것 같아요~
| }); | ||
| }); | ||
|
|
||
| describe('필터 기능 테스트', () => { |
There was a problem hiding this comment.
요 부분도 의미 명확하게 해주면 좋을 것 같아요!
| }); | ||
|
|
||
| it('"진행전" 필터 클릭 시 진행 전 상태의 지원서만 표시된다.', async () => { | ||
| const user = userEvent.setup(); |
There was a problem hiding this comment.
공통되는 userEvent 는 상위에서 선언해서 반복 선언을 줄일 수 있을 것 같네용
| }); | ||
|
|
||
| describe('예외 케이스', () => { | ||
| it('지원서가 없을 경우 안내 메시지 UI가 표시된다.', () => { |
There was a problem hiding this comment.
지원서 목록에 지원 카드가 없는 경우 생성된 지원서가 없습니다 라는 헬퍼 메시지를 띄운다. 처럼 명확하게 해주시면 좋을 것 같아용
| }); | ||
| }); | ||
|
|
||
| it('지원서가 하나만 있을 경우에도 정상적으로 표시된다.', () => { |
There was a problem hiding this comment.
요 부분은 예외가 아니라고 생각되는데, 예외 케이스에 있는 이유가 있나요??
There was a problem hiding this comment.
각 필터별 지원서가 있는 경우가 테스트 하기에 이상적인 상황이라고 생각했습니다.
따라서 지원서가 1개이거나 없는 경우를 예외적인 상황이라고 생각했는데 예외가 아닌
필터 조건에 맞는 데이터가 없을 경우 안내 메시지를 표시하고, 데이터가 존재할 경우 목록을 렌더링한다 라고 명확하게 표시하는게 더 나아보이네요!
007c1ed
| }); | ||
|
|
||
| it('지원서가 하나만 있을 경우에도 정상적으로 표시된다.', () => { | ||
| testQueryClient.setQueryData(['apply'], mockSingleForm); |
There was a problem hiding this comment.
매번 요렇게 테스트쿼리를 작성해야 하는 이유가 있을까요? get 요청을 모킹한다고 생각했는데, 요 테스트 쿼리클라이언트의 이점은 무엇인가요??
There was a problem hiding this comment.
각 테스트 케이스마다 필요한 시나리오 데이터를 명확히 주입할 수 있는게 장점이라고 생각합니다. 그렇기 때문에 매번 작성했는데 비효율적인 방법이라면 어떤 방법들을 통해서 개선할 수 있을지 궁금합니다!
| expect(result.current.formCounts[FORM_STATUS_FILTER.IN_PROGRESS]).toBe(1); | ||
| expect(result.current.formCounts[FORM_STATUS_FILTER.CLOSED]).toBe(1); | ||
| expect(result.current.formCounts[FORM_STATUS_FILTER.BEFORE]).toBe(1); |
There was a problem hiding this comment.
요 테스트들이 이미 AdminPage.test.tsx에 존재한다고 생각해서 의미있는 테스트인가?하는 생각이 드네요. 어떻게 생각하시나요??
expect(getFilter(/진행전/)).toHaveTextContent('(1)');
expect(getFilter(/진행중/)).toHaveTextContent('(1)');
expect(getFilter(/종료/)).toHaveTextContent('(1)');There was a problem hiding this comment.
저는 훅의 기능적으로 테스트하고 컴포넌트간의 상호작용에 대해서 따로 테스트를 해야한다고 생각했어서 작성했습니다. 하지만 의미가 없는 테스트라면 AdminPage.test.tsx 와 훅 테스팅 파일 중 어느 파일에 담겨있는게 조금 더 의미가 있을까요?
src/test/apply/useFormFilter.test.ts
Outdated
|
|
||
| expect(result.current.formFilter).toBe(FORM_STATUS_FILTER.IN_PROGRESS); | ||
| expect(result.current.filteredForms).toHaveLength(1); | ||
| expect(result.current.filteredForms[0].formStatus).toBe('진행 중'); |
There was a problem hiding this comment.
0번째 존재하는 데이터가 진행중이다라는 것보다는 실제 배열에 어떤 데이터가 담길지 테스트하는 것이 더 명확해 보입니당
실제로 배열에 담길 데이터를 기반으로 테스트를 진행하였습니다.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/test/apply/ApplyAdminPage.test.tsx`:
- Around line 42-53: The test currently asserts hardcoded status texts for each
card; change it to assert the status using the mock data's form.formStatus for
each entry. In the test block inside ApplyAdminPage.test.tsx that renders
ApplyAdminClientPage and iterates mockForms, replace the repeated
expect(screen.getByText('진행 중' / '마감' / '진행 전')) checks with a single assertion
using expect(screen.getByText(form.formStatus)).toBeInTheDocument() so each
card's displayed status is validated against mockForms' formStatus.
| it('생성된 지원서의 이름 및 기간을 담고있는 카드가 표시된다.', () => { | ||
| render(<ApplyAdminClientPage />); | ||
|
|
||
| mockForms.forEach((form) => { | ||
| expect(screen.getByText(form.title)).toBeInTheDocument(); | ||
| expect( | ||
| screen.getByText(`${form.startDate} ~ ${form.endDate}`), | ||
| ).toBeInTheDocument(); | ||
| expect(screen.getByText('진행 중')).toBeInTheDocument(); | ||
| expect(screen.getByText('마감')).toBeInTheDocument(); | ||
| expect(screen.getByText('진행 전')).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
상태 검증은 mock 데이터의 formStatus를 직접 사용하세요.
하드코딩된 상태 텍스트를 반복적으로 검증하면 중복이 생기고 데이터 변경에 취약합니다. 각 카드에 대해 form.formStatus를 검증하면 더 견고합니다.
♻️ 리팩터링 제안
mockForms.forEach((form) => {
expect(screen.getByText(form.title)).toBeInTheDocument();
expect(
screen.getByText(`${form.startDate} ~ ${form.endDate}`),
).toBeInTheDocument();
- expect(screen.getByText('진행 중')).toBeInTheDocument();
- expect(screen.getByText('마감')).toBeInTheDocument();
- expect(screen.getByText('진행 전')).toBeInTheDocument();
+ expect(screen.getByText(form.formStatus)).toBeInTheDocument();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('생성된 지원서의 이름 및 기간을 담고있는 카드가 표시된다.', () => { | |
| render(<ApplyAdminClientPage />); | |
| mockForms.forEach((form) => { | |
| expect(screen.getByText(form.title)).toBeInTheDocument(); | |
| expect( | |
| screen.getByText(`${form.startDate} ~ ${form.endDate}`), | |
| ).toBeInTheDocument(); | |
| expect(screen.getByText('진행 중')).toBeInTheDocument(); | |
| expect(screen.getByText('마감')).toBeInTheDocument(); | |
| expect(screen.getByText('진행 전')).toBeInTheDocument(); | |
| }); | |
| it('생성된 지원서의 이름 및 기간을 담고있는 카드가 표시된다.', () => { | |
| render(<ApplyAdminClientPage />); | |
| mockForms.forEach((form) => { | |
| expect(screen.getByText(form.title)).toBeInTheDocument(); | |
| expect( | |
| screen.getByText(`${form.startDate} ~ ${form.endDate}`), | |
| ).toBeInTheDocument(); | |
| expect(screen.getByText(form.formStatus)).toBeInTheDocument(); | |
| }); |
🤖 Prompt for AI Agents
In `@src/test/apply/ApplyAdminPage.test.tsx` around lines 42 - 53, The test
currently asserts hardcoded status texts for each card; change it to assert the
status using the mock data's form.formStatus for each entry. In the test block
inside ApplyAdminPage.test.tsx that renders ApplyAdminClientPage and iterates
mockForms, replace the repeated expect(screen.getByText('진행 중' / '마감' / '진행 전'))
checks with a single assertion using
expect(screen.getByText(form.formStatus)).toBeInTheDocument() so each card's
displayed status is validated against mockForms' formStatus.
3 participants
🔥 연관 이슈
🚀 작업 내용
🤔 고민했던 내용
it()을 통해서 테스트를 진행했는데 루프를 활용해 동시에 처리를 하는게 좋을지 고민됐으나 분리하는것이 더 쉽게 이해할 수 있을 것 같아 따로 분리했습니다.💬 리뷰 중점사항
테스트를 따로 분리하는 명확한 기준이 있다면 그 기준이 궁금합니다.
3번의 내용처럼 중복되는 부분을 상태에 따라서 개별적으로 다르게 테스트를 진행하는게 좋을지 루프를 사용하는것이 좋을지 궁금합니다.
Summary by CodeRabbit