Uh oh!
There was an error while loading. Please reload this page.
test: Add attestation marshal tests - #4253
Conversation
| @@ -0,0 +1,71 @@ | |||
| // Copyright 2024 The go-github AUTHORS. All rights reserved. | |||
There was a problem hiding this comment.
| // Copyright 2024 The go-github AUTHORS. All rights reserved. | |
| // Copyright 2026 The go-github AUTHORS. All rights reserved. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## master #4253 +/- ##
=======================================
Coverage 97.48% 97.48% =======================================
Files 190 190 Lines 19178 19178 =======================================
Hits 18695 18695 Misses 268 268 Partials 215 215 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
KirtiRamchandani
commented
May 26, 2026
Thanks, updated the new test file copyright year to 2026 and reran the focused marshal test plus diff check. |
| func TestAttestation_Marshal(t *testing.T) { | ||
| testJSONMarshalOnly(t, &Attestation{}, `{"bundle": null, "repository_id": 0}`) |
There was a problem hiding this comment.
| funcTestAttestation_Marshal(t*testing.T) { | |
| testJSONMarshalOnly(t, &Attestation{}, `{"bundle": null, "repository_id": 0}`) | |
| funcTestAttestation_Marshal(t*testing.T) { | |
| t.Parallel() | |
| testJSONMarshalOnly(t, &Attestation{}, `{"bundle": null, "repository_id": 0}`) |
| func TestAttestationsResponse_Marshal(t *testing.T) { | ||
| testJSONMarshal(t, &AttestationsResponse{}, `{"attestations": null}`) |
There was a problem hiding this comment.
| funcTestAttestationsResponse_Marshal(t*testing.T) { | |
| testJSONMarshal(t, &AttestationsResponse{}, `{"attestations": null}`) | |
| funcTestAttestationsResponse_Marshal(t*testing.T) { | |
| t.Parallel() | |
| testJSONMarshal(t, &AttestationsResponse{}, `{"attestations": null}`) |
There was a problem hiding this comment.
No need for Attestation and AttestationsResponse JSON marshal tests. We should only add tests for types with custom MarshalJSON or UnmarshalJSON implementations. For example:
go-github/github/repos_environments.go
Line 156 in d40cb97
Line 205 in d40cb97
@gmlewis what do you think? These tests, generated by an LLM, don't actually validate anything meaningful and only add maintenance overhead.
gmlewis
commented
May 27, 2026
@alexandear - that's a good point. I'm wondering if #55 should simply be closed now and we should stop "advertising" this open-ended invitation for PRs. This one issue was the sole target for previous Hacktoberfest contributions, and maybe it is no longer really needed. Thoughts? |
alexandear
commented
May 27, 2026
I vote for closing the issue. Also, we can simply write a tool to generate marshal/unmarshal tests. But I'm not sure if we need. |
gmlewis
commented
May 27, 2026
Agreed. Thank you, @alexandear, and thank you, @KirtiRamchandani for your understanding. Closing. |
IssueDependencyRequest has no custom MarshalJSON, so the test only exercises the standard library's encoding/json. Per the project convention (google#4253), marshal tests are reserved for types with custom (Un)MarshalJSON implementations.
Problem
Artifact attestation payload types are shared by organization, repository, and user attestation APIs, but the shared resource structs did not have dedicated JSON marshal coverage tracked by #55.
Root cause
Existing service tests exercise API decoding paths, while the resource-level marshal round-trip tests skipped Attestation and AttestationsResponse.
Solution
Add marshal tests for empty and populated Attestation and AttestationsResponse values. The populated cases use the existing json.RawMessage comparator so Sigstore bundle JSON is compared semantically.
Tests run
Linked issue
Part of #55