5주차 코드리뷰 요청 by ctaaag · Pull Request #126 · CodeSoom/react-week5-assignment-1

ctaaag · 2023-06-01T10:51:14Z

5주차 코드리뷰 요청드립니다!
현재까지 각각의 컴포넌트 테스트 및 코드 모두 작성했습니다.
우선 api와 테스트코드 모두 정상작동이 되게 짜놨으나,
조금 더 좋은 코드를 짤 수 있게 고민을 해서 조금씩 반영해놓겠습니다!

추가 작업 리스트

App 테스트코드 작성
reducer 테스트코드 작성

ctaaag · 2023-06-02T09:26:53Z

아래의 추가 작업을 진행하고 있습니다.
useEffect로 불러오는 부분과 데이터 페칭하는 부분 테스트코드 짜기가 어렵네요ㅎㅎ..
beforeEach 활용해서 하는 방법을 시도중입니다..!!

남은 추가 작업

테스트 커버리지 100% 달성
redux-thunk로 데이터 페칭하는 부분 테스트 추가하기 (beforeEach 활용?)
src 내의 api 파일 라인커버리지 0 해결하기
기능 추가
레스토랑 데이터 선택된 지역과 카테고리가 없을 때 데이터 불러오지 않기

hannut91 · 2023-06-02T16:33:01Z

src/App.test.jsx

+  useDispatch.mockImplementation(() => dispatch);
+  useSelector.mockImplementation(() => ({
+    regionData: [{ id: 1, name: '서울' }, { id: 2, name: '부산' }],
+    categoryData: [{ id: 1, name: '한식' }, { id: 2, name: '양식' }],
+    restaurantData: [{ id: 1, name: '코코식당' }, { id: 2, name: '네네식당' }],
+    selectedData: { selectedRegion: { id: 1, name: '서울' }, selectedCategory: { id: 1, name: '한식' } },
+  }));
+
+  const renderApp = () => render(<App />);
+
+  describe('RegionContainer가 렌더링 된다.', () => {
+    it('지역정보들이 input으로 보인다.', () => {
+      const { getByText } = renderApp();
+      expect(getByText(/부산/)).not.toBeNull();
+      expect(getByText(/서울/)).not.toBeNull();
+    });


데이터가 보인다는 것을 좀 드러내고 싶으시다면 이렇게도 해볼 수 있을 것 같아요

const regions = [{ id: 1, name: '서울' }, { id: 2, name: '부산' }]; useDispatch.mockImplementation(() => dispatch); useSelector.mockImplementation(() => ({ regionData: regions, categoryData: [{ id: 1, name: '한식' }, { id: 2, name: '양식' }], restaurantData: [{ id: 1, name: '코코식당' }, { id: 2, name: '네네식당' }], selectedData: { selectedRegion: { id: 1, name: '서울' }, selectedCategory: { id: 1, name: '한식' } }, })); const renderApp = () => render(<App />); describe('RegionContainer가 렌더링 된다.', () => { it('지역정보들이 input으로 보인다.', () => { const { container } = renderApp(); regions.forEach(({ name }) => { expect(container).toHaveTextContent(name); }); });

오호 forEach로 반복할 생각은 못했네요!
감사합니다 적용해볼게요!

hannut91 · 2023-06-03T05:56:28Z

src/App.test.jsx

+    it('선택된 식당이 바뀌면 ', () => {
+      useSelector.mockImplementation(() => ({
+        restaurantData: [
+          { id: 1, name: '바뀐식당' }, { id: 2, name: '무슨식당' },
+        ],
+      }));
+      const { getByText } = renderApp();
+      expect(getByText('바뀐식당')).not.toBeNull();
+      expect(getByText('무슨식당')).not.toBeNull();
+    });


다른 값이 들어갔을 경우 잘 동작하나 확인하고 싶으셨던 것 같습니다. 하지만 이것은 우리 프로그램에 어떤 행위를 할 때 어떤 변화가 일어나는지는 잘 드러내지는 못하는 것 같아요

넵..!! fireEvent로 식당 변경이 실행되었을 때 함수가 잘 작동되는지 테스트해보겠습니다!

hannut91 · 2023-06-03T05:56:41Z

src/CategoriesContainer.jsx

+export default function CategoriesContainer() {
+  const dispatch = useDispatch();
+  const { categoryData, selectedData } = useSelector((state) => state);
+  useEffect(() => {


여기 위에 빈 줄 하나를 추가해서 구분해주면 좋을 것 같아요

넵 반영하겠습니다 :)

hannut91 · 2023-06-03T05:57:03Z

src/CategoriesContainer.jsx

+  const dispatch = useDispatch();
+  const { categoryData, selectedData } = useSelector((state) => state);
+  useEffect(() => {
+    dispatch(fetchCategory());


카테고리 목록을 불러오는거니까 fetchCategories로 하는게 좋아보입니다

넵 전반적으로 단수형 복수형 통일이 안되어있는 것 같아서 그 부분 통일된 형태로 변경해보겠습니다!

hannut91 · 2023-06-03T05:58:26Z

src/CategoriesContainer.jsx

+
+export default function CategoriesContainer() {
+  const dispatch = useDispatch();
+  const { categoryData, selectedData } = useSelector((state) => state);


대상이 데이터니까 데이터라고 표현하신 것 같아요. Data라는 접미사는 아무런 일도 하지 않는 것 같습니다. categories같은 이름이 좋을 것 같고, selectedData는 무엇을 나타내는지 잘 모르겠네요.

Data는 사실 엄청 보편적인 단어죠. 우리 애플리케이션에서 보편적인 단어를 사용하고 있다면 무언가 잘못되었을 가능성이 높습니다. 왜냐하면 보편적일수록 해석할 수 있는 방법이 다양하기 때문이죠.

넵 고민해서 반영하겠습니다!

hannut91 · 2023-06-03T05:59:02Z

src/CategoriesContainer.test.jsx

+  jest.mock('react-redux');
+  jest.mock('./services/api');


이건 describe위에 선언해도 좋을 것 같아요. 이게 여기 안에 있으면 테스트에 따라서 다르게 바뀔수도 있다고 오해가 생길 것 같습니다

넵 알겠습니닷

hannut91 · 2023-06-03T06:00:07Z

src/Category.jsx

+export default function Category({ categoryData, selectedData, onClick }) {
+  return (
+    <>
+      {categoryData?.map((category) => (


categoryData가 없을수도 있어서 Optinal chaining을 사용하신 것 같습니다. categoryData가 없을 떄 categoryData의 값은 무엇인가요? 빈 배열이지 않나요? 빈 배열의 map연산은 아무런 일도 하지 않기 때문에, 비어있는 경우는 무시해도 될 것 같습니다

테스트할 때 categoryData가 없을 경우 undeftined 에러가 나서 옵셔널 체이닝을 사용했었는데
이 부분 다시 확인하고 안되어있으면 초기값을 빈배열로 넣어서 반영해보겠습니닷!

아 기본값이 undefined이군요. ㅎㅎ 그러면 사용하는게 맞지만, 카테고리 목록의 빈 값은 빈 목록이 자연스러워보이긴 합니다

hannut91 · 2023-06-03T06:01:42Z

src/action.js

+  };
+}
+
+export function fetchRestaurant({ selectedRegion, selectedCategory }) {


Arrow함수를 사용해도 좋을 것 같아요

넵! 반영하겠습니다

hannut91 · 2023-06-03T06:03:24Z

src/services/api.js

+export async function regionData() {
+  const url = 'https://eatgo-customer-api.ahastudio.com/regions';
+  try {
+    const data = await fetch(url);
+    const result = { regionData: await data.json() };
+    return result;
+  } catch (error) {
+    return console.log(error);
+  }
+}


이 try catch는 잘못된 사용방법입니다. 예외는 문제 발생 사실을 전달해야 하는 방향은 함수 호출의 역방향이어야 합니다. 왜냐하면 문제에 따라서는 더 큰 맥락을 알아야 문제를 해결하거나 대안을 제시할 수 있는데, 호출 사슬의 아래쪽보다 위쪽에 있는 함수들이 더 큰 맥락을 알고 있기 때문입니다.

지역 목록을 불러오는 것을 실패했을 때 올바르게 처리할 수 있으려면 이 API를 호출하는 쪽의 맥락을 알고 있어야 하는데, 이 함수는 모릅니다. 그래서 여기서 예외를 처리해 버리면 더 큰 맥락 관점에서 에러 처리를 못합니다.

앗..!! 저는 async await를 사용할 때는 무조건적으로 try catch를 사용하려고 했던 것 같아요
그럼 호출하는 쪽. 즉 실제로 이 함수를 쓰는 컴포넌트에서 에러 처리를 할 때 try catch를 사용하는 것이 올바른 사용이라고 이해하겠습니다!

무조건적으로 쓰면 안됩니다! 이렇게 써버리면 쓰는 입장에서는 에러가 안나서 좋지만(내결함성이 좋아짐) 오히려 에러가 나도 알 수 없는 상태가 되어 버립니다.

ctaaag · 2023-06-07T17:31:00Z

늦었지만 피드백 사항을 반영했습니다!
더불어서, 테스트 커버리지 향상을 위해 api, action 파일에 대한 테스트 코드 작성을 완료했습니다.
그런데 아직 100% 달성을 하지는 못했네요ㅠㅜ

ctaaag added 13 commits May 31, 2023 21:30

ADD: RegionContainer test complete

f9bae85

ADD: redux 구조 세팅

bb99cf9

Refactor:RegionContainer 테스트 수정

a87f515

ADD:Region컨테이너 컴포넌트 완성

edf2463

REFACTOR: RegionContainer 리팩토링

c22d949

ADD: Region 컴포넌트 및 테스트 완성

49d1a8b

ADD: CategoriesContainer test 완성

75af3f8

ADD: CategoryConainer 완성

7d411ec

ADD: Category 완성

d326475

ADD: RestaurantContainer 완성

81e4631

ADD: Restaurant 컴포넌트 완성

0282f88

ADD: App 테스트 추가

3f8015d

ADD: reducer 테스트 완성

46edcc7

hannut91 reviewed Jun 3, 2023

View reviewed changes

ctaaag added 4 commits June 7, 2023 21:26

Refactor : 변수명 및 try-catch문 수정

4f8ccdb

Refactor : 변수 및 함수명 변경

9a44c87

ADD: fixture로 테스트 데이터 분리

5a892b2

test 코드 리팩토링 및 테스트 커버리지 향상

3f1ded5

Conversation

ctaaag commented Jun 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ctaaag commented Jun 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ctaaag commented Jun 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ctaaag commented Jun 1, 2023 •

edited

Loading

ctaaag commented Jun 2, 2023 •

edited

Loading