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
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
189 changes: 180 additions & 9 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion packages/vscode/.gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,5 @@ dist
node_modules
gh-pages/
.vscode-test/
out
out
src/test/utils/test-repo
10 changes: 10 additions & 0 deletions packages/vscode/jest.config.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import type { Config } from "@jest/types";

const config: Config.InitialOptions = {
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

};

export default config;
6 changes: 5 additions & 1 deletion packages/vscode/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,8 @@
"pretest": "npm run compile-tests && npm run compile && npm run lint",
"lint": "eslint src --ext ts",
"lint:fix": "eslint src --ext ts --fix",
"test": "node ./out/test/runTest.js"
"test:e2e": "node ./out/test/runTest.js",
"test:unit": "jest"
},
"dependencies": {
"@githru-vscode-ext/analysis-engine": "^0.7.0",
Expand All @@ -90,6 +91,7 @@
},
"devDependencies": {
"@types/glob": "^7.2.0",
"@types/jest": "^29.5.12",
"@types/mocha": "^9.1.1",
"@types/node": "14.x",
"@types/vscode": "^1.67.0",
Expand All @@ -107,8 +109,10 @@
"eslint-plugin-unused-imports": "^3.0.0",
"formdata-polyfill": "^4.0.10",
"glob": "^8.0.1",
"jest": "^29.7.0",
"mocha": "^9.2.2",
"prettier": "^3.0.1",
"ts-jest": "^29.2.5",
"ts-loader": "^9.2.8",
"typescript": "^4.6.4",
"webpack": "^5.70.0",
Expand Down
55 changes: 55 additions & 0 deletions packages/vscode/src/test/utils/getGitLog.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import * as cp from "child_process";
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 바깥으로 가는 경로인 걸로 착각했습니다 😭😭😭😭😭


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.

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

const testRepoPath = path.resolve(__dirname, "test-repo");

jest.setTimeout(6000 * 10);

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하게 되버리죠.


const deleteTestRepoDir = () => {
if (fs.existsSync(testRepoPath)) {
fs.rmSync(testRepoPath, { recursive: true, force: true });
}
};

beforeEach(() => {
deleteTestRepoDir();
cloneTestRepo();
removeFilesExceptGit();
});

afterEach(() => {
deleteTestRepoDir();
});

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 안에 실행되어야 한다 등의 테스트 시나리오가 있다면 활용하는것이 맞지요)

const result = await getGitLog("git", testRepoPath);
console.timeEnd("getGitLog Execution Time");

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로 수정하는 게 좋을까요?

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

});
});
2 changes: 1 addition & 1 deletion packages/vscode/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -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에 있는 경우가 많아서요. 다른 분들도 안 헷갈리실듯?

"strict": true,
"allowSyntheticDefaultImports": true,
"skipLibCheck": true,
Expand Down
Loading