Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements comprehensive test coverage for the Announcement handler and creates a mock repository for testing purposes. The implementation adds handler tests for all CRUD operations (List, Detail, Create, Update, Delete) with authentication verification, along with a mock repository that returns predictable test data.
Changes:
- Created a mock implementation of AnnouncementRepository for testing
- Added comprehensive handler tests for all announcement endpoints with table-driven test patterns
- Added testify library as a test dependency
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 19 comments.
| File | Description |
|---|---|
| internal/repository/announcement_mock.go | Mock repository implementation that returns predictable test data for all CRUD operations |
| internal/handler/announcement_test.go | Comprehensive test suite for announcement handlers covering basic functionality and authentication |
| go.mod | Added testify testing library dependency and its transitive dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kantacky
left a comment
There was a problem hiding this comment.
Copilotのレビュー直せば良さそう
403のケースとか、カスタムクレームの組み合わせのケースとか
… for exact error message
Enhanced the TestAnnouncementsV1List function to include a new test case for handling requests with a developer claim. Updated the test structure to accommodate the additional claim and ensured proper validation of the response for this scenario.
Enhanced the TestAnnouncementsV1Update function by adding a new test case that verifies the response when a token with non-admin/developer claims is used. This ensures proper handling of insufficient permissions and improves overall test coverage for authentication scenarios.
Enhanced the TestAnnouncementsV1Create function by adding a new test case that verifies the ability to create announcements with a developer claim. Updated the test structure to include the new claim and ensured proper validation of the response for this scenario.
Updated the TestAnnouncementsV1Delete function to include a new test case for handling deletion requests with a developer claim. Adjusted the expected status code to reflect the correct response for successful deletions and modified the test setup to accommodate different claim types. Additionally, changed the handler to use AbortWithStatus for consistency in response handling.
Updated the TestAnnouncementsV1List and TestAnnouncementsV1Update functions to enhance test clarity and error handling. Introduced the require package for better error assertions and adjusted variable formatting for consistency. This improves overall test reliability and readability.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert.Equal(t, "application/json; charset=utf-8", w.Header().Get("Content-Type")) | ||
| }, |
There was a problem hiding this comment.
Content-Type を完全一致で application/json; charset=utf-8 に固定すると、Gin の挙動変更やミドルウェアによる差分でテストが壊れやすいです。mime.ParseMediaType で media type 部分だけを確認するか、strings.HasPrefix(contentType, "application/json") のように JSON であることを検証する形がより堅牢です。
|
Copilotの指摘を修正し,全テストが通ることを確認しています |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
新しくきたCopilotの指摘を直したらいいと思います |
|
@hikaru-0602 |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@kantacky |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@hikaru-0602 |
…on validation Updated TestAnnouncementsV1List, TestAnnouncementsV1Detail, and TestAnnouncementsV1Delete to include custom claims for testing 403 error scenarios. This allows for more granular permission checks and improves test coverage for authorization logic.
|
@kantacky |
|
まだコメント残ってるけど... |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert.False(t, response.Announcements[0].AvailableFrom.IsZero(), "AvailableFromが設定されていること") | ||
| assert.False(t, response.Announcements[0].AvailableUntil.IsZero(), "AvailableUntilが設定されていること") |
There was a problem hiding this comment.
api.AnnouncementServiceAnnouncement.AvailableUntil は *time.Time なので、response.Announcements[0].AvailableUntil.IsZero() はコンパイルできません。AvailableUntil != nil の確認をした上で response.Announcements[0].AvailableUntil.IsZero() を呼ぶ(もしくは *response... をデリファレンスして IsZero())形に修正してください。
| assert.False(t, response.Announcements[0].AvailableFrom.IsZero(), "AvailableFromが設定されていること") | |
| assert.False(t, response.Announcements[0].AvailableUntil.IsZero(), "AvailableUntilが設定されていること") | |
| assert.False(t, response.Announcements[0].AvailableFrom.IsZero(), "AvailableFromが設定されていること") | |
| assert.NotNil(t, response.Announcements[0].AvailableUntil, "AvailableUntilが設定されていること") | |
| assert.False(t, response.Announcements[0].AvailableUntil.IsZero(), "AvailableUntilがゼロ時刻でないこと") |
| func TestAnnouncementsV1Detail(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| id string | ||
| withAdminClaim bool | ||
| customClaims map[string]interface{} // 指定時はこのクレームでトークンをセット(403検証用) | ||
| wantCode int | ||
| validate func(t *testing.T, w *httptest.ResponseRecorder) | ||
| }{ | ||
| { | ||
| name: "正常にお知らせ詳細が取得できる", | ||
| id: "1", | ||
| withAdminClaim: true, | ||
| wantCode: http.StatusOK, | ||
| validate: func(t *testing.T, w *httptest.ResponseRecorder) { | ||
| var response struct { | ||
| Announcement api.AnnouncementServiceAnnouncement `json:"announcement"` | ||
| } | ||
| err := json.Unmarshal(w.Body.Bytes(), &response) | ||
| assert.NoError(t, err, "JSONのパースに失敗しました") | ||
| assert.Equal(t, "1", response.Announcement.Id) | ||
| assert.Equal(t, "お知らせ1", response.Announcement.Title) | ||
| assert.Equal(t, "https://example.com/1", response.Announcement.Url) | ||
| }, | ||
| }, |
There was a problem hiding this comment.
AnnouncementsV1Detail はハンドラ側で RequireAnyClaim(c, "admin", "developer") を使っているので、List/Create/Delete と同様に developer クレームのみで 200 になるケースもテストに追加したいです(現状 Detail は admin と 401/403 しか検証していません)。
| func TestAnnouncementsV1Update(t *testing.T) { | ||
| now := time.Now() | ||
| until := now.Add(24 * time.Hour) | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| id string | ||
| request api.AnnouncementServiceAnnouncementRequest | ||
| withAdminClaim bool | ||
| customClaims map[string]interface{} // 指定時はこのクレームでトークンをセット(403検証用) | ||
| wantCode int | ||
| validate func(t *testing.T, w *httptest.ResponseRecorder) | ||
| }{ | ||
| { | ||
| name: "正常にお知らせを更新できる", | ||
| id: "1", | ||
| request: api.AnnouncementServiceAnnouncementRequest{ | ||
| Title: "更新されたお知らせ", | ||
| Url: "https://example.com/updated", | ||
| AvailableFrom: now, | ||
| AvailableUntil: &until, | ||
| }, | ||
| withAdminClaim: true, | ||
| wantCode: http.StatusOK, | ||
| validate: func(t *testing.T, w *httptest.ResponseRecorder) { | ||
| var response struct { | ||
| Announcement api.AnnouncementServiceAnnouncement `json:"announcement"` | ||
| } | ||
| err := json.Unmarshal(w.Body.Bytes(), &response) | ||
| assert.NoError(t, err, "JSONのパースに失敗しました") | ||
| assert.Equal(t, "1", response.Announcement.Id) | ||
| assert.Equal(t, "更新されたお知らせ", response.Announcement.Title) | ||
| assert.Equal(t, "https://example.com/updated", response.Announcement.Url) | ||
| }, | ||
| }, |
There was a problem hiding this comment.
AnnouncementsV1Update もハンドラ側で RequireAnyClaim(c, "admin", "developer") を使っているため、developer クレームのみで更新できる(200)ケースのテストが欲しいです。今のテストテーブルには developer 用の入力フラグがなく、admin と 401/403 のみになっています。
やったこと