-
Notifications
You must be signed in to change notification settings - Fork 4
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
test: add tests for gitMergeRequests #169
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #169 +/- ##
==========================================
+ Coverage 33.93% 36.31% +2.37%
==========================================
Files 9 9
Lines 716 716
==========================================
+ Hits 243 260 +17
+ Misses 463 446 -17
Partials 10 10 ☔ View full report in Codecov by Sentry. |
{ | ||
desc: "valid mergeRequestData", | ||
client: &mockClient{ | ||
maxPages: 1, |
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.
You could have another test for multiple pages and no pages
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.
Good idea. Thank!
assert.Equal(t, len(ch), 1) | ||
} else { | ||
assert.Equal(t, len(ch), 1) | ||
for data := range ch { |
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.
Should move this out of the if else block since it should be tested even if theres an error which should have a total mr count of 0. Also there will only ever be a channel size of one so maybe theres a better way to do this than a for loop?
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.
I needed something in the if statement in case I wanted to add something but since there is nothing to add, I'll move it out. Good call out.
} else { | ||
assert.Equal(t, len(ch), 1) | ||
for data := range ch { | ||
assert.Equal(t, len(data), tc.expectedNodeLen) |
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.
This should also be renamed to total mr count (idk how to suggest a code change)
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.
Changed to expectedMergeRequestLength
I think this is what you mean.
I could use some insight on practical test cases. I have hit all the lines of code but the tests are not very significant.