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

5-1 레스토랑 조회 구현하기 #112

Merged
merged 82 commits into from
Jul 12, 2022
Merged

Conversation

alstjr0183
Copy link

@alstjr0183 alstjr0183 commented Jul 5, 2022

안녕하세요!! 아직 reducer쪽 테스트 코드만 작성했는데 리뷰 요청드리겠습니다!!

redux, redux-thunk을 셋팅해주었습니다
index.js에 redux를 연동해주었습니다
레스토랑 지역을 가져오는 지역을 가져오는 reducer 테스트 코드를
작성했습니다
지역 리스트를 보여주는 상태인데 레스토랑이라는 부분은 굳이없어도
될거같아서 뺐습니다
getRegions의 it을 추가한다
regions 테스트 상태를 작성했습니다
지역을 나타내는정보를 얻어오는 역할을하기때문에 restaurant라는 네이밍을
제거해주었습니다
reducer의 음식 카테고리를 얻는 테스트를 추가했습니다
fixtures에 테스트 상태를 추가해주었습니다
categories 불러오는 api를 추가해주었습니다
카테고리 목록을 들고오는 액션을 추가했습니다
reducer에 카테고리 리스트를 저장시켜주는 로직을 추가하였습니다
getCategories의 설명을 수정했습니다
Copy link

@passwd10 passwd10 left a comment

Choose a reason for hiding this comment

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

먼저 올바르게 동작하는 완성된 형태의 어플리케이션을 만들고, 케이스가 하나씩 생길 때 마다 테스트를 추가해가면서 진행해보세요!

처음부터 너무 많은 케이스를 생각하고 진행하다보면 복잡도가 올라가서 구현이 힘들어질 수 있어요.

지금 당장 사용자에게 레스토랑 목록을 보여줘야한다면, 가장 먼저 무엇을 테스트해야할까요?


context('state가 있는경우', () => {
describe('getRegions', () => {
it('regions를 얻는다', () => {
Copy link

Choose a reason for hiding this comment

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

getRegions reducer는 store에 있는 regions 를 반환하는 일을 하고있어요.

Copy link
Author

Choose a reason for hiding this comment

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

수정했습니다

역할에 맞는 설명으로 regions를 return 한다 라는 표현이 괜찮을까요?!


dispatch(getCategories(categories));
};
}
Copy link

Choose a reason for hiding this comment

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

액션함수끼리 비동기 동작을 하는 액션함수 끼리 응집해서 작성하거나, 파일로 분리해서 관리하면 유지보수하기 편하지 않을까요? ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

수정했습니다

그러네요..! 저렇게 분리해주면 더 좋은거같습니다!!

});
});

context('state가 있는경우', () => {
Copy link

Choose a reason for hiding this comment

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

context를 적절하게 활용하셨어요 👍

Copy link
Author

Choose a reason for hiding this comment

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

헉 감사합니다! 😄😄

}

export async function fetchCategories() {
const url = 'https://eatgo-customer-api.ahastudio.com/categories';
Copy link

Choose a reason for hiding this comment

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

중복되는 url 이 보이네요!

Copy link
Author

Choose a reason for hiding this comment

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

수정했습니다

api 파일에 baseUrl 이라는 변수를 만들어 중복되는 부분을 제거해보았습니다!

api 호출시간동안 로딩을 표시하기위해 isLoading 상태를 추가해주었습니다
reducer.test 의 it 부분들을 역할에맞게끔 return 하는 설명으로 변경해주었습니다
actions 파일에 합쳐져있던 action들을 분리해주었습니다
baseUrl 이라는 변수를 만들어 중복을 제거해주었습니다
useDispatch , useSelector mock들을 추가해주었습니다
Regions 컴포넌트의 테스트 코드를 작성하였습니다
api 를 통해 받아온 지역 리스트를  렌더링 하는부분을 구현하였습니다
map 으로 돌린 부분에 key를 추가해주었습니다
app.test 코드를 작성하였습니다
app.test를 토대로 app을 구현하였습니다
app.test의 context 설명을 수정하였습니다
카테고리 목록을 보여주는 테스트 코드를 작성했습니다
카테고리를 보여주는 로직을 작성했습니다
api쪽 관련된 액션에 isloading제어권을 없애주었습니다
isLoading을 제어할수있는 테스트코드를 작성하였습니다
Copy link

@passwd10 passwd10 left a comment

Choose a reason for hiding this comment

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

happy path에 대한 테스트를 잘 작성하셨어요. 👍

만약 fetch한 categories가 undefined 인 경우엔 어떻게 해야할까요?

예외처리도 테스트를 통해 검증해보시고 어떻게하면 해결 할 수 있을지 고민해보시면 좋을 것 같아요!

Comment on lines 25 to 31
it('regions가 렌더링된다', () => {
const { getByText } = renderRegionsContainer();

fireEvent.click(getByText('서울'));

expect(dispathch).toBeCalled();
});
Copy link

Choose a reason for hiding this comment

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

작성하신 테스트 시나리오와 실제 확인하는 테스트가 다르네요!

Copy link
Author

Choose a reason for hiding this comment

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

그 부분은 현재 이렇게 바껴있습니다!!
image

src/Regions.jsx Outdated
@@ -3,7 +3,7 @@ export default function Regions({ regions, onClickSelectRegion }) {
<ul>
{regions.map((i) => (
Copy link

Choose a reason for hiding this comment

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

i 는 아무 의미없는 변수에요. 의미를 담은 네이밍을 고민해보세요!

Copy link
Author

Choose a reason for hiding this comment

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

region , regions 이렇게 변경하려했는데 다른곳에서 사용중인 네이밍이라 구조분해할당으로해서 { name, id }를 사용했는데 괜찮을까요?

@@ -10,7 +10,7 @@ export default function RegionsContainer() {
const { regions } = useSelector((state) => state);

const handleClickSelectRegion = (e) => {
dispatch(setRegion(e.target.value));
dispatch(setRegion(e.target.name));
Copy link

Choose a reason for hiding this comment

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

target의 name을 가져오는 이유를 알 수 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

button쪽에서 e.target.value를 하니 아무값도 안나오더라구요! 그래서 Input쪽에 name을 주고 e.target.name을 들고와 사용했습니다!


const renderRegions = () => render((
<Regions regions={regions} onClickSelectRegion={handleClickSelectRegion} />
const renderRegions = (paramRegion) => render((
Copy link

Choose a reason for hiding this comment

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

param 이라는 prefix 는 불필요해보여요 ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

region으로 네이밍을 주려했는데 다른곳에서 사용중이더라구요..! 이런 경우가 자주있는데 선생님께선 보통 어떤 네이밍을 주시나요?!

Comment on lines 7 to 10
beforeEach(() => {
jest.clearAllMocks();
});

Copy link

Choose a reason for hiding this comment

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

describe 하위에서 선언해주시면 테스트가 더 자연스럽게 읽힐 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

그러네요!! 앞으로는 describe안에 넣겠습니다!

map함수를 사용할때 구조분해할당을 사용했습니다
읽기편하게 describe 안에 넣어주었습니다
api 호출을 하는 각각의 액션에 따로 isLoading을 주어 관리하게끔 수정했습니다
카테고리를 선택하는 리듀서 테스트를 작성했습니다
action과 테스트변수 값을 추가했습니다
카테고리 선택하는 로직을 구현했습니다
테스트 데이터에 category를 추가했습니다
테스트를 작성했습니다
작성한 테스트를 토대로 구현했습니다
CategoriesContainer 추가로인한 수정
로직변경으로인한 컴포넌트 수정
레스토랑 리스트를 들고오는 테스트를 작성했습니다
fixtures - resturant - restaurants 테스트 데이터를 추가했습니다
레스토랑 리스트를 받아오는 액션을 추가했습니다
레스토랑 리스트를 가져오는 로직을 구현했습니다
category -> categoryId로 수정했습니다
category 를 e.target.name으로 저장해주었는데 id로 변경해주었습니다
categoryId로 변경해주었습니다
파라미터를받아서 fetchRestaurans에 넘겨주었습니다
로딩을 true로 만들어주는 로직을 구현했습니다
restaurants 추가로인한 테스트수정
@alstjr0183
Copy link
Author

안녕하세요 선생님..! 피드백주신 내용들 수정해보았습니다
마지막에 지역이랑 카테고리 선택했을때 레스토랑 리스트들 나오는 부분은 너무너무헷갈려서 구현부터 해버렸습니다 ㅠㅠ
resturants 테스트 코드는 좀더 고민후 커밋하겠습니다..!

jest.clearAllMocks();
});

const dispathch = jest.fn();
Copy link

Choose a reason for hiding this comment

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

오타가 있네요 ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

수정했습니다!

src/reducer.js Outdated
Comment on lines 3 to 5
regions: true,
categories: true,
restaurants: false,
Copy link

Choose a reason for hiding this comment

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

로딩 초기상태가 다 다른 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

regions랑 categories는 페이지가 렌더링되자마자 api호출을해야하는 부분이라서true로 했는데
restaurants는 지역이랑 카테고리가정해지면 불러와야해서 false로 했었습니다..!

Comment on lines +7 to +9
onClick={() => {
onClickSelectCategory(id);
}}
Copy link

Choose a reason for hiding this comment

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

저는 인라인에서 바로 함수를 작성하기보단, 핸들러 함수를 따로 선언해서 넣어주는 편이에요. 함수명만 읽고도 대략 어떤역할을 하는지 알 수 있어서 가독성이 올라가거든요!

민석님은 어떻게 생각하시나요? 정답이 있는건 아니라서 의견을 자유롭게 나눠보면 좋을 것 같아요ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

저도 pass님 처럼 생각하는데요! 저기서 id를 넘겨줘야하는데 onClick={onCLickSelectCategory} 이 형태로 id 넘겨줄수있는 방법이있나요?!

Copy link

@passwd10 passwd10 Jul 10, 2022

Choose a reason for hiding this comment

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

id 를 인자로 받는 핸들러 함수를 만들면 될 것 같아요 ㅎㅎ

currying 함수로 작성해보세요!

https://ko.javascript.info/currying-partials

dispatch 오타를 수정했습니다
@alstjr0183
Copy link
Author

안녕하세요! 선생님 한주동안 리뷰 감사합니다 ㅎㅎ
앞으로도 잘부탁드릴게요!!

@passwd10 passwd10 merged commit a617eba into CodeSoom:alstjr0183 Jul 12, 2022
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.

None yet

2 participants