New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug 1909836: Re-enable and update operator-install-* tests to add catalog-source #7699
Conversation
@rhamilto: This pull request references Bugzilla bug 1909836, which is invalid:
Comment 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. |
...s/operator-lifecycle-manager/integration-tests-cypress/tests/operator-install-global.spec.ts
Outdated
Show resolved
Hide resolved
9934ab9
to
6d2675b
Compare
...s/operator-lifecycle-manager/integration-tests-cypress/tests/operator-install-global.spec.ts
Outdated
Show resolved
Hide resolved
...s/operator-lifecycle-manager/integration-tests-cypress/tests/operator-install-global.spec.ts
Outdated
Show resolved
Hide resolved
cy.log('verify the packagemanifest has been created'); | ||
cy.visit('/k8s/ns/openshift-marketplace/packages.operators.coreos.com~v1~PackageManifest'); | ||
listPage.filter.byName(operatorPkgName); | ||
listPage.rows.shouldExist(operatorName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So is this where it fails? Times out after 30 seconds (the default timeout)? If so, you might want to add a longer timeout to listPage.rows.shouldExist
(90 seconds) like we did for listPage.rows.shouldNotExist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I load the list of PackageManifests
before all the PackageManifests
are created, they never will all load (I guess we're not watching them here?). I put the wait of 60 seconds in after creation of the CatalogSource
and everything works as expected. I realize this isn't ideal, but I'm trying to just get something that works at the moment.
If we had a CatalogSource
with just the two Operators
we needed for the tests, that would certainly help, but I'm not sure we want to do that since that would require the creation of another image that someone would have to maintain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm...ok, it might be flaky under slower CI, something to watch for.
If your interested :-) https://clearmeasure.com/polling-a-page-in-cypress/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I load the list of
PackageManifests
before all thePackageManifests
are created, they never will all load (I guess we're not watching them here?).
@spadgett, any idea why /k8s/ns/openshift-marketplace/packages.operators.coreos.com~v1~PackageManifest
is not dynamically updating? Seems like a bug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented the polling technique @dtaylor113 mentioned.
a0f797a
to
1ded875
Compare
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests look good to me. Only issue is the Cypress anti-pattern of Having-tests-rely-on-the-state-of-previous-tests.
@@ -104,15 +130,11 @@ xdescribe(`Interacting with a global install mode Operator (${operatorName})`, ( | |||
); | |||
}); | |||
|
|||
it(`uninstalls the operator`, () => { | |||
cy.log('navigate to the operator uninstall modal in OperatorHub'); | |||
it(`uninstalls the Operator`, () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @rhamilto, we are running into the Cypress anti-pattern of Having-tests-rely-on-the-state-of-previous-tests. Here you have an it
to create the operator, another it
to test it's details, and a final it
to uninstall the operator. While this seems very logical, it is frowned upon by the Cypress gods :-) All its
should be able to run independently; you should be able to specify a its.only
for any test and it should be able to run.
To resolve I think these three tests should be in a single it
test with alot of cy.log
statements.
From Cypress best practices about this:
"This ... is ideal because now we are resetting the state between each test and ensuring nothing in previous tests leaks into subsequent ones.
We’re also paving the way to make it less complicated to write multiple tests against the “default” state of the form. That way each test stays lean but each can be run independently and pass."
I think this also may come into play when/if we ever get to running tests in parallel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not opposed to this, but I was trying to maintain focus on getting the tests re-enabled. This would need to be one it
in order to comply with the best practice since the first it
installs the operator.
}); | ||
|
||
it(`uninstalls the operator from ${testName} namespace`, () => { | ||
cy.log('navigate to the operator uninstall modal in OperatorHub'); | ||
it(`uninstalls the Operator from ${testName} namespace`, () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about running into the Cypress anti-pattern of Having-tests-rely-on-the-state-of-previous-tests.
@@ -13,6 +13,28 @@ const operatorInstance = 'StorageCluster'; | |||
const openshiftOperatorsNS = 'openshift-operators'; | |||
const operandLink = 'portworx'; | |||
|
|||
const verifyPackageManifest = (opName: string, csName: string, depth: number = 0) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dtaylor113, I made an attempt at adding polling per #7699 (comment). What do you think?
If this is good, I will move this and the rest of CatalogSource
install and uninstall out to a separate file so it can be shared between the two tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I like it!
/bugzilla refresh |
@rhamilto: This pull request references Bugzilla bug 1909836, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
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. |
@dtaylor113, @spadgett PTAL. |
.get(`[data-test-rows="resource-row"]`) | ||
.should('exist') | ||
.then(() => { | ||
// the opName can exist in multiple CatalogSources, so check for the csName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use the new "Operators" tab on the CatalogSource added in #7430. I'd think it would simplify the logic here and add more test coverage for that feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion. @spadgett. I've implemented it. Please take another look.
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhamilto, spadgett 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 |
@rhamilto: All pull requests linked via external trackers have merged: Bugzilla bug 1909836 has been moved to the MODIFIED state. 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. |
…alog-source