-
Notifications
You must be signed in to change notification settings - Fork 12
Improve code coverage #103
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 #103 +/- ##
==========================================
+ Coverage 71.72% 75.61% +3.89%
==========================================
Files 24 24
Lines 1542 1542
==========================================
+ Hits 1106 1166 +60
+ Misses 315 257 -58
+ Partials 121 119 -2 ☔ 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
This PR improves code coverage by adding comprehensive test cases for core harvest package functionality. The changes focus on testing String methods, API client functionality, and error handling scenarios.
- Adds extensive test coverage for Project and ProjectList String() methods
- Implements comprehensive tests for API client creation, request handling, and error scenarios
- Adds tests for helper functions, pagination, and rate limiting functionality
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| harvest/project_test.go | Adds String() method tests for Project and ProjectList structs with various field combinations |
| harvest/harvest_test.go | Adds comprehensive test coverage for API client functionality, error handling, helper functions, and core types |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| CreatedAt: harvest.TimeTimeP(time.Date(2017, 6, 26, 21, 52, 18, 0, time.UTC)), | ||
| UpdatedAt: harvest.TimeTimeP(time.Date(2017, 6, 26, 21, 54, 6, 0, time.UTC)), | ||
| }, | ||
| want: `harvest.Project{ID:14308069, Client:harvest.Client{ID:5735776, Name:"123 Industries", Currency:"EUR"}, Name:"Online Store - Phase 1", Code:"OS1", IsActive:true, IsBillable:true, IsFixedFee:false, BillBy:"Project", HourlyRate:100, Budget:200, BudgetBy:"project", BudgetIsMonthly:false, NotifyWhenOverBudget:true, OverBudgetNotificationPercentage:80, ShowBudgetToAll:false, CostBudgetIncludeExpenses:false, Notes:"", StartsOn:harvest.Date{{2017-06-01 00:00:00 +0000 UTC}}, CreatedAt:time.Time{2017-06-26 21:52:18 +0000 UTC}, UpdatedAt:time.Time{2017-06-26 21:54:06 +0000 UTC}}`, //nolint: lll |
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.
[nitpick] This expected string is extremely long and hard to read/maintain. Consider breaking it into multiple lines or using string concatenation to improve readability.
| want: `harvest.Project{ID:14308069, Client:harvest.Client{ID:5735776, Name:"123 Industries", Currency:"EUR"}, Name:"Online Store - Phase 1", Code:"OS1", IsActive:true, IsBillable:true, IsFixedFee:false, BillBy:"Project", HourlyRate:100, Budget:200, BudgetBy:"project", BudgetIsMonthly:false, NotifyWhenOverBudget:true, OverBudgetNotificationPercentage:80, ShowBudgetToAll:false, CostBudgetIncludeExpenses:false, Notes:"", StartsOn:harvest.Date{{2017-06-01 00:00:00 +0000 UTC}}, CreatedAt:time.Time{2017-06-26 21:52:18 +0000 UTC}, UpdatedAt:time.Time{2017-06-26 21:54:06 +0000 UTC}}`, //nolint: lll | |
| want: "harvest.Project{ID:14308069, Client:harvest.Client{ID:5735776, Name:\"123 Industries\", Currency:\"EUR\"}, " + |
| Page: harvest.Int(1), | ||
| }, | ||
| }, | ||
| want: `harvest.ProjectList{Projects:[harvest.Project{ID:14308069, Client:harvest.Client{ID:5735776, Name:"123 Industries", Currency:"EUR"}, Name:"Online Store - Phase 1"} harvest.Project{ID:14307913, Client:harvest.Client{ID:5735774, Name:"ABC Corp", Currency:"USD"}, Name:"Marketing Website"}], Pagination:harvest.Pagination{PerPage:100, TotalPages:1, TotalEntries:2, Page:1}}`, //nolint: lll |
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.
[nitpick] This expected string is extremely long and hard to read/maintain. Consider breaking it into multiple lines or using string concatenation to improve readability.
| want: `harvest.ProjectList{Projects:[harvest.Project{ID:14308069, Client:harvest.Client{ID:5735776, Name:"123 Industries", Currency:"EUR"}, Name:"Online Store - Phase 1"} harvest.Project{ID:14307913, Client:harvest.Client{ID:5735774, Name:"ABC Corp", Currency:"USD"}, Name:"Marketing Website"}], Pagination:harvest.Pagination{PerPage:100, TotalPages:1, TotalEntries:2, Page:1}}`, //nolint: lll | |
| want: `harvest.ProjectList{ |
| }, | ||
| }, | ||
| }, | ||
| want: `harvest.ProjectList{Projects:[harvest.Project{ID:100, Name:"Test Project"}], Pagination:harvest.Pagination{PerPage:25, TotalPages:3, TotalEntries:75, Page:2, Links:harvest.PageLinks{First:"https://api.harvestapp.com/v2/projects?page=1&per_page=25", Next:"https://api.harvestapp.com/v2/projects?page=3&per_page=25", Previous:"https://api.harvestapp.com/v2/projects?page=1&per_page=25", Last:"https://api.harvestapp.com/v2/projects?page=3&per_page=25"}}}`, //nolint: lll |
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.
[nitpick] This expected string is extremely long and hard to read/maintain. Consider breaking it into multiple lines or using string concatenation to improve readability.
| want: `harvest.ProjectList{Projects:[harvest.Project{ID:100, Name:"Test Project"}], Pagination:harvest.Pagination{PerPage:25, TotalPages:3, TotalEntries:75, Page:2, Links:harvest.PageLinks{First:"https://api.harvestapp.com/v2/projects?page=1&per_page=25", Next:"https://api.harvestapp.com/v2/projects?page=3&per_page=25", Previous:"https://api.harvestapp.com/v2/projects?page=1&per_page=25", Last:"https://api.harvestapp.com/v2/projects?page=3&per_page=25"}}}`, //nolint: lll | |
| want: "harvest.ProjectList{Projects:[harvest.Project{ID:100, Name:\"Test Project\"}], " + | |
| "Pagination:harvest.Pagination{PerPage:25, TotalPages:3, TotalEntries:75, Page:2, " + | |
| "Links:harvest.PageLinks{" + | |
| "First:\"https://api.harvestapp.com/v2/projects?page=1&per_page=25\", " + | |
| "Next:\"https://api.harvestapp.com/v2/projects?page=3&per_page=25\", " + | |
| "Previous:\"https://api.harvestapp.com/v2/projects?page=1&per_page=25\", " + | |
| "Last:\"https://api.harvestapp.com/v2/projects?page=3&per_page=25\"}}}", //nolint: lll |
| Last: harvest.String("https://api.harvestapp.com/v2/clients?page=5"), | ||
| }, | ||
| }, | ||
| want: `harvest.Pagination{PerPage:100, TotalPages:5, TotalEntries:450, NextPage:3, PreviousPage:1, Page:2, Links:harvest.PageLinks{First:"https://api.harvestapp.com/v2/clients?page=1", Next:"https://api.harvestapp.com/v2/clients?page=3", Previous:"https://api.harvestapp.com/v2/clients?page=1", Last:"https://api.harvestapp.com/v2/clients?page=5"}}`, //nolint: lll |
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.
[nitpick] This expected string is extremely long and hard to read/maintain. Consider breaking it into multiple lines or using string concatenation to improve readability.
| want: `harvest.Pagination{PerPage:100, TotalPages:5, TotalEntries:450, NextPage:3, PreviousPage:1, Page:2, Links:harvest.PageLinks{First:"https://api.harvestapp.com/v2/clients?page=1", Next:"https://api.harvestapp.com/v2/clients?page=3", Previous:"https://api.harvestapp.com/v2/clients?page=1", Last:"https://api.harvestapp.com/v2/clients?page=5"}}`, //nolint: lll | |
| want: "harvest.Pagination{PerPage:100, TotalPages:5, TotalEntries:450, NextPage:3, PreviousPage:1, Page:2, " + | |
| "Links:harvest.PageLinks{First:\"https://api.harvestapp.com/v2/clients?page=1\", " + | |
| "Next:\"https://api.harvestapp.com/v2/clients?page=3\", " + | |
| "Previous:\"https://api.harvestapp.com/v2/clients?page=1\", " + | |
| "Last:\"https://api.harvestapp.com/v2/clients?page=5\"}}", |
| Previous: harvest.String("https://api.harvestapp.com/v2/clients?page=1"), | ||
| Last: harvest.String("https://api.harvestapp.com/v2/clients?page=5"), | ||
| }, | ||
| want: `harvest.PageLinks{First:"https://api.harvestapp.com/v2/clients?page=1", Next:"https://api.harvestapp.com/v2/clients?page=3", Previous:"https://api.harvestapp.com/v2/clients?page=1", Last:"https://api.harvestapp.com/v2/clients?page=5"}`, //nolint: lll |
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.
[nitpick] This expected string is extremely long and hard to read/maintain. Consider breaking it into multiple lines or using string concatenation to improve readability.
| want: `harvest.PageLinks{First:"https://api.harvestapp.com/v2/clients?page=1", Next:"https://api.harvestapp.com/v2/clients?page=3", Previous:"https://api.harvestapp.com/v2/clients?page=1", Last:"https://api.harvestapp.com/v2/clients?page=5"}`, //nolint: lll | |
| want: "harvest.PageLinks{" + | |
| "First:\"https://api.harvestapp.com/v2/clients?page=1\", " + | |
| "Next:\"https://api.harvestapp.com/v2/clients?page=3\", " + | |
| "Previous:\"https://api.harvestapp.com/v2/clients?page=1\", " + | |
| "Last:\"https://api.harvestapp.com/v2/clients?page=5\"" + | |
| "}", |
No description provided.