Add cmd unittest #2075
Add cmd unittest #2075
Conversation
* bind * unbind * provision * deprovision
* Return the proper exit code when a failure occurs * Print the names of any bindings that did delete sucessfully
* Don't prefix with 'Error:' for ungrouped error messages * Don't capitalize the first letter in the messages
k8s-ci-robot
added
cncf-cla: yes
|
@carolynvs I don't think I can set a reviewer to this, but could you take a look whenever you have some time? |
| name: "describe non existing binding", | ||
| fakeBindings: []string{}, | ||
| bindingName: "mybinding", | ||
| expected: "unable to get binding '" + namespace + ".mybinding': servicebindings.servicecatalog.k8s.io \"mybinding\" not found", |
There was a problem hiding this comment.
Doing an exact full string match is going to make these tests difficult to maintain going forward. We have another set of tests that do full output comparisons on stdout/stderr for the cli.
I recommend that for these tests, we do a strings.Contains check instead, and only specify a small portion of the expected output. That will give us enough to check for error messages, without needing to regularly update these tests as the output changes.
So you could change this to just check for the portion of the error message that is controlled by the CLI: unable to get binding ns/name.
There was a problem hiding this comment.
I agree and latest commit should make the tests work as suggested.
| name: "describe existing binding", | ||
| fakeBindings: []string{"mybinding"}, | ||
| bindingName: "mybinding", | ||
| expected: " Name: mybinding \n Namespace: " + namespace + " \n Status: \n Secret: \n Instance: \n\nParameters:\n No parameters defined\n", |
There was a problem hiding this comment.
For successful cases, I suggest not looking for an expected string and just relying on wantError: false since the full cli output is adequately tested elsewhere.
There was a problem hiding this comment.
Cool, in that case I changed "expected" for "expectedError" and moved the check for only when wantError is true.
cmd/svcat/binding/get_cmd_test.go
Outdated
| wantError: false, | ||
| }, | ||
| { | ||
| name: "get all existing bindings with json output format", |
There was a problem hiding this comment.
These successful --output test cases are already covered by other tests, so we can remove the expectation on their full output.
There was a problem hiding this comment.
Removed them on latest commit
cmd/svcat/binding/get_cmd_test.go
Outdated
| bindingName: "mybinding", | ||
| outputFormat: "unknown", | ||
| expected: "", | ||
| wantError: false, |
There was a problem hiding this comment.
haha, I would have expected this to be wantError: true! You may have found a bug. :-) We can follow up on that after this PR is merged. 💯
There was a problem hiding this comment.
Do you think it is necessary to open an issue to track this?
There was a problem hiding this comment.
Yeah a bug is a good idea. Feel free to make one yourself and reference this test, or I will do that later today.
- Do not check for full output string match in case of success, since this is already tested at other places - Check if, in case of errors, output contains relevant data
|
Thanks a lot for your review @carolynvs, hopefully all issues are addressed with latest commit. Let me know if you think it should be improved some other way. |
|
oops! I forgot that you need a rebase before we can merge, now that #2049 has been merged. I'll put a LGTM on this once that's in. |
carolynvs
added
needs-rebase
|
FYI, @artmello can set reviewers with /cc command and approvers with /assign. |
|
New changes are detected. LGTM label has been removed. |
k8s-ci-robot
removed
lgtm
k8s-ci-robot
added
the
size/L
carolynvs
removed
the
needs-rebase
|
LGTM, and this time I mean it! 😊 |
* Add svcat output tests for waitable commands * bind * unbind * provision * deprovision * Fix writing deleted bindings when --wait isn't set * don't use glog, it's not test friendly * Fix handling when not all binding deletes fail * Return the proper exit code when a failure occurs * Print the names of any bindings that did delete sucessfully * Wrap errors to preserve type checking later * Make error messages consistent with latest version of cobra * Don't prefix with 'Error:' for ungrouped error messages * Don't capitalize the first letter in the messages * appeasing the boilerplate gods * Fix testdata so that its not racey * Update test output with recently merged changes from master * Add unittest for srvcat get and describe command * Update tests for describe and get svcat commands - Do not check for full output string match in case of success, since this is already tested at other places - Check if, in case of errors, output contains relevant data
Based on the work done by @carolynvs at #2049 this PR add unit tests for get and describe commands.