🌱 replacing DeepEqual with cmp Diff in tests #8310
Conversation
Signed-off-by: DiptoChakrabarty <[email protected]> wrapper function to compare cmp diff in case panic occurs with log Signed-off-by: DiptoChakrabarty <[email protected]>
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
| @@ -147,7 +147,7 @@ func TestSetResourceBinding(t *testing.T) { | |||
| tt.resourceSetBinding.SetBinding(tt.resourceBinding) | |||
| exist := false | |||
| for _, b := range tt.resourceSetBinding.Resources { | |||
| if reflect.DeepEqual(b.ResourceRef, tt.resourceBinding.ResourceRef) { | |||
| if diff := cmp.Diff(b.ResourceRef, tt.resourceBinding.ResourceRef); diff == "" { | |||
There was a problem hiding this comment.
| if diff := cmp.Diff(b.ResourceRef, tt.resourceBinding.ResourceRef); diff == "" { | |
| if cmp.Equal(b.ResourceRef, tt.resourceBinding.ResourceRef) { |
How about this?
(same in the other 2 clusterresourcesetbingins_types_test.go files)
util/conditions/compare.go
Outdated
| func Check(actual interface{}, expected interface{}) (result bool, err error) { | ||
| defer func() error { | ||
| if recover() != nil { | ||
| log.Printf("panic occured got %v expected %v", actual, expected) |
There was a problem hiding this comment.
I am not sure about the logs here. If the error needs to be logged it should be handled by the function using this util function.
DiptoChakrabarty
force-pushed
the
cmpconv
branch
2 times, most recently
from
bc78e1f
to
88d19fd
Compare
|
/retest |
DiptoChakrabarty
force-pushed
the
cmpconv
branch
from
88d19fd
to
410e461
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
DiptoChakrabarty
force-pushed
the
cmpconv
branch
2 times, most recently
from
51866ac
to
ee27db4
Compare
fix gci lint error
DiptoChakrabarty
force-pushed
the
cmpconv
branch
from
ee27db4
to
14b7314
Compare
|
/retest |
| "github.com/google/go-cmp/cmp" | ||
| ) | ||
|
|
||
| // Check compares the actual and expected values provided using the |
There was a problem hiding this comment.
I'm curious what's the motivation for this? are we unexpectedly panicking somewhere atm?
There was a problem hiding this comment.
@enxebre I added this to catch panic cases if any occurs
it was mentioned in the issue discussion
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. 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/test-infra repository. |
|
what's the status of this PR? |
|
I'm +1 for closing this PR |
|
/close Feel free to reopen if you want to continue this work |
|
@sbueringer: Closed this PR. In response to this:
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/test-infra repository. |
wrapper function to compare cmp diff in case panic occurs with log
What this PR does / why we need it:
replaces reflect.DeepEqual with go-cmp diff function and also creates a wrapper function to perform comparisons that handles panics which can be used for non testing code as suggested in issue discussion. Tests for the same written
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #8186