Change testJSONMarshal by exageraldo · Pull Request #2708 · google/go-github

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

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

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.16%. Comparing base (77b5b3d) to head (d29f932).
Report is 346 commits behind head on master.

Additional details and impacted files

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

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

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

…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


-	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

+	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

 	want := `{
- "reason": "reason",
- "created_at": ` + referenceTimeStr + `
-	}`
-
-	testJSONMarshal(t, u, want)
-}
-
-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

 	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

 	TextMatch bool `url:"-"`

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


This also seems odd - why is this needed?

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

 	Role string `url:"role,omitempty"`

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


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

 	// 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.functestJSONMarshal(t*testing.T, vinterface{}, wantstring) {
t.Helper()
got, err:=json.Marshal(v)
iferr!=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))
ifdiff!="" {
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.funcnormalizeJSON(t*testing.T, b []byte) []byte {
t.Helper()
varvinterface{}
err:=json.Unmarshal(b, &v)
iferr!=nil {
t.Errorf("Unable to unmarshal JSON for %v: %v", string(b), err)
}
w, err:=json.MarshalIndent(v, "", " ")
iferr!=nil {
t.Errorf("Unable to marshal JSON for %#v", v)
}
returnw
}

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

gmlewis · 2025-01-22T01:26:26Z

I will close this PR around the end of January, 2025 as "abandoned" if there is no further response.

exageraldo · 2025-01-28T12:16:06Z

I'll organize the branch to push up something like @WillAbides suggested!

exageraldo · 2025-03-17T22:15:59Z

@WillAbides 's solution worked very well! Thanks a lot!
I "messed up" this branch a lot and ended up creating another one here #3519
Now I'm working on fixing all the tests that broke.

Maybe we can close this PR.

gmlewis · 2025-03-17T22:28:11Z

@WillAbides 's solution worked very well! Thanks a lot! I "messed up" this branch a lot and ended up creating another one here #3519 Now I'm working on fixing all the tests that broke.
Maybe we can close this PR.

SGTM.
Closing as obsolete.

Change testJSONMarshal and fix all tests related
2e1528e

Add a proposal to improve json visibility
2f4e17b

exageraldo commented Mar 21, 2023
View reviewed changes

Comment threadgithub/actions_artifacts_test.go Outdated

exageraldo commented Mar 22, 2023
View reviewed changes

Comment threadgithub/activity_notifications.go Outdated

exageraldo commented Mar 22, 2023
View reviewed changes

Comment threadgithub/github.go

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 testJSONMarshalMar 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

gmlewis closed this Mar 17, 2025

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

Uh oh!

Conversation

exageraldo commented Mar 15, 2023

Uh oh!

codecovBot commented Mar 15, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gmlewis commented Mar 16, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

exageraldo commented Mar 22, 2023

Uh oh!

exageraldo commented Mar 22, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmlewis commented Mar 23, 2023

Uh oh!

gmlewis commented Sep 18, 2023

Uh oh!

exageraldo commented Sep 18, 2023

Uh oh!

gmlewis commented Sep 18, 2023

Uh oh!

exageraldo commented Oct 7, 2023

Uh oh!

gmlewis commented Oct 10, 2023

Uh oh!

exageraldo commented Oct 10, 2023

Uh oh!

gmlewis commented Oct 10, 2023

Uh oh!

gmlewis commented Oct 10, 2023

Uh oh!

gmlewis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WillAbides commented Oct 11, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmlewis commented Jan 22, 2025

Uh oh!

exageraldo commented Jan 28, 2025

Uh oh!

exageraldo commented Mar 17, 2025

Uh oh!

gmlewis commented Mar 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecovBot commented Mar 15, 2023•
edited
Loading

exageraldo commented Mar 22, 2023•
edited
Loading

WillAbides commented Oct 11, 2023•
edited
Loading