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.

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

Limit tests #230

Merged
merged 10 commits into from
Sep 12, 2016
Merged

Limit tests #230

merged 10 commits into from
Sep 12, 2016

Conversation

ultimateboy
Copy link
Member

@ultimateboy ultimateboy commented Sep 8, 2016

@deis-bot
Copy link

deis-bot commented Sep 8, 2016

@Joshua-Anderson is a potential reviewer of this pull request based on my analysis of git blame information. Thanks @ultimateboy!

}`)
})

err = cmdr.UsersList(-1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want 1 here instead of -1?

@Joshua-Anderson
Copy link
Contributor

One minor thing I noticed, otherwise LGTM.

@ultimateboy
Copy link
Member Author

Nice catch @Joshua-Anderson. I'm going to spend a bit of time investigating why that was passing as it was written. But fixed.

@Joshua-Anderson
Copy link
Contributor

I'm thinking it was passing because the SDK returned 1 item but the count on the object was set to two. The code doesn't look at the requested limit, it only looks at the effective limits (returned items vs total count).

@Joshua-Anderson
Copy link
Contributor

Could you possibly add limits tests for KeysList()?

@Joshua-Anderson
Copy link
Contributor

I also noticed that we still need tests for limits on app.Logs(). If you want to want to wait and do these in a different PR that's perfectly fine as well.

@ultimateboy
Copy link
Member Author

Thanks @Joshua-Anderson. Somehow missed the keys test. Added.

@Joshua-Anderson
Copy link
Contributor

After my changes to codecov, you need to rebase. Sorry!

@codecov-io
Copy link

codecov-io commented Sep 9, 2016

Current coverage is 26.88% (diff: 100%)

Merging #230 into master will increase coverage by 0.75%

@@             master       #230   diff @@
==========================================
  Files            56         56          
  Lines          3861       3861          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1009       1038    +29   
+ Misses         2719       2684    -35   
- Partials        133        139     +6   

Powered by Codecov. Last update a330059...d883144

@mboersma mboersma added the LGTM2 label Sep 12, 2016
@ultimateboy ultimateboy merged commit e31962c into deis:master Sep 12, 2016
@ultimateboy ultimateboy deleted the limit-tests branch September 12, 2016 20:49
ultimateboy added a commit to ultimateboy/workflow-cli that referenced this pull request Sep 14, 2016
* tests(builds): add test for 'deis builds:list --limit'

* tests(certs): add test for 'deis certs:list --limit'

* tests(domains): add test for 'deis domains:list --limit'

* tests(releases): add test for 'deis releases:list --limit'

* tests(users): add test for 'deis users:list --limit'

* tests(perms): add tests for 'deis perms:list' with and without '--limit'

* fix(tests): correct test user limit

* tests(keys): add test for 'deis keys:list --limit'

* fix(tests): correct test for keys list.

* tests(apps): add tests for 'apps:list' and 'apps:list --limit'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants