Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Test for marshalling Organizations and make JSON comparison non sensitive to key order #1303

Merged
merged 2 commits into from
Dec 5, 2019
Merged

Test for marshalling Organizations and make JSON comparison non sensitive to key order #1303

merged 2 commits into from
Dec 5, 2019

Conversation

sgarciac
Copy link
Contributor

@sgarciac sgarciac commented Oct 8, 2019 •
edited
Loading

This pull request contains two items:

  1. Modifies the JSON comparison utility for checking marshalling, making it non sensible to key order.This makes it easier to write tests for marshalling, since there is no need to worry about keeping the right order of the JSON keys in the wanted string. It also removes a non working part of the test. I made sure old tests using this utility would still break if given the wrong input.
  2. Adds a new test for marshalling organizations (issue add tests for resource JSON marshalling #55 )

@googlebot googlebot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Oct 8, 2019
@codecov
Copy link

codecov bot commented Oct 8, 2019 •
edited
Loading

Codecov Report

Merging #1303 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1303   +/-   ##
=======================================
  Coverage   73.42%   73.42%           
=======================================
  Files          86       86           
  Lines        6047     6047           
=======================================
  Hits         4440     4440           
  Misses        837      837           
  Partials      770      770

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 740806a...6314cbb. Read the comment docs.

gmlewis
gmlewis requested changes Oct 8, 2019
View reviewed 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, @sgarciac.

Can you please add to your PR description what problem you are attempting to solve here since this PR is not associated with any issue?

github/github_test.go Outdated
@@ -135,32 +134,26 @@ func testBody(t *testing.T, r *http.Request, want string) {
}
}

// Helper function to test that a value is marshalled to JSON as expected.
// Test whether the marshalling of v produces a JSON that corresponds
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's please spell "marshal" and "unmarshal" consistently throughout the PR with a single "l".
Please also terminate the comment sentence with a period.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(also, added the issue I'm helping with to the description of the PR and the commit message)

@sgarciac sgarciac force-pushed the master branch 2 times, most recently from 9850785 to c5c562f Compare October 8, 2019 00:47
gmlewis
gmlewis requested changes Oct 8, 2019
View reviewed 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.

Just a couple more tweaks, please, and then I think we will be ready for a second LGTM.
Thank you, @sgarciac!

github/github_test.go Outdated
@@ -135,32 +134,26 @@ func testBody(t *testing.T, r *http.Request, want string) {
}
}

// Helper function to test that a value is marshalled to JSON as expected.
// Test whether the marshaling of v produces a JSON that corresponds
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's please say "produces JSON" instead of "produces a JSON".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

github/github_test.go Outdated
}
w, _ := json.Marshal(u)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please test the returned err here instead of ignoring it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done! (however, I wonder if it's ever possible for the marshal here to fail, if the immediate unmarshal had no errors)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably not... but we would like to promote good Go programming practices for all developers who take a look at this code, whether they are new to Go or not... and if developers start ignoring error codes when they think there is no possible way that it could error, that opens up a potential world of pain further down the line.

Let's say, for example, we left it as-is, but then someone in the future decided to change the code above this that generated the value of u, but left this line the same. Suddenly, it might be possible that u was malformed, but this line would not catch that problem.

In summary, it is always safest to check returned error values in Go to minimize future negative surprises. Thanks.

@gmlewis gmlewis requested a review from gauntface October 8, 2019 01:51
gmlewis
gmlewis approved these changes Oct 8, 2019
View reviewed 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.

Just one more tiny change, please, and then I think we will be ready for a second LGTM.
Thank you, @sgarciac!

github/github_test.go Outdated
if err != nil {
t.Errorf("Unable to marshal JSON for %v", v)
t.Errorf("Unable to marshal JSON for %v", u)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: please change this from %v to %#v in order to better see the contents of u.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@wesleimp
Copy link
Collaborator

wesleimp commented Dec 5, 2019

👌

@gmlewis
Copy link
Collaborator

gmlewis commented Dec 5, 2019

Thank you, @wesleimp !
Merging.

@gmlewis gmlewis merged commit 81eca3e into google:master Dec 5, 2019
n1lesh pushed a commit to n1lesh/go-github that referenced this pull request Oct 2, 2020
@sgarciac @gmlewis
Test for marshalling Organizations and make JSON comparison non sensi…
a9ce89c 
…tive to key order (google#1303)

Helps google#55.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmlewis gmlewis gmlewis approved these changes

@wesleimp wesleimp wesleimp approved these changes

@gauntface gauntface Awaiting requested review from gauntface

Assignees
No one assigned
Labels
cla: yes Indication that the PR author has signed a Google Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sgarciac @wesleimp @gmlewis @googlebot