integration_test: Refine cmd run utilities #582
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
approved
- refactored ITest.Run() to return ([]byte,error) - refactored all Run*** methods to use ITest.Run() This also will help us test commands that are expected to fail but were previously not making their stdout/stderr available to the test case for string tests. Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
integration_test/testutil_test.go
Outdated
| if err := cmd.Run(); err != nil { | ||
| return errors.Wrapf(err, "krew %v", it.args) | ||
| out := b.Bytes() | ||
| return out, errors.Wrapf(err, "krew %v: %v, %s", it.args, err, string(out)) | ||
| } | ||
|
|
||
| it.t.Log("Ran in", time.Since(start)) | ||
| return nil | ||
| return b.Bytes(), nil |
There was a problem hiding this comment.
This can be simpler:
err := cmd.Run()
it.t.Log("Ran in", time.Since(start))
return b.Bytes(), errors.Wrapf(err, "krew %v: %v, %s", it.args, err, string(out))WDYT?
There was a problem hiding this comment.
yeah 👍 seems like it except we need to call b.Bytes() only once
| } | ||
|
|
||
| // RunOrFail runs the krew command and fails the test if the command returns an error. | ||
| func (it *ITest) RunOrFail() { |
There was a problem hiding this comment.
Is that still used? IMO we can also get rid of it and make the t.Fatal(..) more visible in the calling code. But this doesn't have to be here.
There was a problem hiding this comment.
yeah this has 33 use cases, so it helps us save 33 if err := ... ; err != nil { t.fatalf } blocks :)
| func (it *ITest) Run() error { | ||
| // Run runs the krew command, and returns its combined output even when | ||
| // it fails. | ||
| func (it *ITest) Run() ([]byte, error) { |
There was a problem hiding this comment.
| func (it *ITest) Run() ([]byte, error) { | |
| func (it *ITest) Run() (string, error) { |
I could be mistaken, but I think we literally everywhere convert the returned byte array to a string. So we could just move the conversion into this helper.
There was a problem hiding this comment.
RunOrFailOutput is []byte so I kept it consistent w/ that.
We can convert both to string in another PR, wanted to keep this patch short.
Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahmetb, corneliusweig 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 |
This also will help us test commands that are expected to fail but were
previously not making their stdout/stderr available to the test case for
string tests.
Fixes #561
/assign @corneliusweig