-
Notifications
You must be signed in to change notification settings - Fork 137
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.
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
Add pagination and tests to Clients and Connections endpoints #113
Conversation
}.not_to raise_error | ||
end | ||
|
||
it 'is expected to add pagination' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is asserting something about adding pagination, but only tests that an error wasn't raised. Is there an indicator that the params weren't silently ignored?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That first expect
line sets up the second one. If you change the page
value in with()
to a different value, it fails.
Right, but I'm suggesting that code wise I cannot tell that what is being tested is ensuring pagination works.
|
@machuga - As in, you can't tell if the results themselves are paginated? I can add an integration test for that. But, for a unit test, I think this works ok. It was a bit of trial and error to get it to work but changing the values of, say, the |
Ya I think the test name just implies that something else is being tested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
:page
and:per_page
method parameters toAuth0::Api::V2::Clients
andAuth0::Api::V2::Connections
modules.Array
for thefields:
parameter and convert to comma-sep string