Fixed fake client for CVO scenarios test #597
Conversation
Signed-off-by: Arjun Naik <arjun.rn@gmail.com>
Signed-off-by: Arjun Naik <arjun.rn@gmail.com>
| @@ -415,7 +425,7 @@ func TestCVO_StartupAndSyncUnverifiedPayload(t *testing.T) { | |||
| // make the image report unverified | |||
| payloadErr := &payload.UpdateError{ | |||
| Reason: "ImageVerificationFailed", | |||
| Message: fmt.Sprintf("The update cannot be verified: some random error"), | |||
There was a problem hiding this comment.
There is no formatting to be done so removed the fmt.Sprintf call.
|
/retest |
|
/retest |
pkg/cvo/cvo_scenarios_test.go
Outdated
| switch a := action.(type) { | ||
| case clientgotesting.GetAction: | ||
| switch strings.ToLower(action.GetVerb()) { | ||
| case "get": |
There was a problem hiding this comment.
Good catch. Checking for precedent upstream, I see them switching on GetActionImpl, etc. Thoughts on that vs. just using the verb?
There was a problem hiding this comment.
Replaced with the Impls
| @@ -2864,19 +2906,6 @@ func verifyAllStatus(t *testing.T, ch <-chan SyncWorkerStatus, items ...SyncWork | |||
| } | |||
| } | |||
|
|
|||
| func waitFor(t *testing.T, fn func() bool) { | |||
pkg/cvo/cvo_scenarios_test.go
Outdated
| var count int | ||
| for _, a := range actions { | ||
| if a.GetResource().Resource != "clusterversions" { | ||
| t.Fatalf("an resource of type %s was accessed/updated", a.GetResource().Resource) |
There was a problem hiding this comment.
nit: "an resource" -> "resource" .
I was wondering if we could also log the name, but the generic Action doesn't have a GetName(). Although I also see the GetVerb() != "get" below. Maybe we should be explicit about the verbs that count as updates? That would be create, update, and delete? patch? I dunno.
And maybe we can log the name as well, to make this a bit easier to debug if it happens?
There was a problem hiding this comment.
Anything which is not a get would be a mutating change. So I think this is good enough. I've added the entire object into the failure log to help with debugging.
Signed-off-by: Arjun Naik <arjun.rn@gmail.com>
| @@ -59,7 +59,11 @@ func setupCVOTest(payloadDir string) (*Operator, map[string]runtime.Object, *fak | |||
| } else { | |||
| existing.Spec = obj.Spec | |||
| existing.ObjectMeta = obj.ObjectMeta | |||
| obj.Generation++ | |||
There was a problem hiding this comment.
obj is the object received in the request payload. The generation is incremented here but this change is not stored back into the existing object. So removed the line and changed logic so the highest value of generation is used and then incremented.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arjunrn, wking 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 |
openshift-ci
bot
added
the
approved
|
/retest |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
The fake client for the CVO scenarios test uses a switch-case statement to compare the received
Actionpointer type to interfaces for update-action, create-action and get-action. However in go when two interfaces have the same definition(same functions) a concrete type can match against both of them. And becauseclientgotesting.CreateActionandclientgotesting.UpdateActionare basically the same interface the action always matched against the first interface. In this case theCreateAction. So theClusterVersionobject was always recreated and theResourceVersionandGenerationfields in the result object were reset all the time.I have fixed the above issue by using the actual structs
CreateActionImpl,UpdateActionImplandGetActionImplin the switch-case. This means that the test now updates theResourceVersioncorrectly. TheGenerationfield was also not being incremented correctly. The test client now takes the highest value from the received payload and the existing object.I've also added a
verifySingleUpdatefunction to ensure that theClusterVersionis updated only once, even if multiplegetcalls are made to it.