Change testJSONMarshal #2708

exageraldo · 2023-03-15T23:53:37Z

codecov · 2023-03-15T23:56:40Z

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 · 2023-03-16T00:51:10Z

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

github/actions_artifacts_test.go

github/activity_notifications.go

github/github.go

…ces before comparison (inside testJSONMarshal)

…itError_Marshal and TestAbuseRateLimitError_Marshal

exageraldo · 2023-03-22T21:38:59Z

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 · 2023-03-22T22:27:20Z

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 · 2023-03-23T00:33:05Z

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

…like '<' and '>'

gmlewis · 2023-09-18T20:57:14Z

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 · 2023-09-18T21:43:19Z

I'll fix these things by this weekend 😄

gmlewis · 2023-09-18T22:57:33Z

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

exageraldo · 2023-10-07T20:12:58Z

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 · 2023-10-10T15:54:03Z

Why are you changing GitHub workflows?

exageraldo · 2023-10-10T16:03:19Z

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 · 2023-10-10T16:16:52Z

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

gmlewis · 2023-10-10T17:48:25Z

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

gmlewis

Wow, thank you, @exageraldo !
A have a few questions...

gmlewis · 2023-10-10T17:56:26Z

github/github_test.go

 	}

-	if diff := cmp.Diff(string(w), string(got)); diff != "" {
-		t.Errorf("json.Marshal returned:\n%s\nwant:\n%s\ndiff:\n%v", got, w, diff)
+	// Remove all spaces and new lines from the JSON strings.


Suggested change

// Remove all spaces and new lines from the JSON strings.

// Remove all tabs and newlines from the JSON strings.

gmlewis · 2023-10-10T17:57:31Z

github/github_test.go

+	want = strings.Replace(want, "\t", "", -1)
+	want = strings.Replace(want, "\n", "", -1)
+
+	// Replace the "<" and ">" characters with their unicode escape sequences.


This seems odd - why was this necessary?

gmlewis · 2023-10-10T17:59:17Z

github/github_test.go

-func TestRateLimitError_Marshal(t *testing.T) {
-	testJSONMarshal(t, &RateLimitError{}, "{}")
-
-	u := &RateLimitError{
-		Rate: Rate{
-			Limit:     1,
-			Remaining: 1,
-			Reset:     Timestamp{referenceTime},
-		},
-		Message: "msg",
-	}
-
-	want := `{
-		"Rate": {
-			"limit": 1,
-			"remaining": 1,
-			"reset": ` + referenceTimeStr + `
-		},
-		"message": "msg"
-	}`
-
-	testJSONMarshal(t, u, want)
-}
-
-func TestAbuseRateLimitError_Marshal(t *testing.T) {
-	testJSONMarshal(t, &AbuseRateLimitError{}, "{}")
-
-	u := &AbuseRateLimitError{
-		Message: "msg",
-	}
-
-	want := `{
-		"message": "msg"
+		"reason":"reason",
+		"created_at":` + referenceTimeStr + `
 	}`

 	testJSONMarshal(t, u, want)
 }


Why were these removed?

wow. It wasn't supposed to happen.
I think it was when I sync the branch. I'll add it again!

gmlewis · 2023-10-10T18:01:07Z

github/orgs_audit_log.go

@@ -16,7 +16,7 @@ type GetAuditLogOptions struct {
 	Include *string `url:"include,omitempty"` // Event type includes. Can be one of "web", "git", "all". Default: "web". (Optional.)
 	Order   *string `url:"order,omitempty"`   // The order of audit log events. Can be one of "asc" or "desc". Default: "desc". (Optional.)

-	ListCursorOptions
+	ListCursorOptions `url:",omitempty"`


This seems odd - why is this necessary for an embedded struct?

gmlewis · 2023-10-10T18:05:03Z

github/search.go

@@ -54,7 +54,7 @@ type SearchOptions struct {
 	// Whether to retrieve text match metadata with a query
 	TextMatch bool `url:"-"`

-	ListOptions
+	ListOptions `url:",omitempty"`


This also seems odd - why is this needed?

gmlewis · 2023-10-10T18:05:33Z

github/teams_members.go

@@ -17,7 +17,7 @@ type TeamListTeamMembersOptions struct {
 	// values are "all", "member", "maintainer". Default is "all".
 	Role string `url:"role,omitempty"`

-	ListOptions
+	ListOptions `url:",omitempty"`


gmlewis · 2023-10-10T18:05:52Z

github/users.go

@@ -196,7 +196,7 @@ type UserListOptions struct {
 	// Note: Pagination is powered exclusively by the Since parameter,
 	// ListOptions.Page has no effect.
 	// ListOptions.PerPage controls an undocumented GitHub API parameter.
-	ListOptions
+	ListOptions `url:",omitempty"`


WillAbides · 2023-10-11T19:03:00Z

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().

Change testJSONMarshal and fix all tests related

2e1528e

Add a proposal to improve json visibility

2f4e17b

exageraldo commented Mar 21, 2023

View reviewed changes

github/actions_artifacts_test.go Outdated Show resolved Hide resolved

exageraldo commented Mar 22, 2023

View reviewed changes

github/activity_notifications.go Outdated Show resolved Hide resolved

exageraldo commented Mar 22, 2023

View reviewed changes

github/github.go Outdated Show resolved Hide resolved

exageraldo added 2 commits March 22, 2023 14:07

Improve the formatting of the json strings and remove unnecessary spa…

c45dba4

…ces before comparison (inside testJSONMarshal)

Improve some json strings, revert some changes and remove TestRateLim…

f34880a

…itError_Marshal and TestAbuseRateLimitError_Marshal

Reverse change

18c1f90

Add json.Marshal to convert the value

083760e

exageraldo added 2 commits March 23, 2023 20:41

Create testStructTags func and add json.Marshal doc

6c047e5

Fix some json strings

1e572a4

exageraldo changed the title ~~[WIP] Change testJSONMarshal~~ Change testJSONMarshal Mar 24, 2023

exageraldo marked this pull request as ready for review March 24, 2023 00:22

exageraldo added 3 commits March 23, 2023 22:15

Remove unnecessary '&' when passing the value (it was being used twice)

84004de

Add a workaround to find out whether the interface is a map or not

ed6221d

Change to testJSONMarshal the responsibility for handling characters …

27eb1af

…like '<' and '>'

gmlewis added the waiting for reply label Jul 27, 2023

Update branch

625d87e

Improve doc

d29f932

gmlewis reviewed Oct 10, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change testJSONMarshal #2708

Change testJSONMarshal #2708

exageraldo commented Mar 15, 2023

codecov bot commented Mar 15, 2023 •

edited

gmlewis commented Mar 16, 2023

exageraldo commented Mar 22, 2023

exageraldo commented Mar 22, 2023 •

edited

gmlewis commented Mar 23, 2023

gmlewis commented Sep 18, 2023

exageraldo commented Sep 18, 2023

gmlewis commented Sep 18, 2023

exageraldo commented Oct 7, 2023

gmlewis commented Oct 10, 2023

exageraldo commented Oct 10, 2023

gmlewis commented Oct 10, 2023

gmlewis commented Oct 10, 2023

gmlewis left a comment

gmlewis Oct 10, 2023

gmlewis Oct 10, 2023

gmlewis Oct 10, 2023

exageraldo Oct 10, 2023

gmlewis Oct 10, 2023

gmlewis Oct 10, 2023

gmlewis Oct 10, 2023

gmlewis Oct 10, 2023

WillAbides commented Oct 11, 2023 •

edited

	// Remove all spaces and new lines from the JSON strings.
	// Remove all tabs and newlines from the JSON strings.

Change testJSONMarshal #2708

Are you sure you want to change the base?

Change testJSONMarshal #2708

Conversation

exageraldo commented Mar 15, 2023

codecov bot commented Mar 15, 2023 • edited

Codecov Report

gmlewis commented Mar 16, 2023

exageraldo commented Mar 22, 2023

exageraldo commented Mar 22, 2023 • edited

gmlewis commented Mar 23, 2023

gmlewis commented Sep 18, 2023

exageraldo commented Sep 18, 2023

gmlewis commented Sep 18, 2023

exageraldo commented Oct 7, 2023

gmlewis commented Oct 10, 2023

exageraldo commented Oct 10, 2023

gmlewis commented Oct 10, 2023

gmlewis commented Oct 10, 2023

gmlewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAbides commented Oct 11, 2023 • edited

codecov bot commented Mar 15, 2023 •

edited

exageraldo commented Mar 22, 2023 •

edited

WillAbides commented Oct 11, 2023 •

edited