tests: fix rare nil dereference in TestCache
#875
Conversation
openshift-ci
bot
added
the
approved
petr-muller
force-pushed
the
fix-mock-nil-touch
branch
from
aa904d7 to
d5e1925
Compare
|
/test e2e-agnostic |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/override ci/prow/e2e-agnostic-upgrade-into-change |
|
@petr-muller: Overrode contexts on behalf of petr-muller: ci/prow/e2e-agnostic-upgrade-into-change DetailsIn 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. |
DavidHurta
left a comment
DavidHurta
left a comment
There was a problem hiding this comment.
One minor requested change and one even smaller possible nitpick 👍
Other than that, the code looks good to me! I really like the new restructured code. At least for me, it's more readable.
| }, | ||
| { | ||
| name: "two entries, both old evaluatations, clear evaluation winner", | ||
| name: "two entries, both old evaluations, clear evaluation winner", |
There was a problem hiding this comment.
This comment actually refers to lines 97--100 in the file cache_test.go:
name := fmt.Sprintf("condition %d", i)
condition := configv1.ClusterCondition{Type: name}
var previousCall *mock.Call
for j, call := range m.Calls {
if reflect.DeepEqual(call.Condition, condition) {Especially the line:
if reflect.DeepEqual(call.Condition, condition) {Given the changes to the Call structure we would be now calling the reflect.DeepEqual() function on a pointer (call.Condition) and a structure (condition) resulting in the function never returning true since a pointer and a data structure are not deeply equal.
Something like:
condition := &configv1.ClusterCondition{Type: name}should do the trick.
There was a problem hiding this comment.
Great catch 👍
I'll also check whether the test could be made more robust against such silent failure, it seems that it's quite easy to unwittingly make this test silently pass by never passing that DeepEqual.
There was a problem hiding this comment.
Looks like this was the only other place where .Condition was used, so we should be safe now. I also improved the test to fail when the checked condition is not found in the recorded calls (it fails before the fix you suggested).
Looking at `Cache.Match()` implementation, the `c.Condition.Match()` on L133 can be called with `targetCondition=nil` and that seems to be intentional. In tests, `c.Condition` is a mock, which did not expect that pointer to be `nil` and dereferenced it, resulting in an occassional panic in tests like openshift#871 (comment). The fix stores the full pointer (its deepcopy to avoid sharing that memory) in the mock instead. I needed to restructure the tests a little to allow adding a nil parameter testcase
petr-muller
force-pushed
the
fix-mock-nil-touch
branch
from
d5e1925 to
882c63d
Compare
|
/retest |
1 similar comment
|
/retest |
|
/override ci/prow/e2e-agnostic-upgrade-out-of-change |
|
@petr-muller: Overrode contexts on behalf of petr-muller: ci/prow/e2e-agnostic-upgrade-out-of-change DetailsIn 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. |
|
Override seems to be justified, this PR only affects unit tests and shouldn't result in the given error. New changes look good to me 👍 /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Davoska, petr-muller The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@petr-muller: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
4 participants
Looking at
Cache.Match()implementation, thec.Condition.Match()on L133 can be called withtargetCondition=niland that seems to be intentional. In tests,c.Conditionis a mock, which did not expect that pointer to beniland dereferenced it, resulting in an occassional panic in tests like #871 (comment). The fix stores the full pointer (its deepcopy to avoid sharing that memory) in the mock instead.I needed to restructure the tests a little to allow adding a nil parameter testcase