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

dockertestの導入 #691

Merged
merged 18 commits into from
Sep 30, 2023
Merged

dockertestの導入 #691

merged 18 commits into from
Sep 30, 2023

Conversation

ikura-hamu
Copy link
Member

@ikura-hamu ikura-hamu commented Sep 19, 2023

resolve: #468

今までdocker compose を使ってDBやストレージのテストを行っていたが、 https://github.com/ory/dockertest/tree/v3 を使うようにした。

これによってコンテナを常時立てておかなくても手元でユニットテストを実行しやすくなった。VSCodeの「run test」ボタンとかからもテストを実行できる。

@codecov
Copy link

codecov bot commented Sep 21, 2023

Codecov Report

All modified lines are covered by tests ✅

Files Coverage Δ
src/storage/s3/client.go 66.97% <100.00%> (+0.93%) ⬆️

... and 1 file with indirect coverage changes

📢 Thoughts on this report? Let us know!.

@ikura-hamu ikura-hamu marked this pull request as ready for review September 21, 2023 07:29
@ikura-hamu ikura-hamu requested a review from mazrean September 21, 2023 07:30
Copy link
Member

@mazrean mazrean left a comment

Choose a reason for hiding this comment

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

消えると面倒なので、とりあえず一旦今まで気になったポイントだけsubmitしておきます

src/repository/gorm2/db_test.go Outdated Show resolved Hide resolved
"github.com/traPtitech/trap-collection-server/src/storage"
)

var testClient *Client

func (c *Client) createBucket() error {
fmt.Printf("bucket: %v\n", c.bucket)
Copy link
Member

Choose a reason for hiding this comment

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

デバッグ用のログ消し忘れていそう


// テスト用のAppConfig
// config.Appを実装
type testAppConfig struct{}
Copy link
Member

Choose a reason for hiding this comment

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

configにもmockがあるはずなので、そちらを使った方がよさそう


// テスト用のRepositoryConfig
// config.RepositoryGorm2を実装
type testRepositoryConfig struct {
Copy link
Member

Choose a reason for hiding this comment

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

testAppConfigと同様

os.Exit(code)
}

type testStorageS3 struct {
Copy link
Member

Choose a reason for hiding this comment

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

testAppConfigと同様


pool, err := dockertest.NewPool("")
if err != nil {
log.Fatalf("Could not create pool: %s", err)
Copy link
Member

Choose a reason for hiding this comment

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

log.Fatalfdeferとか無視して強制終了するので、基本的にpanicfmt.Sprintfとかで書いた方が良いです

Repository: "mariadb",
Tag: "10.6.4",
Env: []string{
"MYSQL_ROOT_PASSWORD=pass",
Copy link
Member

Choose a reason for hiding this comment

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

passとかのdb接続時にも使う定数はconstで定義してそれ使うように書いた方が良いかも

src/repository/gorm2/db_test.go Outdated Show resolved Hide resolved
}

if err := pool.Retry(func() error {
testDB, err = NewDB(&testAppConfig{}, &testRepositoryConfig{resource.GetPort("3306/tcp")})
Copy link
Member

Choose a reason for hiding this comment

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

GetPortのデフォルト、3306/tcpだとデフォルト値になったときstrconv.Atoiに失敗しそう?

Copy link
Member

@mazrean mazrean left a comment

Choose a reason for hiding this comment

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

LGTM!
ただ、1点だけ少し綺麗に書けそうな箇所があったのでコメント入れました。

Comment on lines 162 to 163
s3UsePathStyle := os.Getenv(envKeyS3UsePathStyle)
return s3UsePathStyle == "true"
Copy link
Member

@mazrean mazrean Sep 25, 2023

Choose a reason for hiding this comment

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

os.LookupEnv使った方が「デフォルトはfalse」というのが分かりやすくてよいかも。
あと、boolのstringの判定はstrconv.ParseBoolとかを使った方が綺麗に一般的にboolっぽく扱われる文字列を変換できて良さそう。

@ikura-hamu ikura-hamu merged commit 4b1e973 into main Sep 30, 2023
8 checks passed
@ikura-hamu ikura-hamu deleted the dockertest branch September 30, 2023 04:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

テスト用docker composeのdockertestへの置き換え
3 participants