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

[vscode/engine] getGitLog 함수 관련 테스트 코드 구현 #689

Merged
merged 19 commits into from
Oct 6, 2024

Conversation

novice1993
Copy link
Contributor

Related issue

#681

Result

getGitLog_Test.mp4

Work list

  • vscode/src/utils/git.tils.ts에 선언된 getGitLog 함수를 테스트 하는 코드입니다
  • test repository로 설정한 원격 저장소에서 git clone을 받은 후, 해당 git 데이터를 활용하여 테스트를 진행합니다
  • test repositoryDjango로 설정하였습니다 (커밋 개수 30,000개 이상으로, 성능 측정에 적합한 케이스라고 판단)
  • 테스트 진행 전/후로 test repository를 제거하여 패키지에 해당 파일이 남지 않도록 처리하였습니다
    (.gitignoretest repository를 추가하여 오류로 test repository가 제거되지 않아도 문제가 발생하지 않도록 설정)
  • console.time/console.timeEnd 메서드를 활용하여 getGitLog 함수의 처리 시간을 측정할 수 있도록 구현하였습니다
    (타이머 라벨명 : getGitLog Execution Time)

Discussion

  • vscode 패키지의 package.json에 본래 설정되어 있던 test 스크립트가 있으나 확인 결과 해당 코드 주석 처리 되어 비활성화 된 상태 ("node ./out/test/runTest.js")
  • 기록된 주석을 확인해보니 통합 테스트 목적의 코드지만, 현재 사용하지 않는 것으로 추정 ( " ~ Download VS Code, unzip it and run the integration test ~ ")
  • 테스트를 구별하기 위해 기존의 test 스크립트를 test:e2e로 수정하고 unit 테스트 관련 스크립트를 test:unit로 추가
  • 이렇게 처리하는 게 적절한지 명확하지 않아 논의 필요

kimyh93 and others added 8 commits September 2, 2024 14:23
Desc
- mocking 함수 작성하여 간단한 테스트 코드 구현
- 실제 데이터 처리를 테스트 하기 위해서 mocking 함수 보단 실제 child_process를 사용하는 것이 낫다고 판단하여 로직 수정할 예정
Desc:
- 커밋 개수 고려하여 테스트용 git 데이터 추가 (nginx, 커밋 개수 약 8,000개)
- git 충돌을 고려하여 압축 파일로 관리
Desc
- 지정한 ssh 를 활용하여 clone 받은 후 테스트에 활용하도록 로직 수정
- 테스트 수행 후  clone 받은 파일 제거
Desc
- 테스트 실행 시 설정해놓은 test repo에서 clone 받은 뒤, 해당 git 데이터 활용하여 테스트 진행
- test repo로 django 설정 (commit 개수 30,000개 이상으로 성능 테스트에 적합하다고 판단)
- 테스트 실행 전/후로 clone 받은 디렉토리 제거하여 패키지에 남지 않도록 설정
feat: getGitLog 관련 테스트 코드 구현
Desc
- ts-jest 관련 버전 수정
Copy link
Contributor

@ytaek ytaek left a comment

Choose a reason for hiding this comment

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

테스트 코드 완전 환영합니다 😄
이번 기회에 vscode쪽의 test code도 좀 많아졌으면 좋겠네요!!

리뷰 커멘트 중에서, 아래 2가지는 전체에 영향을 끼치는 부분이 커서 수정해서 올리면 좋을 것 같습니다 : )

  • engine 모듈 import 부분
  • mock 데이터 만드는 부분

@@ -5,7 +5,7 @@
"outDir": "dist",
"lib": ["ES2020"],
"sourceMap": true,
"rootDir": "src",
"rootDir": "./",
Copy link
Contributor

Choose a reason for hiding this comment

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

여길 변경하신 이유가 따로 있을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ytaek

확인이 늦어서 죄송합니다ㅠㅠ
기존에는 .ts 확장자를 가진 파일이 src 디렉토리 내부에만 존재했는데,
jest를 적용하면서 tsconfig.json과 같은 경로에 jest.config.ts 파일이 생성되어서
타입스크립트 관련 config 적용 경로를 변경하게 되었습니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

앗. 제가 잘 이해가 가지 않아서 다시 여쭤봅니다~.
engine의 경우에는 rootDir을 기존처럼 src로 둬도 잘 수행됐던거 같은데,
현재는 ./로 수정되지 않으면 안 돌아가는 상황인가요?

https://github.com/githru/githru-vscode-ext/blob/main/packages/analysis-engine/jest.config.ts
https://github.com/githru/githru-vscode-ext/blob/main/packages/analysis-engine/tsconfig.json

Copy link
Contributor Author

@novice1993 novice1993 Sep 10, 2024

Choose a reason for hiding this comment

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

@ytaek

저도 기존에 설정되어있던 경로를 수정하는 부분은 최대한 지양하는 것이 좋을 것 같아서
engine 패키지를 참고했었는데,

왜 그런지는 저도 정확히 모르겠지만
vscode 패키지에서 tsconfig.json의 rootdir을 engine과 동일하게 수정할 경우 관련 알림이 발생하는 상황입니다
(engine 패키지에서는 발생하지 않습니다)

engine과 동일하게 rootdir을 ./src로 적용해도 돌아가기는 하는 상황입니다!�

스크린샷 2024-09-10 오후 9 30 03

Copy link
Contributor

Choose a reason for hiding this comment

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

그러면 engine과 동일하게 ./src 로 적용하시면 어떨까요? : )
보통 code들은 모두 src에 있는 경우가 많아서요. 다른 분들도 안 헷갈리실듯?

import * as fs from "fs";
import * as path from "path";

import { getGitLog } from "../../utils/git.util";
Copy link
Contributor

Choose a reason for hiding this comment

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

앗. 이 부분은 바로 상대경로를 쓰는게 아니라, engine pkg lib를 import 해서 쓰셔야 해요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

�넵 해당 부분 수정하도록 하겠습니다!

Copy link
Contributor Author

@novice1993 novice1993 Sep 14, 2024

Choose a reason for hiding this comment

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

@ytaek

영택님 코멘트 주셨던 부분 중 아래의 2가지는 수정하여 반영하였는데

  1. mock 데이터 활용 방식으로 test 코드 수정
  2. jest.config.ts 파일 수정 (test가 특정 디렉토리에 한정되지 않도록 전체 경로로 변경)

해당 이슈 (engine 모듈 import 수정) 는 어떻게 해결해야할지 잘 모르습니다..!

getGitLog 함수를 "@githru-vscode-ext/analysis-engine"에서 import 하려고 하니,
해당 패키지에서 export 하지 않는다고 나오는 상황입니다!

getGitLog 함수는 vscode/src/utils/git.util.ts 파일에서 선언된 함수라서 상대 경로로 import 하는 게 맞지 않나 생각이 되는데
(https://github.com/githru/githru-vscode-ext/blob/main/packages/vscode/src/utils/git.util.ts)

혹시 제가 잘못 이해하고 있는 거라면 어떻게 수정하는게 좋을지 코멘트 부탁드립니다..!

Copy link
Contributor

Choose a reason for hiding this comment

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

헉... 제가 상대경로 위치를 잘 못 봣었네요 ㅜ.ㅜ 죄송합니당.
../../ 부분이 package 바깥으로 가는 경로인 걸로 착각했습니다 😭😭😭😭😭


import { getGitLog } from "../../utils/git.util";

const TEST_REPO_SSH = "[email protected]:django/django.git"; // Sample repository: Django (https://github.com/django/django), with over 30,000 commits
Copy link
Contributor

@ytaek ytaek Sep 4, 2024

Choose a reason for hiding this comment

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

test에 쓰이는 데이터는 동적인 데이터 말고, 정적인 데이터를 박아놓는게 좋습니다. (그럴일이 잘 없겠지만) 장고 repo가 이름이 바뀌거나 없어질 수도 있구요.

https://learn.microsoft.com/ko-kr/dotnet/core/testing/unit-testing-best-practices#stub-static-references

Copy link
Contributor Author

Choose a reason for hiding this comment

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

앗 그런데 해당 링크가 안열려서 (저만 그런 걸지도 모르겠지만..?)
혹시 어떤 내용일까요..?!

Copy link
Contributor

Choose a reason for hiding this comment

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

링크가 이상한게 달렸었네요 ㅜ.ㅜ 수정했습니다!

Comment on lines 12 to 25
const cloneTestRepo = () => {
cp.execSync(`git clone ${TEST_REPO_SSH} ${testRepoPath}`, { stdio: "inherit" });
};

const removeFilesExceptGit = () => {
if (fs.existsSync(testRepoPath)) {
fs.readdirSync(testRepoPath).forEach((file) => {
const fullPath = path.join(testRepoPath, file);
if (file !== ".git") {
fs.rmSync(fullPath, { recursive: true, force: true });
}
});
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

(위와 같은 이유로) 매번 pull 해서 사용하는 로직이 테스트에서 실행되는데,
그냥 mock이나 fake data 를 정적으로 만들어놓는 것이 더 좋은 practice 입니다 😄

이런 경우, 만약에 network이 안되거나 git에 이상이 있을 경우에는,
현재 저희 코드와는 아무 관계없는 이유 때문에 test가 fail하게 되버리죠.


describe("getGitLog util test", () => {
it("return git log output", async () => {
console.time("getGitLog Execution Time");
Copy link
Contributor

Choose a reason for hiding this comment

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

오 .time() 이란게 있었는지 몰랐군요 : ) 감사합니다.

단, test에서 현재 validation을 하는 코드가 아니라면,
불필요한 코드는 사실 삭제하는 것이 맞긴 합니다 : )

(만약에 해당 test가 10s 안에 실행되어야 한다 등의 테스트 시나리오가 있다면 활용하는것이 맞지요)

Comment on lines 49 to 53
expect(result).toContain("commit");
expect(result).toContain("Author");
expect(result).toContain("AuthorDate");
expect(result).toContain("Commit");
expect(result).toContain("CommitDate");
Copy link
Contributor

Choose a reason for hiding this comment

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

테스트 코드는 언제나 환영입니다. :)

테스트 코드의 목적이 해당 expect에 적힌 정도를 테스트 하고자 함이었다면
적당히 Mock 객체를 만들어서 테스트 하는 것도 괜찮을 것 같습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ytaek

리뷰 주신 부분 수정해서 반영하도록 하겠습니다!

추가적으로 한가지 더 문의 드릴 부분이 있는데,
현재 CI 에러가 나는 원인이 vscode 패캐지의 package.json 스크립트에서 test 명령어를 찾을 수 없기 때문인 것 같은데
(아래에 에러 메세지 첨부)

  • npm error location /home/runner/work/githru-vscode-ext/githru-vscode-ext/packages/vscode
    npm error Missing script: "test"

discussion에 말씀드린 것처럼,
기존에 test 스크립트가 존재하긴 했지만 관련 코드가 작동하지 않는 상황인데 (주석처리 되어있음)
기존에 존재하던 코드/파일을 제거 하기에는 좀 애매해서

기존 test 스크립트를 test:e2e로 수정하고,
jest를 적용한 스크립트를 test:unit으로 새롭게 추가한 상황입니다.

CI에러를 해결하기 위해서는 test 스크립트를 다시 지정해줘야 할 것 같은데,
test:e2e로 지정한 스크립트를 제거하고, test:unit으로 생성한 스크립트를 test로 수정하는 게 좋을까요?

Copy link
Contributor

Choose a reason for hiding this comment

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

CI에러를 해결하기 위해서는 test 스크립트를 다시 지정해줘야 할 것 같은데, test:e2e로 지정한 스크립트를 제거하고, test:unit으로 생성한 스크립트를 test로 수정하는 게 좋을까요?

넵 그렇게 하시면 되겠습니다.

preset: "ts-jest",
testEnvironment: "node",
verbose: true,
rootDir: "./src/test/utils",
Copy link
Contributor

Choose a reason for hiding this comment

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

test/util 만 test하는게 아니라, 전체 or src폴더를 지정해야 하는게 아닐까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ytaek

해당 코드 관련해서는 기존에 vscode 패키지에 존재하던 test 관련 파일을 고려한 것인데

(vscode/src/test/suite/extension.test.ts)
https://github.com/githru/githru-vscode-ext/blob/main/packages/vscode/src/test/suite/extension.test.ts

해당 파일이 이전에 생성된 이후 현재는 활용되지 않는 파일로 생각되어
일단 금번 PR에 생성한 파일만 test에 적용되도록 test/util로 범위를 한정한 상황입니다

테스트 경로를 전체로 수정해주는 것이 좋을까요?

Copy link
Contributor

@ytaek ytaek Sep 10, 2024

Choose a reason for hiding this comment

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

테스트 경로를 전체로 수정해주는 것이 좋을까요?

넵. 곧 다른 테스트가 늘어날꺼니까요!!
어차피 폴더 안을 다 뒤져서 있는 tc 를 다 실행하기 때문에,
보통은(특별한 이유가 없는 이상) 저렇게 rootDir을 특정 폴더로 지정하지 않습니다 :-)

이 설정 파일이 vscode 패키지 전체에 대한 설정임을 고려해서 생각해보시면 좋을 것 같습니다 😸
SW란게 항상 수정이 덜 되는 방향으로 움직이는 거라고 생각한다면,
좀 오버하자면 OCP에 해당하는 케이스라고 보면 되겠네요 : )
https://ko.wikipedia.org/wiki/%EA%B0%9C%EB%B0%A9-%ED%8F%90%EC%87%84_%EC%9B%90%EC%B9%99

Desc
- 테스트 적용 범위 수정
- 기존 : src/util
- 수정 : 전체 경로
Desc
- test 관련 스크립트 수정 (test:e2e, test:unit 스크립트  제거 후 test 스크립트 설정)
Desc
- mock 데이터 및 함수를 설정하여 테스트를 수행하도록 수정 (기존에는 직접 git clone 받은 후 테스트 하도록 구현됨)
Desc
- out 디렉토리는 테스트에서 제외되도록 설정
- 제외 사유 : .ts 파일이 .js로 트랜스파일링 되는 과정에서 jest 파일의 import 구문을 인식하지 못하여 오류가 발생하므로 테스트에서 제외하도록 설정
Desc
- 이전에 생성 후 활용하지 않는 테스트 파일 존재 (extension.test.ts)
- 해당 테스트 파일로 인해 테스트 실패가 발생하므로, 확장자를 .test.ts에서 .ts로 변경하여 테스트에서 제외
Desc
- 변경 전 테스트 케이스에서 필요했던 코드 제거 (clone 받았던 git repo 디렉토리  관련 .gitignore 코드)
Copy link
Contributor

@ytaek ytaek left a comment

Choose a reason for hiding this comment

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

리뷰 커멘트 남겼습니다.
제가 잘못 말씀 드린 부분이 있어서 수정했고,
새로 넣은 커멘트 하나만 반영해주시면 되겠습니다.

it("should return the correct git log output", async () => {
const result = await getGitLog("git", "/mocked/path/to/repo");

expect(result).toContain("commit 1234567890abcdef1234567890abcdef12345678");
Copy link
Contributor

Choose a reason for hiding this comment

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

해당 결과가 순차적으로 나오는 걸까요?
그렇다면 해당 값들도 하나의 변수로 만들어주면 좋겠습니다.

toContain을 하면 기대하던 값이 그대로 나오는게 아니라, 포함만 되면 되는걸로 보이거든요.

cf.) 참고로 이렇게 통으로 결과값을 비교하는 테스트들을 golden master testing이라고 합니다.
https://medium.com/musinsa-tech/%EB%A6%AC%ED%8C%A9%ED%86%A0%EB%A7%81%EC%9D%84-%EC%9C%84%ED%95%9C-%ED%86%B5%ED%95%A9-%ED%85%8C%EC%8A%A4%ED%8A%B8-cd23498918a7

mac added 2 commits October 3, 2024 17:39
Desc
- rootDir 경로 변경 (./src)
- 관련하여 오류 제거하기 위하여 jest.config.ts 파일 확장자를 mjs로 변경
Desc
- contain -> equal로 테스트 성격 보다 더 정확하게 비교하도록 수정
@novice1993
Copy link
Contributor Author

@ytaek

리뷰 해주신 내용 두 가지

  1. tsconfig.json 설정 변경 (rootdir : ./src 로 변경)
  2. 테스트 관련 수정 (contain이 아닌 equal 비교로 수정)

반영하여 업데이트 했습니다. 검토 부탁드립니다!

Copy link
Contributor

@yoouyeon yoouyeon left a comment

Choose a reason for hiding this comment

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

고생하셨습니다 😁👍✨
child_process를 모킹하는 것은 생각하지 못했던 방법이네요...!!! 😲😲😲
몇가지 질문이 있어서 comment 남겼는데 확인해봐주시면 감사하겠습니다~! 😊

Comment on lines 96 to 105
stdout: {
on: jest.fn((event, callback) => {
if (event === "data") {
callback(Buffer.from(mockGitLogData));
}
if (event === "close") {
callback();
}
}),
},
Copy link
Contributor

Choose a reason for hiding this comment

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

(질문) 이렇게 하면 getGitLog에서 child process를 생성했을 때 git log를 실행하는 child process 대신 mockGitLogData를 출력하는 child process가 생성되는 것인가요???

Copy link
Contributor Author

Choose a reason for hiding this comment

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

네 맞습니다!

실제로 spawn 메서드를 통해 git log 명령어를 실행하려면 이를 위한 git 파일이 필요해서,
처음에는 github의 django 레포지토리를 clone 받아서 git log 명령어를 직접 실행하는 방식으로 구현했는데

이렇게 테스트 할 경우 getGitLog 함수 자체의 결함이 아닌 외부 요인들 (ex. 네트워크 오류, 해당 레포지토리 삭제 등) 로 인해 테스트가 실패할 수 있다는 피드백을 주셔서
spawn에 대한 mocking 함수를 설정하고, getGitLog 함수에서 spawn이 실행될 경우 mocking 함수가 실행되도록 설정했습니다.

mocking 함수의 동작은 사전에 정의한 mock 데이터를 반환하는 것으로 설정했는데
이렇게 설정한 이유는 spawn 메서드를 통해 수행하는 동작이 결국 git log ~ 명령어들을 실행해서 log 데이터를 산출하는 것이라서 해당 양식을 가진 mock 데이터를 반환하는 것으로 설정했습니다!

Comment on lines 120 to 123
it("should return the correct git log output", async () => {
const result = await getGitLog("git", "/mocked/path/to/repo");
return expect(result).toEqual(mockGitLogData);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

(질문)
테스트 이름에는 'correct git log output' 이라는 문구가 있어서 getGitLog 내에서 실행하는 git log 명령의 출력 결과가 잘 나오는지를 확인하는 테스트라고 생각했는데,
테스트 내용은 git log와는 관계없이 child process의 출력을 잘 받아서 return하는지를 확인하는 테스트 같아 보여서요...!
제가 테스트코드를 잘 이해한 것이 맞는지,, 어떤 쪽을 의도하신 것인지 궁금합니다!!

(만약에 후자를 의도하신 것이라면 테스트 이름을 "should return the correct child process output" 이런 식으로 변경해도 좋을 것 같아요...ㅎㅎ)

Copy link
Contributor Author

@novice1993 novice1993 Oct 4, 2024

Choose a reason for hiding this comment

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

오 맞습니다..!
사실 getGitLog 함수의 동작이 git log 관련 데이터를 산출하는 간단한 동작이라 테스트 자체도 조금 모호한 면이 있는 것 같습니다 ㅇㅅㅇ

(+) 댓글로 문의주신 내용 바탕으로 더 고민해서 테스트 코드를 수정했는데,
만약 spawn 메서드가 getGitLog 내부에서 여러번 실행 될 경우 (멀티 쓰레드 등을 활용한다고 가정했을 때)
각 spawn 마다 다른 mock 데이터를 부여하고 최종적으로 이를 합산한 데이터를 반환하는지 테스트 하는 방식으로 내용을 조금 더 추가해봤습니다!

현재는 싱글 쓰레드를 활용 중이라 크게 다른 점이 없을 것 같은데,
engine 팀에서 테스트 하고 있는 부분 (멀티 쓰레드로 getGitLog 효율화) 이 구현되어서 반영된다면
해당 동작이 정확히 이루어지는지 테스트 해볼 수 있을 것 같습니다..!

Copy link
Contributor Author

@novice1993 novice1993 Oct 4, 2024

Choose a reason for hiding this comment

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

@ytaek

영택님 위 댓글에 어느 정도 설명을 첨부하긴 했는데
추후 멀티 쓰레드를 활용하는 방식으로 getGitLog 함수가 개선되었을 때 이를 테스트 할 수 있도록 테스트 코드를 수정해봤습니다. 관련해서 피드백 부탁드립니다!

kimyh93 added 2 commits October 5, 2024 00:52
Desc
- spawn 메서드가 호출될 때마다 새로운 mock 데이터를 생성 후 반환하도록 하고, 최종적으로 이를 합산한 문자열을 반환하는지 테스트
Copy link
Contributor

@yoouyeon yoouyeon left a comment

Choose a reason for hiding this comment

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

LGTM!! ✨✨✨✨ 고생하셨습니다 🔥💪

mockSpawnCallCount = 0; // initailize call count
});

it("should return the combined git log output from number of threads", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

😊👍

@novice1993 novice1993 merged commit 9f68406 into githru:main Oct 6, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants