Search code, repositories, users, issues, pull requests...


 Add @WillAbides suggestion

6a4ac46


 Fix TestUpdateAttributeForSCIMUserOptions_Marshal

047ea57


 Fix tests

4ccd157


 Add test case in TestAuditEntry_Marshal

845e916


 Add TestAuditEntry_Unmarshal

5eb1677

@exageraldo exageraldo force-pushed the improve-testJSONMarshal branch from 07dba43 to 5eb1677 Compare

March 18, 2025 17:50

@gmlewis

Copy link

Collaborator

gmlewis commented Apr 7, 2025

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


 Merge branch 'master' into improve-testJSONMarshal

0ded469

@exageraldo exageraldo marked this pull request as ready for review

April 23, 2025 23:54

exageraldo

exageraldo commented

Apr 23, 2025

github/orgs_audit_log_test.go Show resolved Hide resolved

exageraldo

exageraldo commented

github/actions_cache_test.go Show resolved Hide resolved

gmlewis

gmlewis approved these changes

Copy link

Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@gmlewis gmlewis added the NeedsReview label

stevehipwell

stevehipwell suggested changes

Copy link

Contributor

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

func testJSONMarshal(t *testing.T, v any, want string) {
	t.Helper()
	gotBytes, err := json.Marshal(v)
	if err != nil {
		t.Errorf("Unable to marshal JSON for %#v", v)
	}
	if diff := cmp.Diff(want, string(gotBytes)); diff != "" {
		t.Errorf(
			"json.Marshal returned:\n%s\nwant:\n%s\ndiff:\n%v",
			gotBytes,
			want,
			diff,
		)
	}
	var gotAny any
	err = json.Unmarshal([]byte(want), gotAny)
	if err != nil {
		t.Errorf("Unable to unmarshal JSON %v: %v", want, err)
	}
	if diff := 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.

Copy link

Contributor Author

exageraldo commented Apr 24, 2025

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

Copy link

Collaborator

gmlewis commented Apr 24, 2025

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

exageraldo commented

go-github/github/github.go

github/users_test.go

Comment on lines 513 to 517

want := `{

"since" : 1900,

"page": 1,

"perPage": 10

"Since" : 1900,

"Page": 1,

"PerPage": 10

Copy link

Contributor Author

@exageraldo exageraldo Apr 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link

Collaborator

@gmlewis gmlewis Apr 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Line 250 in 04274a9

PerPage int `url:"per_page,omitempty"`

which looks like this:

	PerPage int `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.

Copy link

Collaborator

@gmlewis gmlewis Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Contributor Author

@exageraldo exageraldo Apr 25, 2025 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

func testURLParseValues(t *testing.T, v interface{}, want string) {
	values, err := query.Values(v)
	if err != nil {
		t.Errorf("Unable to parse URL values for %#v: %v", v, err)
	}
	got := values.Encode()
	if got != 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
// BEFORE
func TestHovercardOptions_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)
}
// AFTER
func TestHovercardOptions_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.

Copy link

Collaborator

@gmlewis gmlewis Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Collaborator

@gmlewis gmlewis Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Contributor

@stevehipwell stevehipwell Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Collaborator

@gmlewis gmlewis Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link

Contributor Author

@exageraldo exageraldo Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Collaborator

@gmlewis gmlewis Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Contributor

stevehipwell commented Apr 25, 2025

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.

func testJSONMarshal[T any](t *testing.T, v T, want string) {
	t.Helper()
	testJSONMarshalMarshal(t, v, want)
	testJSONMarshalUnmarshal(t, want, v)
}
func testJSONMarshalMarshal[T any](t *testing.T, v T, want string) {
	t.Helper()
	got, err := json.Marshal(v)
	if err != nil {
		t.Errorf("Unable to marshal got JSON for %#v", v)
	}
	var gotAny, wantAny any
	if err = json.Unmarshal(got, &gotAny); err != nil {
		t.Errorf("Unable to unmarshal want JSON %v: %v", got, err)
	}
	if err = json.Unmarshal([]byte(want), &wantAny); err != nil {
		t.Errorf("Unable to unmarshal JSON %v: %v", want, err)
	}
	if diff := cmp.Diff(wantAny, gotAny); diff != "" {
		t.Errorf(
			"json.Marshal returned:\n%s\nwant:\n%s\ndiff:\n%v",
			got,
			want,
			diff,
		)
	}
}
func testJSONMarshalUnmarshal[T any](t *testing.T, v string, want T) {
	t.Helper()
	var got T
	err := json.Unmarshal([]byte(v), &got)
	if err != nil {
		t.Errorf("Unable to unmarshal JSON %v: %v", want, err)
	}
	if diff := cmp.Diff(want, got); diff != "" {
		t.Errorf(
			"json.Unmarshal returned:\n%#v\nwant:\n%#v\ndiff:\n%v",
			got,
			v,
			diff,
		)
	}
}