-
Notifications
You must be signed in to change notification settings - Fork 12
Add stringify test coverage #104
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #104 +/- ##
==========================================
+ Coverage 75.61% 76.00% +0.38%
==========================================
Files 24 24
Lines 1542 1542
==========================================
+ Hits 1166 1172 +6
+ Misses 257 251 -6
Partials 119 119 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Pull Request Overview
Adds test coverage for String() methods of Role, RoleList, and Company domain types by introducing table-driven tests validating formatted output across varied field population scenarios.
- Adds comprehensive String() representation tests for harvest.Role
- Adds comprehensive String() representation tests for harvest.RoleList
- Adds comprehensive String() representation tests for harvest.Company
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| harvest/role_test.go | Adds table-driven tests for Role.String() and RoleList.String(). |
| harvest/company_test.go | Adds table-driven tests for Company.String(). |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| got := tt.in.String() | ||
| assert.Equal(t, tt.want, got) | ||
| }) | ||
| } |
Copilot
AI
Oct 4, 2025
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.
The loop variable tt is captured by the parallel subtest closure; in Go versions prior to 1.22 this can cause races/flaky tests because all goroutines may observe the final tt value. Safely shadow it with tt := tt inside the loop before t.Run.
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| got := tt.in.String() | ||
| assert.Equal(t, tt.want, got) | ||
| }) | ||
| } |
Copilot
AI
Oct 4, 2025
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.
Same loop variable capture issue as above: add tt := tt inside the loop before invoking t.Run to avoid incorrect test data being used when run in parallel on Go versions prior to 1.22.
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| got := tt.in.String() | ||
| assert.Equal(t, tt.want, got) | ||
| }) | ||
| } |
Copilot
AI
Oct 4, 2025
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.
Loop variable tt is captured by a parallel subtest; add a shadow variable (tt := tt) inside the loop to prevent potential data races or mis-associations in parallel execution (required for safety on Go versions prior to 1.22).
No description provided.