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 Scripts - Create from git add flow ODC-4976 #7103

Merged
merged 4 commits into from Nov 30, 2020
Merged

Cypress Scripts - Create from git add flow ODC-4976 #7103

merged 4 commits into from Nov 30, 2020

Conversation

makambalaji
Copy link
Contributor

@makambalaji makambalaji commented Nov 4, 2020 •
edited

Renamed the feature files and added the scripts related to git add flow
Defect Raised :
ODC-5108 - Page redirecting to topology list view instead of topology graph view
Web Accessibility testing defect raised - ODC-5119, ODC-5129
Please Note: This scripts are not integrated with CI
If need to verify, execute the below command "npm run test-cypress-devconsole" on fronted

Test Results:
No. of smoke test cases in this pr : 2 [Execution time: 55 sec]
No. of regression test cases in this pr: 12 [Execution time: 9 min]

@makambalaji makambalaji changed the title Modified the feature file names and added scripts related to Add git … [WIP]Modified the feature file names and added scripts related to Add git … Nov 4, 2020
@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. component/dev-console Related to dev-console labels Nov 4, 2020
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 4, 2020
@makambalaji makambalaji changed the title [WIP]Modified the feature file names and added scripts related to Add git … [WIP] create from git add flow related scripts Nov 10, 2020
@makambalaji makambalaji changed the title [WIP] create from git add flow related scripts [WIP] create from git add flow related scripts ODC-4976 Nov 11, 2020
@makambalaji makambalaji changed the title [WIP] create from git add flow related scripts ODC-4976 create from git add flow related scripts ODC-4976 Nov 11, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 11, 2020
@makambalaji makambalaji changed the title create from git add flow related scripts ODC-4976 [WIP] Create from git add flow related scripts ODC-4976 Nov 11, 2020
@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 11, 2020
@makambalaji makambalaji changed the title [WIP] Create from git add flow related scripts ODC-4976 Create from git add flow related scripts ODC-4976 Nov 12, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 12, 2020
@makambalaji makambalaji changed the title Create from git add flow related scripts ODC-4976 [WIP] Create from git add flow related scripts ODC-4976 Nov 12, 2020
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 12, 2020
@makambalaji makambalaji changed the title [WIP] Create from git add flow related scripts ODC-4976 Create from git add flow related scripts ODC-4976 Nov 12, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 12, 2020
@makambalaji
Copy link
Contributor Author

Uploading PR-7103.png…

@makambalaji
Copy link
Contributor Author

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 12, 2020
@makambalaji makambalaji changed the title Create from git add flow related scripts ODC-4976 Cypress Scripts - Create from git add flow ODC-4976 Nov 12, 2020
@makambalaji
Copy link
Contributor Author

/retest

@makambalaji
Copy link
Contributor Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 12, 2020
@andrewballantyne
Copy link
Contributor

@makambalaji You have typescript errors from the looks of things:

ERROR in /go/src/github.com/openshift/console/frontend/packages/dev-console/integration-tests/support/pages/helm/helm-details-page.ts
ERROR in /go/src/github.com/openshift/console/frontend/packages/dev-console/integration-tests/support/pages/helm/helm-details-page.ts(18,28):
TS2339: Property 'eq' does not exist on type 'Matchers<void>'.
ERROR in /go/src/github.com/openshift/console/frontend/packages/dev-console/integration-tests/support/pages/helm/helm-details-page.ts
ERROR in /go/src/github.com/openshift/console/frontend/packages/dev-console/integration-tests/support/pages/helm/helm-details-page.ts(19,28):
TS2339: Property 'eq' does not exist on type 'Matchers<void>'.
ERROR in /go/src/github.com/openshift/console/frontend/packages/dev-console/integration-tests/support/pages/helm/helm-details-page.ts
ERROR in /go/src/github.com/openshift/console/frontend/packages/dev-console/integration-tests/support/pages/helm/helm-details-page.ts(20,28):
TS2339: Property 'eq' does not exist on type 'Matchers<void>'.
ERROR in /go/src/github.com/openshift/console/frontend/packages/dev-console/integration-tests/support/pages/topology/topology-page.ts
ERROR in /go/src/github.com/openshift/console/frontend/packages/dev-console/integration-tests/support/pages/topology/topology-page.ts(113,23):
TS2551: Property 'contains' does not exist on type 'Matchers<void>'. Did you mean 'toContain'?
ERROR in /go/src/github.com/openshift/console/frontend/packages/dev-console/integration-tests/support/pages/topology/topology-side-pane-page.ts
ERROR in /go/src/github.com/openshift/console/frontend/packages/dev-console/integration-tests/support/pages/topology/topology-side-pane-page.ts(34,25):
TS2551: Property 'contains' does not exist on type 'Matchers<void>'. Did you mean 'toContain'?
ERROR in /go/src/github.com/openshift/console/frontend/packages/dev-console/integration-tests/support/pages/topology/topology-side-pane-page.ts
ERROR in /go/src/github.com/openshift/console/frontend/packages/dev-console/integration-tests/support/pages/topology/topology-side-pane-page.ts(87,22):
TS2339: Property 'eq' does not exist on type 'Matchers<void>'.
ERROR in /go/src/github.com/openshift/console/frontend/packages/dev-console/integration-tests/support/step-definitions/common/addFlow.ts
ERROR in /go/src/github.com/openshift/console/frontend/packages/dev-console/integration-tests/support/step-definitions/common/addFlow.ts(17,9):
TS2339: Property 'createGitWorkload' does not exist on type '{ waitForLoad: (timeout?: number) => Chainable<JQuery<HTMLElement>>; }'.

@makambalaji
Copy link
Contributor Author

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 19, 2020
@makambalaji
Copy link
Contributor Author

/retest

@makambalaji
Copy link
Contributor Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 19, 2020
@sanketpathak
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 23, 2020
invincibleJai
invincibleJai reviewed Nov 24, 2020
View reviewed changes
frontend/packages/dev-console/integration-tests/support/pageObjects/add-flow-po..ts Outdated
@@ -0,0 +1,192 @@
export const cardTitle = 'div.catalog-tile-pf-title';
Copy link
Member

Choose a reason for hiding this comment

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

file name has 2 .. , add-flow-po..ts

frontend/packages/dev-console/integration-tests/support/pages/functions/createGitWorkload.ts Outdated
Comment on lines 5 to 10
export function createGitWorkload(
gitUrl: string = 'https://github.com/sclorg/nodejs-ex.git',
componentName: string = 'nodejs-ex-git',
resourceType: string = 'Deployment',
appName: string = 'nodejs-ex-git-app',
) {
Copy link
Member

Choose a reason for hiding this comment

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

nit : can we make use of arrow function?

Suggested change
export function createGitWorkload(
gitUrl: string = 'https://github.com/sclorg/nodejs-ex.git',
componentName: string = 'nodejs-ex-git',
resourceType: string = 'Deployment',
appName: string = 'nodejs-ex-git-app',
) {
export const createGitWorkload(
gitUrl: string = 'https://github.com/sclorg/nodejs-ex.git',
componentName: string = 'nodejs-ex-git',
resourceType: string = 'Deployment',
appName: string = 'nodejs-ex-git-app',
) => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, I will update it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

i still see this 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its updated

...packages/dev-console/integration-tests/support/pages/functions/createHelmChartFromAddPage.ts
import { addPage } from '../add-flow/add-page';
import { addOptions } from '../../constants/add';
import { pageTitle } from '../../constants/pageTitle';
import { topologyPO } from '../../pageObjects/topology-po';
Copy link
Member

Choose a reason for hiding this comment

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

just curious what does P0 mean here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pageobjects

frontend/packages/dev-console/integration-tests/support/pages/topology/topology-actions-page.ts
Comment on lines +7 to +8
case 'Edit Application Grouping':
case nodeActions.EditApplicatoinGrouping: {
Copy link
Member

Choose a reason for hiding this comment

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

why two similar switch case one with const and one string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's like a or condition, where either or case will pass then the statement will get executed

Copy link
Member

Choose a reason for hiding this comment

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

that i got my doubt why we need condition as both looks similar to me 'Edit Application Grouping' and nodeActions.EditApplicatoinGrouping shouldn't the const as well have same values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sometimes we use to pass them from gherkin scripts like
user selects "Move Sink" from context menu
and some times without "" (double quotes) so it should work for both. So I used these technique

@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm Indicates that a PR is ready to be merged. labels Nov 27, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 27, 2020
@makambalaji
Copy link
Contributor Author

@invincibleJai - Updated as per your comments, Can you please re-review it

@makambalaji @sanketpathak
Update frontend/packages/dev-console/integration-tests/features/addFl…
04f26f5 
…ow/create-from-git.feature

Co-authored-by: Sanket Pathak <sanketpathak95@gmail.com>
invincibleJai
invincibleJai reviewed Nov 27, 2020
View reviewed changes
frontend/packages/dev-console/integration-tests/support/pages/add-flow/catalog-page.ts Outdated
@@ -0,0 +1,102 @@
import { catalogPO } from '../../pageObjects/add-flow-po.';
import { caatalogCards, catalogTypes } from '../../constants/add';
Copy link
Member

Choose a reason for hiding this comment

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

typo: caatalogCards

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

frontend/packages/dev-console/integration-tests/support/pages/add-flow/git-page.ts Outdated Show resolved Hide resolved
frontend/packages/dev-console/integration-tests/support/pages/topology/topology-actions-page.ts
Comment on lines +7 to +8
case 'Edit Application Grouping':
case nodeActions.EditApplicatoinGrouping: {
Copy link
Member

Choose a reason for hiding this comment

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

that i got my doubt why we need condition as both looks similar to me 'Edit Application Grouping' and nodeActions.EditApplicatoinGrouping shouldn't the const as well have same values?

invincibleJai
invincibleJai reviewed Nov 27, 2020
View reviewed changes
frontend/packages/dev-console/integration-tests/support/pages/topology/topology-actions-page.ts Outdated
export const topologyActions = {
selectAction: (action: nodeActions | string) => {
switch (action) {
case nodeActions.EditApplicatoinGrouping || 'Edit Application Grouping': {
Copy link
Member

Choose a reason for hiding this comment

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

what you had before was fine then having || here , my doubt was why we need condition as both looks similar to me 'Edit Application Grouping' and nodeActions.EditApplicatoinGrouping shouldn't the const as well have same values?

and as i see nodeActions.EditApplicatoinGrouping as well have same value 'Edit Application Grouping'

https://github.com/openshift/console/pull/7103/files#diff-b28558d63fcea46d8035f6daef029b0245aa203ca30755e6ae21a3cff2dbbab5R8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sometimes we use to pass them from gherkin scripts like
user selects "Move Sink" from context menu
and some times without "" (double quotes) so it should work for both. So I used these technique

Copy link
Member

Choose a reason for hiding this comment

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

ah does that matter in switch case string is string right check here https://jsfiddle.net/invincibleJai/3tov1ags/3/

@makambalaji @invincibleJai
Update frontend/packages/dev-console/integration-tests/support/pages/…
ddb6c38 
…add-flow/git-page.ts

Co-authored-by: Jaivardhan Kumar <mailjai.vardhan@gmail.com>
@invincibleJai
Copy link
Member

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 27, 2020
@sanketpathak
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 30, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: invincibleJai, makambalaji, sanketpathak

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

@openshift-merge-robot openshift-merge-robot merged commit 44e2cbc into openshift:master Nov 30, 2020
@spadgett spadgett added this to the v4.7 milestone Nov 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@invincibleJai invincibleJai invincibleJai left review comments

@sanketpathak sanketpathak sanketpathak left review comments

@gajanan-more gajanan-more Awaiting requested review from gajanan-more

Assignees

@sanketpathak sanketpathak

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/dev-console Related to dev-console lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
v4.7
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@makambalaji @andrewballantyne @sanketpathak @invincibleJai @openshift-ci-robot @spadgett @openshift-merge-robot