Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Cypress test for Pipeline list page[Update] #2780

Closed
wants to merge 1 commit into from
Closed

Cypress test for Pipeline list page[Update] #2780

wants to merge 1 commit into from

Conversation

uidoyen
Copy link
Contributor

@uidoyen uidoyen commented May 7, 2024 •
edited

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):

  • The developer has manually tested the changes and verified that the changes work
  • Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change (find relevant UX in the SMEs section).

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

@openshift-ci openshift-ci bot requested review from lucferbux and pnaik1 May 7, 2024 08:43
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented May 7, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign andrewballantyne for approval. For more information see the Kubernetes Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@uidoyen uidoyen force-pushed the RHOAIENG-3833 branch 2 times, most recently from 70eaa04 to 18764bd Compare May 7, 2024 10:56
@uidoyen uidoyen force-pushed the RHOAIENG-3833 branch 2 times, most recently from a9fc261 to 784ee5e Compare May 8, 2024 12:27
@christianvogt
Copy link
Contributor

Some of these tests seem to be copies of tests in Pipelines.cy.ts.
We should avoid creating duplicate tests.

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.

@uidoyen
Copy link
Contributor Author

uidoyen commented May 10, 2024

Some of these tests seem to be copies of tests in Pipelines.cy.ts. We should avoid creating duplicate tests.

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.

christianvogt
christianvogt reviewed May 10, 2024
View reviewed changes
frontend/src/__tests__/cypress/cypress/pages/pipelines/pipelinesTable.ts
@@ -26,6 +27,20 @@ class PipelinesTableRow extends TableRow {
class PipelinesTable {
private testId = 'pipelines-table';

sortTable() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
sortTable() {
shouldSortTable() {

frontend/src/__tests__/cypress/cypress/pages/pipelines/pipelinesGlobal.ts
Comment on lines +140 to +146
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();
Copy link
Contributor

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.

frontend/src/__tests__/cypress/cypress/pages/pipelines/pipelinesGlobal.ts
Comment on lines +137 to +138
pipelinesGlobal.findConfigurePipelineServerButton().should('be.enabled');
pipelinesGlobal.findConfigurePipelineServerButton().click();
Copy link
Contributor

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.

frontend/src/__tests__/cypress/cypress/pages/pipelines/pipelinesGlobal.ts
this.findSubmitButton().should('be.enabled');
this.findSubmitButton().click();

cy.wait('@createSecret').then((interception) => {
Copy link
Contributor

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.

frontend/src/__tests__/cypress/cypress/pages/pipelines/pipelinesGlobal.ts
}

class ViewPipelineServerModal extends Modal {
constructor() {
super('View pipeline server');
}

viewPipelineServerDetails() {
Copy link
Contributor

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.

Suggested change
viewPipelineServerDetails() {
shouldHaveSecret({ ... }) {

frontend/src/__tests__/cypress/cypress/pages/pipelines/pipelinesGlobal.ts
Comment on lines +200 to +201
pipelinesGlobal.selectPipelineServerAction('View pipeline server configuration');
this.findCloseButton().click();
Copy link
Contributor

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.

frontend/src/__tests__/cypress/cypress/pages/pipelines/pipelineImportModal.ts
pipelinesTable.getRowByName('New pipeline').find().should('exist');
}

uploadPipelineVersion(
Copy link
Contributor

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.

@openshift-merge-robot openshift-merge-robot added the needs-rebase PR needs to be rebased label May 15, 2024
@openshift-merge-robot
Copy link
Contributor

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.

@christianvogt
Copy link
Contributor

Superseded by recent re-work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christianvogt christianvogt christianvogt left review comments

@pnaik1 pnaik1 pnaik1 requested changes

@ppadti ppadti ppadti requested changes

@lucferbux lucferbux Awaiting requested review from lucferbux

Assignees

@pnaik1 pnaik1

@ppadti ppadti

Labels
needs-rebase PR needs to be rebased
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@uidoyen @christianvogt @openshift-merge-robot @pnaik1 @ppadti