test/e2e: Fix 'resource name may not be empty' failure in catalog templating test case #2457
Conversation
…plating test case Update the 'adding catalog template adjusts image used' test case and fix the issue where get requests for the test catalogsource were failing due to the source.GetName() and source.GetNamespace() being empty, and therefore the test catalogsource could never be updated. Updating the test logic to avoid overwriting the `source` variable during every poll by suppressing the variable returned appeared to be the culprit. This was consistently reproducible on a cluster when focusing on this single test and running the e2e suite directly. After making these changes, I wasn't able to reproduce locally anymore. Signed-off-by: timflannagan <[email protected]>
…the catalog e2e package Signed-off-by: timflannagan <[email protected]>
timflannagan
force-pushed
the
test/fix-resource-name-may-not-empty
branch
from
14332e6 to
c420158
Compare
|
I'd recommend reviewing by the commits vs. the full diff as I sneaked in another commit to replace context.TODO -> context.Background commit too. |
|
More context here as well: #2397 (comment) |
|
/approve |
openshift-ci
bot
added
the
approved
| @@ -1099,7 +1105,7 @@ var _ = Describe("Catalog represents a store of bundles which OLM can use to ins | |||
| } | |||
|
|
|||
| return false, nil | |||
| }, 5*time.Minute, 1*time.Second).Should(BeTrue()) | |||
| }).Should(BeTrue()) | |||
|
|
|||
| // source should be the latest we got from the eventually block | |||
| Expect(source.Status.Conditions).ToNot(BeNil()) | |||
There was a problem hiding this comment.
nit: I suspect we could refactor most of the rest of this test into a matcher for the previous Eventually statement.
| source.SetAnnotations(map[string]string{ | ||
| catalogsource.CatalogImageTemplateAnnotation: fmt.Sprintf("quay.io/olmtest/catsrc-update-test:%s.%s.%s", catalogsource.TemplKubeMajorV, catalogsource.TemplKubeMinorV, catalogsource.TemplKubePatchV), | ||
| }) |
There was a problem hiding this comment.
nit: we could define the map literal outside of this closure and set it here too
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kevinrizza, njhale, timflannagan 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 |
|
/lgtm |
Description of the change:
Update the 'adding catalog template adjusts image used' test case and
fix the issue where get requests for the test catalogsource were failing
due to the source.GetName() and source.GetNamespace() being empty, and
therefore the test catalogsource could never be updated.
Updating the test logic to avoid overwriting the
sourcevariableduring every poll by suppressing the variable returned appeared to be
the culprit. This was consistently reproducible on a cluster when
focusing on this single test and running the e2e suite directly.
After making these changes, I wasn't able to reproduce locally anymore.
Motivation for the change:
Reducing e2e flakes overtime.
Reviewer Checklist
/doc