Change testJSONMarshal

[exageraldo]

Fixes: #2699

[codecov[bot]]

Codecov Report

Merging #2708 (d29f932) into master (77b5b3d) will increase coverage by 0.03%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2708      +/-   ##
==========================================
+ Coverage   98.13%   98.16%   +0.03%     
==========================================
  Files         148      148              
  Lines       12783    12783              
==========================================
+ Hits        12544    12548       +4     
+ Misses        164      160       -4     
  Partials       75       75              

Files	Coverage Δ
github/orgs_audit_log.go	100.00% <ø> (ø)
github/search.go	93.61% <ø> (ø)
github/teams.go	98.18% <ø> (ø)
github/teams_members.go	95.23% <ø> (ø)
github/users.go	94.17% <ø> (ø)

... and 1 file with indirect coverage changes

[gmlewis]

Thanks, @exageraldo ! Please remove "Draft" status when you would like me to review this PR.

[exageraldo]

Some tests are no longer _Marshal but _addOtions, because the structures are responsible for mounting query strings, not a json.

TestGetAuditLogOptions_addOptions
TestTrafficBreakdownOptions_addOptions
TestSearchOptions_addOptions
TestTeamListTeamMembersOptions_addOptions
TestListExternalGroupsOptions_addOptions
TestUserListOptions_addOptions
TestHovercardOptions_addOptions

Since we do a tag validation in the testAddURLOptions function, I had to make some small changes to the following structures for the tests to pass.

GetAuditLogOptions ~> ListCursorOptions `url:",omitempty"`
SearchOptions ~> ListOptions `url:",omitempty"`
TeamListTeamMembersOptions ~> ListOptions `url:",omitempty"`
ListExternalGroupsOptions ~> ListOptions `url:",omitempty"`
UserListOptions ~> ListOptions `url:",omitempty"`

Theoretically it is just an explicit way of leaving the structure as it already was (did that make any sense?)

[exageraldo]

In some cases, I had to use json.Marshal to mount part of the expected string.

github/event_types_test.go

part, _ := json.Marshal(">= 2.0.0, < 2.0.2")

github/teams_discussions_test.go

bodyHTML, _ := json.Marshal(`<p>test</p>`)

In both cases, the idea is just to convert ">" into "\u003e" and "<" into "\u003c".

Do you think we can keep it that way, or can we add two other strings.Replace inside the testJSONMarshal function?

want = strings.Replace(want, ">", "\u003e", -1)
want = strings.Replace(want, "<", "\u003c", -1)

I could not find any other simple way to handle this.

---

And now a new case:

github/repos_contents_test.go

contentValue, _ := json.Marshal([]byte{1})

[gmlewis]

Yeah, those cases look a bit odd. Go ahead with what you think is best, and then when I take the time to review the whole PR, I might come up with other ideas. 😄

[gmlewis]

We'll give this PR a couple weeks to get a reply and have the conflicts resolved, then if we haven't got any updates, it will be closed as abandoned.

[exageraldo]

I'll fix these things by this weekend 😄

[gmlewis]

Awesome - @exageraldo - thank you for the update! No rush, actually - I just need to clean out old, abandoned PRs periodically.

[exageraldo]

hey, sorry for the delay. ~i had some unforeseen circumstances here, but~ I'm already finalizing it! by monday I'll be pushing up the changes.

[gmlewis]

Why are you changing GitHub workflows?

[exageraldo]

I didn't change anything in the workflows. I ended up opting for a "merge" instead of a "rebase" because some changes were disappearing and I didn't know how to solve it. If you look in the "Files Changed" tab, you'll see that all the changes are related to tests only.

[gmlewis]

I'll look again when I'm not on my android phone later today.

[gmlewis]

OK, I must have been looking only at a merge commit... looking now and it is looking much better, thanks. Performing code review...

[WillAbides]

We could simplify this by changing testJSONMarshal to this:

// Test whether the marshaling of v produces JSON that corresponds
// to the want string.
func testJSONMarshal(t *testing.T, v interface{}, want string) {
	t.Helper()
	got, err := json.Marshal(v)
	if err != nil {
		t.Errorf("Unable to marshal JSON for %#v", v)
	}
	got = normalizeJSON(t, got)
	wantBytes := normalizeJSON(t, []byte(want))
	diff := cmp.Diff(string(wantBytes), string(got))
	if diff != "" {
		t.Errorf("json.Marshal returned:\n%s\nwant:\n%s\ndiff:\n%v", string(got), string(wantBytes), diff)
	}
}

// normalizeJSON normalizes the JSON in b by unmarshaling and marshaling it
// again.
func normalizeJSON(t *testing.T, b []byte) []byte {
	t.Helper()
	var v interface{}
	err := json.Unmarshal(b, &v)
	if err != nil {
		t.Errorf("Unable to unmarshal JSON for %v: %v", string(b), err)
	}
	w, err := json.MarshalIndent(v, "", "  ")
	if err != nil {
		t.Errorf("Unable to marshal JSON for %#v", v)
	}
	return w
}

That way differences like casing are failures but it isn't required to have the same whitespace or order of fields. This is similar to what testify does with assert.JSONEq().