test: increase github.go test coverage from 49.2% to 83.1% #1067
Conversation
Kusari Analysis ResultsAnalysis for commit: bbf0f32, performed at: 2025-08-11T20:49:59Z • • Recommendation✅ PROCEED with this Pull Request Summary✅ No Flagged Issues Detected All values appear to be within acceptable risk parameters. No pinned version dependency changes, code issues or exposed secrets detected! Found this helpful? Give it a 👍 or 👎 reaction! |
|
This looks to affect the same code tests as #995. I'll take a look if there's anything that is still untested after that one is merged. |
|
Thank you for the update @patzielinski |
|
@yuvraj-kolkar17 Please feel free to take a look at https://coveralls.io/github/gittuf/gittuf, the coverage report for gittuf. Most files that don't have decent coverage can be worked on to better their coverage. |
05455ef to
371c083
Compare
|
Kusari PR Analysis rerun based on - 371c083 performed at: 2025-08-11T20:45:38Z - link to updated analysis |
371c083 to
bbf0f32
Compare
|
Kusari PR Analysis rerun based on - bbf0f32 performed at: 2025-08-11T20:49:59Z - link to updated analysis |
Co-authored-by: Yongjae Chung <[email protected]> Signed-off-by: yuvraj Kolkar <[email protected]>
bbf0f32 to
ab41e72
Compare
| attestations := &Attestations{} | ||
|
|
||
| // This should fail validation | ||
| err := attestations.SetGitHubPullRequestApprovalAttestation(repo, invalidEnv, baseURL, 1, appName, testRef, testID, testID) |
There was a problem hiding this comment.
This case shouldn't be under TestGetGitHubPullRequestApprovalAttestation I think?
There was a problem hiding this comment.
@adityasaky Seems like we already test for validation in a different test file:
Should we get rid of this particular test case here then? Should we factor it out to a different test function?
There was a problem hiding this comment.
I think that we can probably put this case under TestSetGitHubPullRequestApprovalAttestation.
There was a problem hiding this comment.
I could see it either way, do we see a coverage difference with and without this specific case?
There was a problem hiding this comment.
Yup, we cover an addition line where validation errors. I'll go ahead and move the test case under TestSetGitHubPullRequestApprovalAttestation.
|
|
||
| // Try to get attestation with invalid URL | ||
| _, err := attestations.GetGitHubPullRequestApprovalAttestationForReviewID(repo, "invalid-url", 123, "github") | ||
| assert.NotNil(t, err) |
There was a problem hiding this comment.
Do we have a specific error we can test for here?
What type of PR is this?
/kind test
/area attestations
What this PR does / why we need it:
This PR adds a complete unit test suite for the logic defined in
internal/attestations/github.go.Coverage Improvement
Before vs After Coverage:
Which issue(s) this PR fixes:
#463
Does this PR introduce a user-facing change?
None