-
Notifications
You must be signed in to change notification settings - Fork 617
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
CONSOLE-3740: Upgrade Cypress #13070
Conversation
@@ -12,8 +12,15 @@ | |||
"sourceMap": true, | |||
"noUnusedLocals": true, | |||
"typeRoots": ["node_modules/@types", "@types"], | |||
"types": ["node", "jest", "cypress", "cypress-axe", "cypress-file-upload", "console"] | |||
"types": ["node", "jest", "cypress-axe", "console"] |
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.
cypress
and cypress-file-upload
do not have @types
packages.
3d635b0
to
b3042a1
Compare
/retest |
/retest |
b1ec710
to
439692b
Compare
@rhamilto: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
fcf1ff3
to
c61c7ee
Compare
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.
Just a few observations. I don't fully understand why the ts config for the integration test package is weird. I remember trying to tinker with it at one point because VSCode kept telling me that 'cy' wasn't defined in the global scope, but I didn't have much luck with it. Let me know if you want to look at it together.
frontend/packages/integration-tests-cypress/support/selectors.ts
Outdated
Show resolved
Hide resolved
frontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-hub.cy.ts
Show resolved
Hide resolved
frontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-hub.cy.ts
Show resolved
Hide resolved
frontend/packages/operator-lifecycle-manager/integration-tests-cypress/views/operator.view.ts
Show resolved
Hide resolved
d08018d
to
8b38367
Compare
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.
Looks good. Adding px-approved label.
/label docs-approved |
@rhamilto: This pull request references CONSOLE-3740 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but it targets "openshift-4.15" instead. 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. |
…in-Add-dashboard-card-with-chart-using-QueryBrowser-component CONSOLE-3675: Dynamic Demo Plugin: Add dashboard card with chart using QueryBrowser component
1d48dc7
to
b269f98
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.
Just a few comments. Let me know your thoughts.
frontend/packages/integration-tests-cypress/tests/app/auth-multiuser-login.cy.ts
Show resolved
Hide resolved
cy.byTestID('user-dropdown').click(); | ||
cy.byTestID('log-out').should('be.visible'); | ||
cy.byTestID('log-out').click({ force: true }); | ||
cy.visit('/'); |
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 need some kind of assertion here to make sure we actually logged out.
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.
Probably, but this isn't functionally different than what it was doing before, so no change in behavior. Follow on?
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.
Yeah, I think that's fine. I just worry that with sessions, we won't know if the logout failed.
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 after playing with this, I don't think there is a way to actually test logout fully because of the way cy.session
works since it's interecepting the credentials. Will leave it for now.
frontend/packages/integration-tests-cypress/tests/app/poll-console-updates.cy.ts
Show resolved
Hide resolved
...packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-uninstall.cy.ts
Show resolved
Hide resolved
...packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-uninstall.cy.ts
Show resolved
Hide resolved
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
We can fix the logout test as a follow-on.
cy.byTestID('user-dropdown').click(); | ||
cy.byTestID('log-out').should('be.visible'); | ||
cy.byTestID('log-out').click({ force: true }); | ||
cy.visit('/'); |
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.
Yeah, I think that's fine. I just worry that with sessions, we won't know if the logout failed.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhamilto, TheRealJon 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 |
Adding qe-approval label since this isn't customer facing. /label qe-approval |
@rhamilto: The label(s) 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. |
/label qe-approved typing is hard |
@rhamilto: This pull request references CONSOLE-3740 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but it targets "openshift-4.15" instead. 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. |
@rhamilto: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
Based on my findings, there are a number of recommendations/observations:
As part of the upgrade from 8.5.0 >= 10.x, the existing cypress.json configs and test names need to be updated. Running
cypress open
with a >= 10.x version of Cypress installed provides a wizard that covers this process. I have done already done so forpackages/integration-tests-cypress
andpackages/operator-lifecycle-manager/integration-tests-cypress
in this PR. But the following configs and tests need updating:packages/ceph-storage-plugin/integration-tests-cypress/cypress-ceph.json(will be removed per @SanjalKatiyar)The tests run significantly faster in Cypress >= 10.x. This exposes an issue with tests that use
cy.visit
where the console redirects to/
if the console isn't fully loaded before Cypress starts testing. A workaround is to visit/
first and then check the perspective is set toAdministrator
. This appears to give the console time to fully load. I've made these adjustments forpackages/integration-tests-cypress
andpackages/operator-lifecycle-manager/integration-tests-cypress
via global command (cy.initAdmin
) I added.I've also added
cy.createProjectWithCLI
andcy.deleteProjectWithCLI
. It occurs to me we should use the CLI to create and delete projects where testing these functions isn't important to the test in order to reduce the run time of the tests, which is a task from https://issues.redhat.com/browse/CONSOLE-3718. cc: @TheRealJonNotes on updated tests
Requires demo plugin be running locally:
Require monitoring plugin be running locally: