Fix SCIM query params #2680
Fix SCIM query params #2680
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Codecov Report
@@ Coverage Diff @@
## master #2680 +/- ##
=======================================
Coverage 98.05% 98.05%
=======================================
Files 131 131
Lines 11457 11457
=======================================
Hits 11234 11234
Misses 152 152
Partials 71 71
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
github/github_test.go
Outdated
| func testAddOptions(t *testing.T, url string, v interface{}, want string) { | ||
| t.Helper() | ||
|
|
||
| u, err := addOptions(url, v) |
There was a problem hiding this comment.
| u, err := addOptions(url, v) | |
| got, err := addOptions(url, v) |
github/github_test.go
Outdated
| } | ||
|
|
||
| if u != want { | ||
| t.Errorf("addOptions(%q, %#v) returned %s, want %s", url, v, u, want) |
There was a problem hiding this comment.
| t.Errorf("addOptions(%q, %#v) returned %s, want %s", url, v, u, want) | |
| t.Errorf("addOptions(%q, %#v) returned %v, want %v", url, v, got, want) |
github/github_test.go
Outdated
| @@ -169,6 +169,19 @@ func testJSONMarshal(t *testing.T, v interface{}, want string) { | |||
| } | |||
| } | |||
|
|
|||
| func testAddOptions(t *testing.T, url string, v interface{}, want string) { | |||
There was a problem hiding this comment.
Since the structures passed in must have "url" tags in the fields (more in addOptions), is it valid to test if the fields have this tag?
Something like this:
st := reflect.TypeOf(v)
for i := 0; i < st.NumField(); i++ {
field := st.Field(i)
if alias, ok := field.Tag.Lookup("url"); ok {
if alias == "" {
// t.Errorf("...")
}
} else {
// t.Errorf("...")
}
}There was a problem hiding this comment.
Sure. Please add a comment before line 172 that describes this requirement so that people will not start adding testAddOptions to every test.
In fact, let's make the function name more descriptive. How about testAddURLOptions ?
There was a problem hiding this comment.
Or maybe testAddURLTags ?
exageraldo
force-pushed
the
scim-query-params
branch
from
dcf58c9 to
f8b4ff5
Compare
| func TestListSCIMProvisionedIdentitiesOptions_Marshal(t *testing.T) { | ||
| testJSONMarshal(t, &ListSCIMProvisionedIdentitiesOptions{}, `{}`) | ||
| func TestListSCIMProvisionedIdentitiesOptions_addOptions(t *testing.T) { | ||
| testJSONMarshal(t, &ListSCIMProvisionedIdentitiesOptions{}, `{ |
There was a problem hiding this comment.
OK, now I understand why you removed this before, as it really is no longer relevant.
Feel free to delete this if you wish, either in this PR or in your upcoming one.
There was a problem hiding this comment.
Thank you, @exageraldo !
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.
|
Thank you, @valbeat ! |
Fixes: #2679