test(vscode): getRepo 함수, 기존 URL 파싱 테스트 케이스에 타입/구조 검증 추가#849
Conversation
- 각 케이스마다 값과 owner/repo 타입을 함께 검증 - 반환 구조 명세를 위한 Contract 테스트 별도 추가
| expect(typeof result.owner).toBe("string"); | ||
| expect(typeof result.repo).toBe("string"); |
There was a problem hiding this comment.
(잘 모름)
음 저도 정확하게는 모르지만, 결국 ts test code도 transpile되서 js가 되고,
js의 type은 아래 처럼 7종류가 될껀데,
https://developer.mozilla.org/ko/docs/Web/JavaScript/Guide/Data_structures
이 중에서 저희가 string 말고 다른 type이 들어올 일이 있을까요? json string을 받아오는거라면
모두 string이 될게 명확한게 아닌걸까용?
There was a problem hiding this comment.
좋은 의견 감사합니다!
코드를 다시 확인해보니,
말씀해주신 대로 타입스크립트에서 이미 getRepo의 반환 타입이 문자열로 명확하게 보장되고 있어서,
각 케이스별 typeof 검증 테스트는 불필요하다고 판단하여 해당 부분을 삭제했습니다.
또한, 기존 문서화(Contract) 테스트 코드의 의도가 충분히 전달되지 않는 것 같아
#843 이슈와 같이 발생할 수 있는 문제를 예방하는 취지와 배경을 주석으로 명확히 남기도록 하겠습니다.
ytaek
left a comment
There was a problem hiding this comment.
리뷰 남겼습니다!!!
주석 처리 같은 경우는 unused comment 처리 정도의 이슈로 남겨놓고 다음 PR에서 해결하시면 어떨까요? 😸
| // [의도] 과거 repo[0] 등 부분 문자열 사용으로 인한 버그가 발생했던 이력이 있었습니다. | ||
| // repo는 전체 저장소명(string)으로 반환되며, 반환된 저장소명 그대로 사용해야 합니다. (부분 문자열이나 repo[0] 사용 금지) |
There was a problem hiding this comment.
음 가급적이면 주석은 영어로 적으면 좋을 것 같습니다.
그리고, 가능하다면 적지 않고 코드로 말하면 좋을 수 있구요!
(주석도 소스코드의 일부분이지요. 꼭 필요한 코드만 넣듯이 주석도 필요한 부분!!)
지금 처럼, 이전 에러 같은 경우에 꼭 필요하다고 하면
짧게 이슈 넘버를 활용하는 것도 방법입니다. // refer #323
아래 의도같은 경우에는 test이름에 충분히 넣을 수 있을 것 같습니다 😺
There was a problem hiding this comment.
네 알겠습니다. 추가 이슈 남겨서 처리해보겠습니다.
감사합니다~
|
설명해주신 과거 repo[0] 등 부분 문자열 사용으로 인한 버그 재발 방지 의도가 좋은 것 같습니다~👍
혹시 제가 파악못한 부분이 있다면 알려주시면 감사하겠습니다~! |
Related issue
Closes #848
#843
Result
Work list
Discussion
변경 배경 및 기대 효과