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

feature/router 라우터 기능 수정 및 테스트 코드 수정 #3

Merged
merged 5 commits into from
Jan 15, 2024

Conversation

devbit4
Copy link
Member

@devbit4 devbit4 commented Jan 12, 2024

  • router 클래스로 변경 및 테스트코드 수정
  • articleList.json 데이터 파일 추가 및 페이지 api 연동 테스트

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions

C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

idea Catch issues before they fail your Quality Gate with our IDE extension SonarLint SonarLint

}

getArticle() {
fetch('/dbs/articleList.json')

Choose a reason for hiding this comment

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

data fetching하는 부분을 함수로 분리하여 다음과 같은 형태로 쓸 수 있도록 만들어보면 좋을 것 같네요.

const article = await getArticle(this.props.params.id)
this.setState({ ...this.state, article })

});
}

renderArticleList() {

Choose a reason for hiding this comment

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

Component의 render가 아닌 새로운 함수를 선언해서 렌더링하는 이유가 무엇인가요?

},
{ path: '*', page: jest.fn().mockReturnValue('<div>404-page</div>') },
];
class Home extends Component {
Copy link

@f-lab-dennis f-lab-dennis Jan 13, 2024

Choose a reason for hiding this comment

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

테스트를 위한 페이지 컴포너트를 이용하여 테스트하는 것도 필요하지만 실제 페이지 컴포넌트를 import하여 테스트하는 코드도 필요할 것 같습니다.

const router = new Router(target);

beforeAll(() => {
router

Choose a reason for hiding this comment

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

test가 돌 때마다 공통으로 초기화 해주어야 하는 부분인지 확인하고 사용하면 좋을 것 같습니다.

this.target = target;
}

addRoute(route) {

Choose a reason for hiding this comment

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

체이닝을 할 수 있는 형태로 만든건 좋네요!

@f-lab-dennis
Copy link

f-lab-dennis commented Jan 13, 2024

Router 모듈과 컴포넌트 부분은 클래스를 사용하면서 이전보다 확실히 나아졌네요. 수고많으셨습니다.
그런데 하나의 test에 여러 개의 expect를 두면 특정 expect만 실패했을 때 파악이 어려워지기 때문에 되도록이면 하나의 test에 하나의 expect를 사용하는 것을 권장하고 describe로 묶는 것이 좋을 것 같습니다.
그리고 PR 내용을 조금 더 상세하게 작성해주시면 좋을 것 같습니다. 어디까지 구현을 하였고 구현하면서 어떤 고민들을 PR에 정리해주시면 좋을 것 같습니다.

@devbit4 devbit4 requested a review from f-lab-dennis January 13, 2024 09:14
@devbit4 devbit4 merged commit 68dba6e into main Jan 15, 2024
1 of 2 checks passed
@devbit4 devbit4 deleted the feature/router branch January 15, 2024 16:46
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