-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feat/create handler edition test #1152
base: main
Are you sure you want to change the base?
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1152 +/- ##
==========================================
- Coverage 51.09% 48.28% -2.82%
==========================================
Files 123 123
Lines 11116 11118 +2
==========================================
- Hits 5680 5368 -312
- Misses 5092 5455 +363
+ Partials 344 295 -49 ☔ View full report in Codecov by Sentry. |
ありがとう。まだ中身ちゃんと見てないんですが、CIのlintが落ちてるので、それだけ直しといてください。 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
とてもよく書けていると思います。テストケースが抜けている部分もほぼ無くてとてもいいです。細かいところですが修正お願いします。
Goのテストは縦に長くなってしまいがちなので、可読性が失われない範囲で短くするよう工夫できるといいと思います。今回のコメントで言えば、あらかじめdomain.NewGame()
を呼ぶとか、if ... { t.Fatal() }
の代わりにrequire.*
を使うなどです。
src/handler/v2/edition_test.go
Outdated
t.Parallel() | ||
|
||
ctrl := gomock.NewController(t) | ||
defer ctrl.Finish() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go1.14から、gomock.NewController()
に*testing.T
を渡していればctrl.Finish()
を呼ばなくてもよくなっています。あっても問題ないですが、新しいテストコードには書かない方針で行きたいです。
defer ctrl.Finish() |
src/handler/v2/edition_test.go
Outdated
if errors.As(err, &httpErr) { | ||
assert.Equal(t, testCase.statusCode, httpErr.Code) | ||
} else { | ||
t.Errorf("error should be *echo.HTTPError, but got %T", err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert.ErrorAs
を使うと、簡潔に書けるようになります。
strURL := "https://example.com/questionnaire" | ||
questionnaireURL, err := url.Parse(strURL) | ||
if err != nil { | ||
t.Fatalf("failed to parse url: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
t.Fatal
系とt.Error
系の使い分けができていていいと思います。t.Fatal
系の処理を簡単に書けるrequire.Error
とかがassert.Error
とかと似たAPIで使えます。ただのラッパーで処理はほぼ変わらないので、書き換えなくても問題ないです。今後書くときに覚えとくと幸せになると思います。
assert.Equal(t, testCase.expectEditions[i].Name, ed.Name) | ||
assert.Equal(t, testCase.expectEditions[i].Questionnaire, ed.Questionnaire) | ||
assert.WithinDuration(t, testCase.expectEditions[i].CreatedAt, ed.CreatedAt, 2*time.Second) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ここが2秒になってるのはどんな意図がありますか?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
あまり覚えていないので,おそらくどこかのコピペの修正漏れだと思います。すみません:pray:
1sにしておきました。
editionName := "テストエディション" | ||
strURL := "https://example.com/questionnaire" | ||
invalidURL := " https://example.com/questionnaire" | ||
longName := strings.Repeat("あ", 33) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
これもっと明らかにinvalidなURLにしちゃってもいいと思います。最初どこがinvalidなのかわからなかった
}, | ||
{ | ||
description: "ゲームURLとプラットフォームファイルがnullでもエラーなし", | ||
editionID: editionUUID, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
プラットフォームファイルって何だろう。ゲームファイルかと思います
src/handler/v2/edition_test.go
Outdated
gameVersions: []*service.GameVersionWithGame{ | ||
{ | ||
Game: domain.NewGame( | ||
gameID, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
毎回domain.NewGame()
やdomain.NewGameVersion()
を呼び出していてテストケース部分が縦に長くなってしまっているので、他の変数のように前もって定義しておくと見やすくなると思います。
} | ||
|
||
func TestPatchEditionGame(t *testing.T) { | ||
t.Parallel() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TestPatchEditionGame
でPostEditionGame
のテストをやってそう。
そもそもコード本体のコメントが間違ってそうなので、そっちも直しちゃってください
reqBody *openapi.PatchEditionGameRequest | ||
invalidBody bool | ||
executeMock bool | ||
launcherVersionID values.LauncherVersionID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
この変数名だとなにを実行するのかわかりにくいので、他のところと同様にexecute{関数名}
にして欲しいです
src/handler/v2/edition_test.go
Outdated
resultGameVersions: []*service.GameVersionWithGame{ | ||
{ | ||
Game: domain.NewGame( | ||
gameID, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
こちらもあらかじめ定義しておくと見やすくなると思います。
User description
handler/v2/edition.goのテストedition_test.goを追加しました。
あまりテストの書き方に自信がなく,別の色々なテストを参考にしながら書いたので書き方に統一感がない部分があるかもしれないので,そういった部分があれば指摘いただきたいです:bow-nya:
PR Type
Tests
Description
edition_test.go
ファイルに新しいテストケースを追加エディション関連のCRUD操作を網羅するテストを実装
ゲームバージョンや関連データのテストケースを拡充
エラーハンドリングや異常系のテストを強化
Changes walkthrough 📝
edition_test.go
エディションとゲーム関連のテストケースを追加
src/handler/v2/edition_test.go