-
Notifications
You must be signed in to change notification settings - Fork 163
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
Cypress test for Pipeline list page[Update] #2780
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
70eaa04
to
18764bd
Compare
frontend/src/__tests__/cypress/cypress/e2e/pipelines/PipelinesList.cy.ts
Outdated
Show resolved
Hide resolved
frontend/src/__tests__/cypress/cypress/e2e/pipelines/PipelinesList.cy.ts
Outdated
Show resolved
Hide resolved
frontend/src/__tests__/cypress/cypress/e2e/pipelines/PipelinesList.cy.ts
Outdated
Show resolved
Hide resolved
frontend/src/__tests__/cypress/cypress/e2e/pipelines/PipelinesList.cy.ts
Outdated
Show resolved
Hide resolved
frontend/src/__tests__/cypress/cypress/e2e/pipelines/PipelinesList.cy.ts
Outdated
Show resolved
Hide resolved
a9fc261
to
784ee5e
Compare
Some of these tests seem to be copies of tests in If we have shared components appearing on multiple pages, while testing both locations is somewhat redundant, there is potential for one page to function and the other is broken which gives a reason for testing both locations. We should look at sharing the same tests and running them in both locations. |
@christianvogt updated the PR accordingly. |
@@ -26,6 +27,20 @@ class PipelinesTableRow extends TableRow { | |||
class PipelinesTable { | |||
private testId = 'pipelines-table'; | |||
|
|||
sortTable() { |
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.
sortTable() { | |
shouldSortTable() { |
this.findAwsKeyInput().type('test-aws-key'); | ||
this.findAwsSecretKeyInput().type('test-secret-key'); | ||
this.findEndpointInput().type('https://s3.amazonaws.com/'); | ||
this.findRegionInput().should('have.value', 'us-east-1'); | ||
this.findBucketInput().type('test-bucket'); | ||
this.findSubmitButton().should('be.enabled'); | ||
this.findSubmitButton().click(); |
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.
This data needs to be parameterized.
pipelinesGlobal.findConfigurePipelineServerButton().should('be.enabled'); | ||
pipelinesGlobal.findConfigurePipelineServerButton().click(); |
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.
The modal should not be responsible for opening itself.
this.findSubmitButton().should('be.enabled'); | ||
this.findSubmitButton().click(); | ||
|
||
cy.wait('@createSecret').then((interception) => { |
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.
This function has a soft dependency on named interceptors being present.
As a page object, it should be reusable for multiple scenarios and should not make any assumptions about setup.
I think you're better off sharing these kinds of assertions in a shared test utility.
} | ||
|
||
class ViewPipelineServerModal extends Modal { | ||
constructor() { | ||
super('View pipeline server'); | ||
} | ||
|
||
viewPipelineServerDetails() { |
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.
This function needs to be parameterized with the data it asserting.
viewPipelineServerDetails() { | |
shouldHaveSecret({ ... }) { |
pipelinesGlobal.selectPipelineServerAction('View pipeline server configuration'); | ||
this.findCloseButton().click(); |
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.
The modal should not know how to open itself as it can likely be opened from multiple places in the UI.
pipelinesTable.getRowByName('New pipeline').find().should('exist'); | ||
} | ||
|
||
uploadPipelineVersion( |
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 should have been more clear on my suggestion to move code to the page objects and also that not all test code should be moved to page objects.
We need to consider that page objects must be reusable for both mock and live cluster testing. Especially when using a modal, we should not assume that the modal is being invoked from a particular page.
There is value in abstracting some common interactions and assertions into page objects. But you should also consider creating shared test utilities that are specific to your mock tests to be reused across pages.
As part of the pipeline import modal, these functions have too much knowledge about the overall state of the app as well as are not fully parameterized to be reusable in live cluster testing for different scenarios.
PR needs rebase. 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-sigs/prow repository. |
Superseded by recent re-work. |
Closes: RHOAIENG-3833
Description
Cypress test for Pipeline list page[Update]
How Has This Been Tested?
npm run test
Test Impact
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main