test: Log input to json.Unmarshal when it fails
#16099
Conversation
pchaigno
added
area/CI-improvement
pchaigno
force-pushed
the
log-non-json-input-for-flake
branch
from
2632ef5
to
742f3dd
Compare
test/helpers/cmd.go
Outdated
| @@ -85,7 +85,7 @@ func (b *CmdStreamBuffer) Filter(filter string) (*FilterBuffer, error) { | |||
|
|
|||
| err := json.Unmarshal(b.Bytes(), &data) | |||
| if err != nil { | |||
| return nil, fmt.Errorf("could not parse JSON from command %q", b.Cmd()) | |||
| return nil, fmt.Errorf("could not parse JSON from command %q\n%s\n%s", b.Cmd(), err, b.Bytes()) | |||
There was a problem hiding this comment.
Sorry one more nit, %w for errors
There was a problem hiding this comment.
That doesn't seem consistent across the codebase. We're using %s, %v, and %w according to git grep -P "fmt.Errorf.*, err". https://github.com/golang/go/wiki/CodeReviewComments also points to %v.
I'm happy to change but not sure to understand the logic. I also don't think it will scale if we rely solely on reviews to catch this.
There was a problem hiding this comment.
@pchaigno It's relatively new and it seems like the code review doc hasn't been updated for it :/
Unfortunately, yes there's inconsistency in the codebase as it sometimes slips by. In this case, consistency is not better--we should just use the correct format specifier when we can :)
Doubly unfortunate in terms of enforcement, we have gone back and forth on this. We used to have some linter that would look at these things, but we removed it because it was too noisy with unhelpful suggestions. @sayboras Do you know if our GH Actions Go linter can pick this up?
json.Unmarshal sometimes fails to parse its input. We should log the input in such cases to figure out what happened. This is currently causing a test to (rarely) flake. Signed-off-by: Paul Chaignon <paul@cilium.io>
pchaigno
force-pushed
the
log-non-json-input-for-flake
branch
from
742f3dd
to
b929023
Compare
|
test-me-please |
|
k8s-1.20-kernel-4.19 failed due to #16154. Other failing tests are not required and not affected by these changes anyway. Reviews are in. Marking as ready to merge. |
pchaigno
added
the
ready-to-merge
json.Unmarshalsometimes fails to parse its input. We should log the input in such cases to figure out what happened. This is currently causing a test to (rarely) flake (cf. #16088).