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

new-test-environment #24

Merged
merged 2 commits into from
Feb 27, 2023
Merged

new-test-environment #24

merged 2 commits into from
Feb 27, 2023

Conversation

jmnote
Copy link
Contributor

@jmnote jmnote commented Feb 26, 2023

What this PR does / why we need it (변경 내용 / 필요성):

최근 merge된 코드로 인해 테스트를 통과 못하게 되었습니다.
그래서 테스트 환경(go test)을 정비하고, 모든 테스트를 통과하도록 수정하였습니다.
되도록 기능 변경이 없이 하고자 하였지만, 불가피하게 변경된 부분은 있습니다.

  1. 로그파일
    테스트환경에서는 작업 디렉토리는 프로젝트 루트 디렉토리가 되며,
    로그디렉토리는 그것을 기준으로 ./tmp/log가 됩니다.
    ./testutil/log에 테스트 환경용 로그 파일들이 준비되어 있습니다.
    testutil를 이용하여 ./testutil/log를 ./tmp/log로 다시 복사함으로써 테스트 환경을 초기화할 수 있습니다.

  2. 현재시각
    아시다시피 lethe는 시간을 기준으로 동작하는 기능이 많기 때문에
    clock이라는 패키지를 추가하여 GetNow() 메소드를 통해 현재시각을 받도록 하였습니다.
    런 환경(go run)에서는 실제 현재시각이 나오게 되고,
    테스트 환경에서는 2009-11-10 23:00:00 UTC가 나옵니다.
    이것은 Go Playground 사례를 참고하여 정했습니다.

  3. DeleteByAge, DeleteBySize 테스트케이스 세분화

  • DeleteByAge 테스트: retention.time = [10d, 1d, 1h]인 경우
  • DeleteBySize 테스트: retention.size = [1m, 50k, 1k]인 경우
  1. DeleteByAge, DeleteBySize dryRun 모드 삭제
    실제 삭제는 하지 않고, 삭제대상만 출력하는 dryRun 모드가 있었는데, 테스트 환경을 정비하고 나니 필요 없는 것 같아서 제거했습니다.

모든 점검 통과

root@wsl:~/go/src/github.com/kuoss/lethe# make check
go fmt ./...
go vet ./...
# go install honnef.co/go/tools/cmd/staticcheck@latest
/root/go/bin/staticcheck ./...
go test ./... -failfast -cover
ok      github.com/kuoss/lethe  0.012s  coverage: 66.7% of statements
ok      github.com/kuoss/lethe/cli      0.058s  coverage: 0.0% of statements
?       github.com/kuoss/lethe/cli/cmd  [no test files]
?       github.com/kuoss/lethe/cli/cmd/list     [no test files]
?       github.com/kuoss/lethe/cli/cmd/task     [no test files]
?       github.com/kuoss/lethe/cli/util [no test files]
ok      github.com/kuoss/lethe/clock    (cached)        coverage: 100.0% of statements
ok      github.com/kuoss/lethe/config   (cached)        coverage: 0.0% of statements
?       github.com/kuoss/lethe/handlers [no test files]
ok      github.com/kuoss/lethe/letheql  (cached)        coverage: 73.0% of statements
ok      github.com/kuoss/lethe/logs     0.125s  coverage: 41.1% of statements
?       github.com/kuoss/lethe/storage  [no test files]
?       github.com/kuoss/lethe/storage/driver   [no test files]
?       github.com/kuoss/lethe/storage/driver/factory   [no test files]
ok      github.com/kuoss/lethe/storage/driver/filesystem        (cached)        coverage: 10.4% of statements
ok      github.com/kuoss/lethe/testutil (cached)        coverage: 73.6% of statements
?       github.com/kuoss/lethe/util     [no test files]

Which issue(s) this PR fixes (관련 이슈):

Fixes: #23

Special notes for your reviewer (리뷰어에게 하고 싶은 말):

이후로는 모든 단위 테스트를 통과한 후에 PR을 등록해주시면 감사하겠습니다.
윈도우 환경에서 테스트 코드를 통과하지 못할 것으로 예상됩니다.
이 부분은 추가 수정이 필요합니다.

storageDriver, logs, rotator 를 잘 정의해주셨는데 역할 분담을 개선할 부분이 있어 보입니다.
현재는 파일관리기능, 로그조회기능, 로테이트 기능이 모두 rotator 리시버(logs 디렉토리)에 묶여 있습니다.

개선방향을 간단히만 적어보면...

  • storageDriver - 스토리지가 바뀌어도 동일한 메소드 제공
  • files - 로그파일 관리 기능 (로그 내용과 무관한 부분)
  • logs - 로그 조회 기능 (로그 내용과 관계 있는 부분)
  • rotator - rotator 기능 (files를 이용하여 제어)

이 부분은 좀더 논의가 필요할 것 같습니다.
감사합니다.

Additional documentation, usage docs, etc. (기타 관련 문서, 사용법 등):

없음

@jmnote jmnote added the testing label Feb 26, 2023
@jmnote jmnote self-assigned this Feb 26, 2023
@jmnote jmnote linked an issue Feb 26, 2023 that may be closed by this pull request
@kuoss kuoss deleted a comment from codecov-commenter Feb 26, 2023
@jmnote jmnote mentioned this pull request Feb 26, 2023
@jmnote
Copy link
Contributor Author

jmnote commented Feb 27, 2023

"All checks have passed"에서 보듯이 모든 점검항목을 통과하였습니다.

  • go-licenses는 68분이나 걸렸는데, 따로 이슈로 만들겠습니다.
  • go-test-with-coverage는 현재 프로젝트 루트에 있는 코드만 대상으로 수행된 것으로 보이는데, 전체를 점검할 수 있도록 해야 겠습니다. 따로 이슈로 만들겠습니다. 폴더(패키지)별 커버리지는 저 위에 기록해놨습니다.

@dozer-jang
Copy link
Contributor

go의 가장 큰 장점 중 하나인 멀티 플랫폼 지원을 잃지 않기 위해서는 개발환경뿐만 아니라 워크로드도 돌아가는게 좋지 않을까요?

@jmnote
Copy link
Contributor Author

jmnote commented Feb 27, 2023

go의 가장 큰 장점 중 하나인 멀티 플랫폼 지원을 잃지 않기 위해서는 개발환경뿐만 아니라 워크로드도 돌아가는게 좋지 않을까요?

네 좋은 의견입니다. prometheus, elasitcsearch, loki는 윈도우 지원하네요. 일단 이번 PR은 리눅스에서 테스트 통과를 목적으로 하고 있었고요. 다음 PR에서 윈도우도 챙겨보겠습니다.

Copy link
Contributor

@dozer-jang dozer-jang left a comment

Choose a reason for hiding this comment

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

윈도우 환경에 대한 지원이 완료되면 다시 코드리뷰를 진행할 예정이고, 테스트 코드 통과를 위해 일단 approve합니다.

@jmnote
Copy link
Contributor Author

jmnote commented Feb 27, 2023

네 확인 감사합니다.
기존 상태가 테스트 통과를 못하는 unstable이라, 나중에 뒤집더라도 테스트를 통과하는 코드를 유지하는 건 중요하다고 생각이 했습니다. 후속으로 윈도우 테스트 진행하여 PR하겠습니다.

@jmnote jmnote merged commit 6a62ede into main Feb 27, 2023
@jmnote jmnote mentioned this pull request Feb 27, 2023
Copy link
Contributor

@dozer-jang dozer-jang left a comment

Choose a reason for hiding this comment

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

좋은 리팩토링이 많네요.

  1. makefile은 매우 유용할 것 같아요. 버전 관리랑 운영용 빌드를 위해서 ldflag를 많이 쓰던데 나중에 참고해서 디벨롭 해보면 좋을 것 같습니다.

  2. testutil.CheckEqualJSON으로 cli output을 검사하는건가요?

  3. assert.Equal로 타입간 비교하던 방식을 다시 testutil.CheckEqualJSON로 변경하셨던데 이유가 있을까요?

  4. unix 패키지 사용은 아래 이유로 재고해봐야 한다고 생각합니다.

    1. lethe가 사용자 환경의 disk 용량까지 확인해야하는가?
    2. 또 확인한다면 권한 문제는 없는지?
    3. os호환성을 잃게 됩니다.

cli/list_test.go Show resolved Hide resolved
clock/clock.go Show resolved Hide resolved
config/config.go Show resolved Hide resolved
letheql/query.go Show resolved Hide resolved
letheql/query.go Show resolved Hide resolved
logs/delete.go Show resolved Hide resolved
logs/filter.go Show resolved Hide resolved
logs/list_test.go Show resolved Hide resolved
logs/list_test.go Show resolved Hide resolved
logs/logs.go Show resolved Hide resolved
@jmnote jmnote deleted the 23-test-env branch February 28, 2023 12:31
Copy link
Contributor

@dozer-jang dozer-jang left a comment

Choose a reason for hiding this comment

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

lethe 로그형식에 대해서 의견을 추가했습니다. 이 PR에서 계속해선 논의하는게 맞는지는 모르겠네요. 오히려 개발 표준이나 환경에 대한 문서를 작성하고 해당 문서에서 쟁점을 논의하는게 좋을지도 모르겠습니다.

letheql/query.go Show resolved Hide resolved
@jmnote
Copy link
Contributor Author

jmnote commented Mar 1, 2023

lethe 로그형식에 대해서 의견을 추가했습니다. 이 PR에서 계속해선 논의하는게 맞는지는 모르겠네요. 오히려 개발 표준이나 환경에 대한 문서를 작성하고 해당 문서에서 쟁점을 논의하는게 좋을지도 모르겠습니다.

#27 에서 논의하는 건 어떨까요?

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.

테스트 환경 정비
2 participants