Improve testJSONMarshal

[exageraldo]

Fixes: #2699

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.24%. Comparing base (6a7684f) to head (0ded469).
⚠️ Report is 324 commits behind head on master.

Additional details and impacted files

@@ Coverage Diff @@## master #3519 +/- ##
=======================================
Coverage 91.23% 91.24% =======================================
Files 183 183 Lines 16053 16063 +10 =======================================
+ Hits 14646 14656 +10 
Misses 1231 1231 Partials 176 176 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[gmlewis]

@exageraldo - is this still a draft PR? If not, can you please remove the draft status?

[gmlewis]

Thank you, @exageraldo!
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

@stevehipwell - might you have time for a code review? Thank you!

[stevehipwell]

Thanks for the PR @exageraldo, I'm familiar with this issue as I had to implement my own solutions (marshal & unmarshal) for this when I refactored the rules and a global fix would be great!

Based on my experience think your implementation of testJSONMarshal is an improvement but still doesn't quite keep the marshal and unmarshal concerns separate. I think a function to test both marshal and unmarshal independently would look something like this.

functestJSONMarshal(t*testing.T, vany, wantstring) {
t.Helper()
gotBytes, err:=json.Marshal(v)
iferr!=nil {
t.Errorf("Unable to marshal JSON for %#v", v)
}
ifdiff:=cmp.Diff(want, string(gotBytes)); diff!="" {
t.Errorf(
"json.Marshal returned:\n%s\nwant:\n%s\ndiff:\n%v",
gotBytes,
want,
diff,
)
}
vargotAnyanyerr=json.Unmarshal([]byte(want), gotAny)
iferr!=nil {
t.Errorf("Unable to unmarshal JSON %v: %v", want, err)
}
ifdiff:=cmp.Diff(v, gotAny); diff!="" {
t.Errorf(
"json.Unmarshal returned:\n%#v\nwant:\n%#v\ndiff:\n%v",
gotAny,
v,
diff,
)
}
}

If this was greenfield I'd suggest splitting the test methods up and have a seperate test method for marshal and unmarshal, but the above signature matches the current state of play.

[exageraldo]

hey @stevehipwell , many thanks for the comment/suggestion!

After applying this changes (only the testJSONMarshal function was changed), some tests started to break because they were expecting a json string in a certain way, with the fields in a certain order (other errors are also occurring which I'm still investigating further).
Approximately 460 tests broke with this change.

Here is an example of the output of the github/actions_oidc_test.go::TestOIDCSubjectClaimCustomTemplate_Marshal test:

json.Marshal returned:
{"use_default":false,"include_claim_keys":["s"]}
want:
{
"use_default": false,
"include_claim_keys": [
"s"
]
}
diff:
(
"""
- {
- "use_default": false,
- "include_claim_keys": [
- "s"
- ]
- }
+ {"use_default":false,"include_claim_keys":["s"]}
"""
)
Unable to unmarshal JSON {
"use_default": false,
"include_claim_keys": [
"s"
]
}: json: Unmarshal(nil)
json.Unmarshal returned:
<nil>
want:
&github.OIDCSubjectClaimCustomTemplate{UseDefault:(*bool)(0x14004fec4e0), IncludeClaimKeys:[]string{"s"}}
diff:
any(
- &github.OIDCSubjectClaimCustomTemplate{UseDefault: &false, IncludeClaimKeys: []string{"s"}},
)

Do you think the idea of the order of the JSON fields being considered when setting up the test is valid?

[gmlewis]

Do you think the idea of the order of the JSON fields being considered when setting up the test is valid?

We explicitly do NOT want to dictate the required order of the JSON fields in any test as that is an unnecessary maintenance and developer burden.

[exageraldo]

@gmlewis another comment on the changes: some fields have had their initial letters changed to capital letters (like this). I did a quick search and couldn't find if these properties are really case insensitive.

Since there's no issue about it, can we consider that it's working properly?

[gmlewis]

This looks really odd to me. ListOptions is defined here:

go-github/github/github.go

Line 250 in 04274a9

PerPageint`url:"per_page,omitempty"`

which looks like this:

PerPageint`url:"per_page,omitempty"`

Looking at the documentation for json.Marshal, it only handles the Go json field tag. Our Go url field tags are being handled by this package:
https://pkg.go.dev/github.com/google/go-querystring/query#Values

So my theory is that we are sending url query params through json.Marshal which has no json field tag on it, so it uses the exact spelling of the field name (i.e. PerPage). Apparently, it is also forgiving and was allowing perPage previously.

But I think the bottom line is that url query fields should not be handled by json.Marshal or even tested with it, as it provides confusing results as seen here, which will never happen in normal usage.

[gmlewis]

It just dawned on me that we test url field tags with testFormValues and not with json.Marshal... so this should probably fixed. For example, this is very common in our repo:

testFormValues(t, r, values{"per_page": "2", "page": "2"})

which has the correct case for the fields.

[exageraldo]

I was initially taking a look at these two test functions:

• func testAddURLOptions(t *testing.T, url string, v interface{}, want string): It's used in only one test (github/scim_test.go::TestListSCIMProvisionedIdentitiesOptions_addOptions)
• func testFormValues(t *testing.T, r *http.Request, values values): It is used in many cases, but in the tests we have today using the testJSONMarshal function, we don't have the request object (http.Request).

I created a new test function (testURLParseValues) to help us with this, trying to keep the same signature as testJSONMarshal.
The idea of the function is also the same: to take any structure and check if it matches a string that we are expecting, which will be sent to github during the request we are making.

functestURLParseValues(t*testing.T, vinterface{}, wantstring) {
values, err:=query.Values(v)
iferr!=nil {
t.Errorf("Unable to parse URL values for %#v: %v", v, err)
}
got:=values.Encode()
ifgot!=want {
t.Errorf("query.Values returned %v, want %v", got, want)
}
}

For the tests where we have to check the URL tags, the changes are as follows:

// github/users_test.go// BEFOREfuncTestHovercardOptions_Marshal(t*testing.T) {
t.Parallel()
testJSONMarshal(t, &HovercardOptions{}, `{ "SubjectType" : "", "SubjectID" : ""	}`)
u:=&HovercardOptions{
SubjectType: "subjectType",
SubjectID: "subjectID",
}
want:=`{ "SubjectType" : "subjectType", "SubjectID" : "subjectID"	}`testJSONMarshal(t, u, want)
}
// AFTERfuncTestHovercardOptions_URLParse(t*testing.T) {
t.Parallel()
testURLParseValues(t, &HovercardOptions{}, "subject_id=&subject_type=")
u:=&HovercardOptions{
SubjectType: "subjectType",
SubjectID: "subjectID",
}
testURLParseValues(t, u, "subject_id=subjectID&subject_type=subjectType")
}

This way I believe we still have a good view of what is being assembled and sent to Github.
But this way we unfortunately hit the case that the order of the fields is taken into account, since we are comparing two strings.

[gmlewis]

But this way we unfortunately hit the case that the order of the fields is taken into account, since we are comparing two strings.

Right... the only way to get around the ordering problem is to run both the "want" and the "got" through json.MarshalIndent (preferred over json.Marshal because of the nice line-oriented formatting) so they get the same sorting treatment.

[gmlewis]

Then why not test with query.Values instead of json.Marshal?

[stevehipwell]

@gmlewis I'm not sure why json.Marshal would be anywhere near the query string params tests, but I could just be confused.

[gmlewis]

@gmlewis I'm not sure why json.Marshal would be anywhere near the query string params tests, but I could just be confused.

Yes, that is my point. It shouldn't be involved at all and should be removed.

[exageraldo]

Would it be valid to use the functiontestURLParseValues(t *testing.T, v interface{}, want string) (suggested a bit above) to test query strings?

[gmlewis]

Would it be valid to use the functiontestURLParseValues(t *testing.T, v interface{}, want string) (suggested a bit above) to test query strings?

SGTM.

[stevehipwell]

Sorry I wrote my reply too quickly and didn't spot two issues with the code. The first being that comparing strings doesn't work for this use case (for my quick fix for rules I just set the order) and the second being that you can't use any for the unmarshal test.

Here is the updated pattern with the two sub-checks in their own functions. I've quickly tested this code on TestOIDCSubjectClaimCustomTemplate_Marshal and it works as expected.

functestJSONMarshal[Tany](t*testing.T, vT, wantstring) {
t.Helper()
testJSONMarshalMarshal(t, v, want)
testJSONMarshalUnmarshal(t, want, v)
}
functestJSONMarshalMarshal[Tany](t*testing.T, vT, wantstring) {
t.Helper()
got, err:=json.Marshal(v)
iferr!=nil {
t.Errorf("Unable to marshal got JSON for %#v", v)
}
vargotAny, wantAnyanyiferr=json.Unmarshal(got, &gotAny); err!=nil {
t.Errorf("Unable to unmarshal want JSON %v: %v", got, err)
}
iferr=json.Unmarshal([]byte(want), &wantAny); err!=nil {
t.Errorf("Unable to unmarshal JSON %v: %v", want, err)
}
ifdiff:=cmp.Diff(wantAny, gotAny); diff!="" {
t.Errorf(
"json.Marshal returned:\n%s\nwant:\n%s\ndiff:\n%v",
got,
want,
diff,
)
}
}
functestJSONMarshalUnmarshal[Tany](t*testing.T, vstring, wantT) {
t.Helper()
vargotTerr:=json.Unmarshal([]byte(v), &got)
iferr!=nil {
t.Errorf("Unable to unmarshal JSON %v: %v", want, err)
}
ifdiff:=cmp.Diff(want, got); diff!="" {
t.Errorf(
"json.Unmarshal returned:\n%#v\nwant:\n%#v\ndiff:\n%v",
got,
v,
diff,
)
}
}

[exageraldo]

@stevehipwell when applying this new suggestion, some test cases broke in testJSONMarshalUnmarshal (approximately 25). It's difficult trying to convert a JSON into a struct, because it's not taking into account omitted and non-exported fields, for example. There is also the case of uninitialized lists/slices (nil vs []any{}).

Here are some examples (I added the structure to give it more context):

github/repos_test.go::TestDispatchRequestOptions_Marshal

typeDispatchRequestOptionsstruct {
EventTypestring`json:"event_type"`ClientPayload*json.RawMessage`json:"client_payload,omitempty"`
}
// OUTPUT:// json.Unmarshal returned:// &github.DispatchRequestOptions{EventType:"test_event_type", ClientPayload:(*json.RawMessage)(0x140036507c8)}// want:// "{\n\t\t\"event_type\": \"test_event_type\",\n\t\t\"client_payload\": {\n\t\t \"testKey\": \"testValue\"\n\t\t}\n\t }"// diff:// &github.DispatchRequestOptions{// EventType: "test_event_type",// ClientPayload: &json.RawMessage{// 0x7b,// + 0x0a,// + 0x09,// + 0x09,// + 0x20,// + 0x20,// 0x22,// 0x74,// ... // 6 identical elements// 0x22,// 0x3a,// + 0x20,// 0x22,// 0x74,// ... // 7 identical elements// 0x65,// 0x22,// + 0x0a,// + 0x09,// + 0x09,// 0x7d,// },// }

github/search_test.go::TestCommitResult_Marshal

typeCommitResultstruct {
SHA*string`json:"sha,omitempty"`Commit*Commit`json:"commit,omitempty"`Author*User`json:"author,omitempty"`Committer*User`json:"committer,omitempty"`Parents []*Commit`json:"parents,omitempty"`HTMLURL*string`json:"html_url,omitempty"`URL*string`json:"url,omitempty"`CommentsURL*string`json:"comments_url,omitempty"`Repository*Repository`json:"repository,omitempty"`Score*float64`json:"score,omitempty"`
}
// OUTPUT:// json.Unmarshal returned:// &github.CommitResult{SHA:(*string)(0x1400067c650), Commit:(*github.Commit)(0x140017b0e00), Author:(*github.User)(0x140017ec9c0), Committer:(*github.User)(0x140017ecb60), Parents:[]*github.Commit(nil), HTMLURL:(*string)(0x1400067c670), URL:(*string)(0x1400067c680), CommentsURL:(*string)(0x1400067c6b0), Repository:(*github.Repository)(0x140017aae08), Score:(*float64)(0x14001789e00)}// want:// "{\n\t\t\"sha\": \"test\",\n\t\t\"commit\": {\n\t\t \"sha\": \"test\"\n\t\t},\n\t\t\"author\": {\n\t\t \"id\": 1\n\t\t},\n\t\t\"committer\": {\n\t\t \"id\": 1\n\t\t},\n\t\t\"html_url\": \"hurl\",\n\t\t\"url\": \"url\",\n\t\t\"comments_url\": \"curl\",\n\t\t\"repository\": {\n\t\t \"id\": 1\n\t\t},\n\t\t\"score\": 123\n\t }"// diff:// &github.CommitResult{// ... // 2 identical fields// Author: &{ID: &1},// Committer: &{ID: &1},// - Parents: []*github.Commit{},// + Parents: nil,// HTMLURL: &"hurl",// URL: &"url",// ... // 3 identical fields// }

github/packages_test.go::TestPackageEventContainerMetadata_Marshal

typePackageContainerMetadatastruct {
Tags []string`json:"tags,omitempty"`
}
// OUTPUT:// json.Unmarshal returned:// &github.PackageEventContainerMetadata{Labels:map[string]interface {}{"k":"v"}, Manifest:map[string]interface {}{"k":1}, Tag:(*github.PackageEventContainerMetadataTag)(0x140027188e0)}// want:// "{\n\t\t\"labels\": {\n\t\t\t\"k\": \"v\"\n\t\t},\n\t\t\"manifest\": {\n\t\t\t\"k\": 1\n\t\t},\n\t\t\"tag\": {\n\t\t\t\"name\": \"n\",\n\t\t\t\"digest\": \"d\"\n\t\t}\n\t}"// diff:// &github.PackageEventContainerMetadata{// Labels: {"k": string("v")},// - Manifest: map[string]any{"k": int(1)},// + Manifest: map[string]any{"k": float64(1)},// Tag: &{Name: &"n", Digest: &"d"},// }

github/enterprise_actions_runner_groups_test.go::TestEnterpriseRunnerGroup_Marshal

typeEnterpriseRunnerGroupstruct {
ID*int64`json:"id,omitempty"`Name*string`json:"name,omitempty"`Visibility*string`json:"visibility,omitempty"`Default*bool`json:"default,omitempty"`SelectedOrganizationsURL*string`json:"selected_organizations_url,omitempty"`RunnersURL*string`json:"runners_url,omitempty"`Inherited*bool`json:"inherited,omitempty"`AllowsPublicRepositories*bool`json:"allows_public_repositories,omitempty"`RestrictedToWorkflows*bool`json:"restricted_to_workflows,omitempty"`SelectedWorkflows []string`json:"selected_workflows,omitempty"`WorkflowRestrictionsReadOnly*bool`json:"workflow_restrictions_read_only,omitempty"`
}
// OUTPUT:// json.Unmarshal returned:// &github.EnterpriseRunnerGroup{ID:(*int64)(0x1400235c868), Name:(*string)(0x1400368dd10), Visibility:(*string)(0x1400368dd20), Default:(*bool)(0x1400235c870), SelectedOrganizationsURL:(*string)(0x1400368dd30), RunnersURL:(*string)(0x1400368dd60), Inherited:(*bool)(0x1400235c871), AllowsPublicRepositories:(*bool)(0x1400235c872), RestrictedToWorkflows:(*bool)(0x1400235c873), SelectedWorkflows:[]string(nil), WorkflowRestrictionsReadOnly:(*bool)(nil)}// want:// "{\n\t\t\"id\": 1,\n\t\t\"name\": \"n\",\n\t\t\"visibility\": \"v\",\n\t\t\"default\": true,\n\t\t\"selected_organizations_url\": \"s\",\n\t\t\"runners_url\": \"r\",\n\t\t\"inherited\": true,\n\t\t\"allows_public_repositories\": true,\n\t\t\"restricted_to_workflows\": false\n\t}"// diff:// &github.EnterpriseRunnerGroup{// ... // 7 identical fields// AllowsPublicRepositories: &true,// RestrictedToWorkflows: &false,// - SelectedWorkflows: []string{},// + SelectedWorkflows: nil,// WorkflowRestrictionsReadOnly: nil,// }

github/actions_secrets_test.go::TestEncryptedSecret_Marshal

typeEncryptedSecretstruct {
Namestring`json:"-"`KeyIDstring`json:"key_id"`EncryptedValuestring`json:"encrypted_value"`Visibilitystring`json:"visibility,omitempty"`SelectedRepositoryIDsSelectedRepoIDs`json:"selected_repository_ids,omitempty"`
}
// OUTPUT:// json.Unmarshal returned:// &github.EncryptedSecret{Name:"", KeyID:"kid", EncryptedValue:"e", Visibility:"v", SelectedRepositoryIDs:github.SelectedRepoIDs{1}}// want:// "{\n\t\t\"key_id\": \"kid\",\n\t\t\"encrypted_value\": \"e\",\n\t\t\"visibility\": \"v\",\n\t\t\"selected_repository_ids\": [1]\n\t}"// diff:// &github.EncryptedSecret{// - Name: "n",// + Name: "",// KeyID: "kid",// EncryptedValue: "e",// ... // 2 identical fields// }

[stevehipwell]

@exageraldo for the empty slices you should be able to use the cmpopts.EquateEmpty() func to set the diff options. The go-cmp library has support for dealing with unexported fields. For the omitted fields, why are they in the test want?

[alexandear]

Hi @exageraldo, just checking in - is there anything we can do to support you in wrapping up the PR?