[view] tag와 releaseTag에 대한 테스트 코드 추가#827
Conversation
2Estella
left a comment
There was a problem hiding this comment.
안녕하세요, 여은님!
Summary.util과 Detail.util의 테스트 코드 개선사항 잘 확인했습니다. 기존 기능에 맞춰 일관성 있게 테스트 커버리지를 강화해주셔서 코드의 안정성이 훨씬 높아졌네요. 훌륭한 작업 정말 감사합니다! 👍
| test("Get the latest release tag from the cluster", () => { | ||
| const clusters = getInitData(clusterNodeMockData); | ||
| expect(getCommitLatestTag(clusters[2].clusterTags)).toBe("v1.2.0"); |
There was a problem hiding this comment.
이 테스트는 잘 동작하지만 getCommitLatestTag 함수가 'v1.10.0'과 'v1.2.0' 같은 버전을 문자열로 비교할 경우 오류를 낼 수 있는 위험성이 보여요.
semver 같은 라이브러리를 사용해서 버전을 비교하면 더 견고한 함수가 되지 않을까요? 아니면 의존성을 높이지 않기 위해 간단한 비교 함수를 만드는 것도 좋은 방법일 것 같아요 :)
There was a problem hiding this comment.
이 테스트는 잘 동작하지만 getCommitLatestTag 함수가 'v1.10.0'과 'v1.2.0' 같은 버전을 문자열로 비교할 경우 오류를 낼 수 있는 위험성이 보여요. semver 같은 라이브러리를 사용해서 버전을 비교하면 더 견고한 함수가 되지 않을까요? 아니면 의존성을 높이지 않기 위해 간단한 비교 함수를 만드는 것도 좋은 방법일 것 같아요 :)
semver 라이브러리가 있었군요 : ) 좋은 것 알아갑니다!
| expect(result1[0].commit.tags).toBe(fakeCommitNodeListInCluster[0].commit.tags); | ||
| expect(result1[0].commit.releaseTags).toBe(fakeCommitNodeListInCluster[0].commit.releaseTags); | ||
| expect(result1[1].commit.tags).toBe(fakeCommitNodeListInCluster[1].commit.tags); | ||
| expect(result1[1].commit.releaseTags).toBe(fakeCommitNodeListInCluster[1].commit.releaseTags); | ||
| expect(result1[2].commit.tags).toBe(fakeCommitNodeListInCluster[2].commit.tags); | ||
| expect(result1[2].commit.releaseTags).toBe(fakeCommitNodeListInCluster[2].commit.releaseTags); |
There was a problem hiding this comment.
Jest에서 배열이나 객체의 내용을 비교할 때는 toBe 대신 toEqual을 사용하는 것이 더 안전해요.
toBe는 메모리 주소까지 같은지를 확인(엄격한 비교)해서, 나중에 함수 구현이 바뀌면 예기치 않게 테스트가 실패할 수 있거든요! toEqual로 바꿔주시면 더 안정적인 테스트가 될 것 같습니다!
There was a problem hiding this comment.
Jest에서 배열이나 객체의 내용을 비교할 때는 toBe 대신 toEqual을 사용하는 것이 더 안전해요. toBe는 메모리 주소까지 같은지를 확인(엄격한 비교)해서, 나중에 함수 구현이 바뀌면 예기치 않게 테스트가 실패할 수 있거든요! toEqual로 바꿔주시면 더 안정적인 테스트가 될 것 같습니다!
해당 내용 넘 유익한 것 같습니다!!!
다른 test case 파일 내용에서도 저런 부분이 있을 것 같은데,
@ongheong 님, 요 내용 이슈로 새로 등록하신 다음에
repository 전체로 한번 검토해봐주시면 어떨까요?
There was a problem hiding this comment.
여기서 특히 getInitData 테스트 정말 꼼꼼하게 작성해주셨네요!
단순히 동작만 확인한 게 아니라 clusterTags가 없는 경우부터 하나만 있는 경우, 여러 커밋에 걸친 경우, 중복된 경우 등 다양한 엣지 케이스까지 챙겨주신 덕분에 기능 안정성이 훨씬 올라갈 거 같아요. 세심한 작업 최고👍
There was a problem hiding this comment.
안녕하세요.
테스트 이름이 구체적이고 명확해서 테스트의 의도를 쉽게 이해할 수 있었고,
태그 관련 다양한 케이스를 잘 커버해주셔서 전반적으로 좋은 테스트 코드라고 생각했습니다!
제가 생각해 본 개선 포인트로는, clusters = getInitData(clusterNodeMockData); 구문이 반복되고 있는 점이 있었습니다.
이 부분은 describe 블록 내부에서 beforeAll()을 사용해 중복을 제거하면 어떨까요?
`describe("getCommitLatestTag test", () => {
let clusters;
beforeAll(() => {
clusters = getInitData(clusterNodeMockData);
});
test("should return empty string when clusterTags is empty", () => {
expect(getCommitLatestTag(clusters[0].clusterTags)).toBe("");
});
test("should return the latest tag (semver)", () => {
expect(getCommitLatestTag(clusters[2].clusterTags)).toBe("v1.2.0");
});
});`
| expect(result1[0].commit.tags).toBe(fakeCommitNodeListInCluster[0].commit.tags); | ||
| expect(result1[0].commit.releaseTags).toBe(fakeCommitNodeListInCluster[0].commit.releaseTags); | ||
| expect(result1[1].commit.tags).toBe(fakeCommitNodeListInCluster[1].commit.tags); | ||
| expect(result1[1].commit.releaseTags).toBe(fakeCommitNodeListInCluster[1].commit.releaseTags); | ||
| expect(result1[2].commit.tags).toBe(fakeCommitNodeListInCluster[2].commit.tags); | ||
| expect(result1[2].commit.releaseTags).toBe(fakeCommitNodeListInCluster[2].commit.releaseTags); |
There was a problem hiding this comment.
Jest에서 배열이나 객체의 내용을 비교할 때는 toBe 대신 toEqual을 사용하는 것이 더 안전해요. toBe는 메모리 주소까지 같은지를 확인(엄격한 비교)해서, 나중에 함수 구현이 바뀌면 예기치 않게 테스트가 실패할 수 있거든요! toEqual로 바꿔주시면 더 안정적인 테스트가 될 것 같습니다!
해당 내용 넘 유익한 것 같습니다!!!
다른 test case 파일 내용에서도 저런 부분이 있을 것 같은데,
@ongheong 님, 요 내용 이슈로 새로 등록하신 다음에
repository 전체로 한번 검토해봐주시면 어떨까요?
ethandeveloper2
left a comment
There was a problem hiding this comment.
LGTM!
저도 테스트 코드로 Githru에 기여하려고 준비 중인데,
이번 작업이 많은 도움이 되었습니다.
의미 있는 테스트 코드 공유해주셔서 감사합니다!
There was a problem hiding this comment.
안녕하세요.
테스트 이름이 구체적이고 명확해서 테스트의 의도를 쉽게 이해할 수 있었고,
태그 관련 다양한 케이스를 잘 커버해주셔서 전반적으로 좋은 테스트 코드라고 생각했습니다!
제가 생각해 본 개선 포인트로는, clusters = getInitData(clusterNodeMockData); 구문이 반복되고 있는 점이 있었습니다.
이 부분은 describe 블록 내부에서 beforeAll()을 사용해 중복을 제거하면 어떨까요?
`describe("getCommitLatestTag test", () => {
let clusters;
beforeAll(() => {
clusters = getInitData(clusterNodeMockData);
});
test("should return empty string when clusterTags is empty", () => {
expect(getCommitLatestTag(clusters[0].clusterTags)).toBe("");
});
test("should return the latest tag (semver)", () => {
expect(getCommitLatestTag(clusters[2].clusterTags)).toBe("v1.2.0");
});
});`
|
@ethandeveloper2 님, core test 쪽에서도 |
네 이 PR이 머지되면 beforeAll()이나 toEqual() 이슈 등록해서 진행하도록 하겠습니다. @ongheong @ytaek |
Related issue
#725
Result
Work list
getCommitListDetail의tagLength,releaseTagLength계산을 추가했습니다.테스트 시나리오
getCommitListDetail
getInitData
getCommitLatestTag
Discussion