Fix Service.OwnedResources() to return correct result #975
Conversation
pedjak
force-pushed
the
fix-wrong-detected-ownded-resources
branch
from
7e6bf7e
to
7367583
Compare
|
/retest |
pedjak
force-pushed
the
fix-wrong-detected-ownded-resources
branch
from
7367583
to
57d00f2
Compare
|
/retest |
Codecov Report
@@ Coverage Diff @@
## master #975 +/- ##
==========================================
+ Coverage 54.27% 55.56% +1.28%
==========================================
Files 23 23
Lines 1227 1240 +13
==========================================
+ Hits 666 689 +23
+ Misses 473 462 -11
- Partials 88 89 +1
Continue to review full report at Codecov.
|
|
If no plan to write an acceptance test as part of this PR, please create an issue to track it. |
| @@ -43,8 +47,55 @@ var _ = Describe("Service", func() { | |||
| Expect(res).To(BeNil()) | |||
| }) | |||
|
|
|||
| It("should return owned resources", func() { | |||
There was a problem hiding this comment.
There is a scenario where the OwnedResources could return an error. A unit test would be nice.
Use this function to return error: https://pkg.go.dev/k8s.io/client-go/testing#Fake.PrependReactor
There is an example in PR #971
| var children []interface{} | ||
|
|
||
| for _, gvr := range bindableResourceGVRs { | ||
| gvk := gvr.GroupVersion().WithKind(strings.Title(gvr.Resource)[:len(gvr.Resource)-1]) |
There was a problem hiding this comment.
The title form Configmap is not equivalent to ConfigMap. But I think it's ok for testing. An acceptance test would be better to capture a real-world scenario.
There was a problem hiding this comment.
good catch - I have modified the test to exclude configmaps and check additional path when List returns not found error.
pedjak
force-pushed
the
fix-wrong-detected-ownded-resources
branch
from
57d00f2
to
8ef84c0
Compare
@baijum IMHO, constructing a reliable, deterministic acceptance test for this bug is impossible, because the bug is triggered by the order of resources returned by cluster API, and we cannot control that in practice. Fortunately we can control the order in the unit tests. I could agree that writing additional acceptance tests demonstrating |
We were bitten by https://github.com/golang/go/wiki/CommonMistakes#using-reference-to-loop-iterator-variable Fixed the loop to use the new variable. Signed-off-by: Predrag Knezevic <pknezevi@redhat.com>
pedjak
force-pushed
the
fix-wrong-detected-ownded-resources
branch
from
8ef84c0
to
615d956
Compare
|
/lgtm Not adding "approve" now as @pratikjagrut is also looking into this. |
|
/lgtm |
|
/approve |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: baijum, pmacik, pratikjagrut The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
We were bitten by https://github.com/golang/go/wiki/CommonMistakes#using-reference-to-loop-iterator-variable
Fixed the loop to use the new variable.
Fixes #966