Add ginkgo tests for svcat get classes #2230
Conversation
k8s-ci-robot
added
size/L
|
This looks good. Are we keeping the old tests around? They just seem kinda weird to me because they're more like integration tests than units (they go all the way down through the pkg lib to a faked out clientset). Should we move them somewhere else? I do think we should have integration tests. |
|
I think that they really belong in the pkg/svcat/service-catalog package as unit tests. I'll move them there and switch them to use ginkgo too for consistency. Separate PR or this one? |
cmd/svcat/class/get_cmd_test.go
Outdated
| Expect(cmd.name).To(Equal("mysqldb")) | ||
| }) | ||
| }) | ||
| Describe("getAll", func() { |
There was a problem hiding this comment.
generally you don't have to test private methods, as they should be subsumed in whatever public methods use them. I would change this to test Run(), and control the different cases by modifying whether you pass in a uuid or name.
There was a problem hiding this comment.
What's wrong with a unit test testing a single function?
There was a problem hiding this comment.
it's not the testing a single function, it's that it's a private function - if the content of getAll was just inlined in Run, it wouldn't make sense to try to test the functionality of that block specifically, and because it's private, it kinda is just inlined.
This wouldn't be a big change, just switch the describe to "Run", and change the calls to to getAll with calls to Run with the appropriate fields filled in.
There was a problem hiding this comment.
Add a comment to the test about not worrying about it if it breaks. I'm not hung up on idealogical test purity that says you only test public methods.
There was a problem hiding this comment.
Why would we add a test that we don't care if it breaks? And you do test private methods, but implicitly. A test for Run(), which I think we should definitely have, would be 99% the same lines of code as the tests she's written here for getAll() and a hypothetical test for get()
| Expect(output).To(ContainSubstring("postgresdb")) | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Would also be nice to have a set of cases for when you're just getting a single class
There was a problem hiding this comment.
That is the next PR. This change was only for getting a list of classes. 😀
|
The ginkgo tests I added in pkg/svcat/service-catalog already do that, though (fake out the clientset and call out to it). |
|
Oh! Okay I'll compare what's there with these that we think that maybe duplicates and see if they add any new test cases or can be dropped. Expect a follow-up PR. In the meantime, I'm fixing up the test to call Run instead of getAll |
|
@jberkhahn I've switched to testing Run vs. getAll. |
|
/retest |
|
/lgtm |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jboyd01 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
This is a follow-up on #2116. I had originally only written tests for error handling cases, this fills that in to use the new pattern that @jberkhahn has set for our command tests.