Fix ErrorToStringArray empty output handling - #29534
Conversation
Honny1
left a comment
Honny1
left a comment
There was a problem hiding this comment.
LGTM, Thanks!
PTAL @podman-container-tools/podman-reviewers @podman-container-tools/podman-maintainers
|
Please add a commit message indicating why these changes were necessary |
9c4aaf7 to
eb8ee2b
Compare
|
I mean, this function has 2 callers and this PR removes one of them … (is removing it without replacement correct??) Its very existence might all be a bit of an over-engineering. |
|
this is a test utility. not sure this kind of thing needs its own PRs ? agree with adding a commit message bc my guess this is all theoretical ... |
strings.Split(output, "\n") on empty output returns [""], not []. so ErrorToStringArray() reported empty stderr as one line of empty output instead of no output, and any caller checking len() got a wrong count. volume_ls_test.go had to carry HaveLen(1) just to tolerate that on empty stderr, updated to BeEmpty() now that the length is actually correct. filter out empty lines when building the result. Signed-off-by: Atishyy27 <142108881+Atishyy27@users.noreply.github.com>
eb8ee2b to
48de0cd
Compare
|
you're right, that assertion should have been updated not deleted. restored it in volume_ls_test.go, now checking BeEmpty() instead of HaveLen(1), which is the correct value once the function is fixed. sorry, that was sloppy on my end. on over-engineering: fair, but that's the existing function's design, not something this PR adds. this PR only fixes what it returns on empty input. can open a separate issue if you want to discuss whether the function should exist at all. @baude it's a one-line behavior bug, empty stderr was reported as one line of empty output instead of zero, with a real caller that had to work around it. read as a normal bugfix-sized PR to me, no objection either way if you'd rather fold it into something else. |
5 participants
ErrorToStringArray currently returns an empty string as an entry when stderr is completely empty because splitting an empty string on a newline produces a one-element result. This makes empty stderr appear as if error output was produced.
This change filters out empty lines before returning the result. It also updates the existing test expectation, adds coverage for empty output, and updates the HaveLen(1) assertion in volume_ls_test.go to BeEmpty(), since that's the correct value now that the underlying bug is fixed.