🐛 (go/v4): Refactor e2e-tests for readable logs #4159
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
|
Hi @mogsie. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mogsie The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
mogsie
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
utils.go has a function to execute a command, and it returns the output if the command is successful. It returns a byte array, the same as the underlying exec.Cmd() provides. This, being a test heper function would make sense to return a string instead, so that the ergonomics of using Gomega assertions on strings work better by default. Removed the now unneccesary cast from []byte to string. Found an additional place where byte arrays were being asserted against ContainsSubstring (plugin_cluser_test.go).
mogsie
force-pushed
the
remove-byte-array-output
branch
from
05da7bc
to
f40a94b
Compare
| @@ -54,10 +54,10 @@ func Run(cmd *exec.Cmd) ([]byte, error) { | |||
| _, _ = fmt.Fprintf(GinkgoWriter, "running: %s\n", command) | |||
| output, err := cmd.CombinedOutput() | |||
| if err != nil { | |||
| return output, fmt.Errorf("%s failed with error: (%v) %s", command, err, string(output)) | |||
| return "", fmt.Errorf("%s failed with error: (%v) %s", command, err, string(output)) | |||
There was a problem hiding this comment.
Then, we are no longer return the output when we we should here.
We need return the output.
There was a problem hiding this comment.
The output is in the error message, though. Generally the return values are zero-valued, or the error is nil. This is what Gomega checks, and so I replaced it with "" to adhere to the go idioms.
| @@ -54,10 +54,10 @@ func Run(cmd *exec.Cmd) ([]byte, error) { | |||
| _, _ = fmt.Fprintf(GinkgoWriter, "running: %s\n", command) | |||
| output, err := cmd.CombinedOutput() | |||
| if err != nil { | |||
| return output, fmt.Errorf("%s failed with error: (%v) %s", command, err, string(output)) | |||
| return "", fmt.Errorf("%s failed with error: (%v) %s", command, err, string(output)) | |||
There was a problem hiding this comment.
we should return the output always.
| @@ -110,7 +110,7 @@ var _ = Describe("Manager", Ordered, func() { | |||
| "pods", controllerPodName, "-o", "jsonpath={.status.phase}", | |||
| "-n", namespace, | |||
| ) | |||
| g.Expect(utils.Run(cmd)).To(BeEquivalentTo("Running"), "Incorrect controller-manager pod status") | |||
| g.Expect(utils.Run(cmd)).To(Equal("Running"), "Incorrect controller-manager pod status") | |||
There was a problem hiding this comment.
Just for curiosity,
what is the diff between BeEquivalentTo and Equal do you know?
There was a problem hiding this comment.
Equal checks for equality while BeEquivalentTo does some type conversion, like between string and []byte. Unfortunately, it still prints out the byte array representation. Equals doesn't allow a byte array to be compared to a string.
My recent PR #4141 introduced a problem with error reporting. Previously, error resporting would contain the output of the command that it was matching against (e.g. a substring). After #4141, a byte array was being printed out, making it unreadable. This PR fixes that, making the error output readable again.
utils.go has a function to execute a command, and it returns the output if the command is successful. It returns a byte array, the same as the underlying exec.Cmd() provides. This, being a test heper function would make sense to return a string instead, so that the ergonomics of using Gomega assertions on strings work better by default.
Removed the now unneccesary cast from []byte to string.
Shamelessly stole the title from #4158.
Closes #4158
Part of #4135