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.

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 1878873 : cypress-cucumber-automation-framework #6776

Merged
merged 1 commit into from Nov 3, 2020
Merged

Bug 1878873 : cypress-cucumber-automation-framework #6776

merged 1 commit into from Nov 3, 2020

Conversation

makambalaji
Copy link
Contributor

@makambalaji makambalaji commented Sep 29, 2020

As per the comments mentioned in PR : #6618, splitting PR into small pr's
jira Id : ODC-4926
Tracking the addition of data-test attributes to the dev-console code using JIRA id : ODC-4937

@makambalaji
Copy link
Contributor Author

/assign @dtaylor113

Copy link
Contributor

@dtaylor113 dtaylor113 left a comment

Choose a reason for hiding this comment

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

Hi @makambalaji, thanks for breaking down this PR to a smaller size. I think we still need to work on using common helper methods, and if they do not meet some needs, enahnce the already existing ones. -thanks

Also please link a Bugzilla or JIRA story to this PR which will keep track of removing all the css selectors. - thanks

frontend/package.json Outdated Show resolved Hide resolved
@openshift openshift deleted a comment from makambalaji Oct 1, 2020
frontend/package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@dtaylor113 dtaylor113 left a comment

Choose a reason for hiding this comment

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

/lgtm thanks!

@dtaylor113
Copy link
Contributor

/approve

@dtaylor113
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 5, 2020
Comment on lines +30 to +35
"test-cypress-devconsole": "cd packages/dev-console/integration-tests && ../../../node_modules/.bin/cypress open --env openshift=true",
"test-cypress-devconsole-headless": "cd packages/dev-console/integration-tests && node --max-old-space-size=4096 ../../../node_modules/.bin/cypress run --env openshift=true --browser ${BRIDGE_E2E_BROWSER_NAME:=chrome} --headless --spec \"features/project-creation.feature\";",
Copy link
Member

Choose a reason for hiding this comment

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

Will these tests run as part of CI? Do they run if I use yarn run test-cypress-headless?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, test-cypress-headless is now:
"test-cypress-headless": "yarn run test-cypress-console-headless && yarn run test-cypress-devconsole-headless",

Copy link
Member

Choose a reason for hiding this comment

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

But how will this scale if every package adds a test?

Copy link
Contributor

Choose a reason for hiding this comment

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

@makambalaji i thought you weren't planning to run all the dev-console tests on CI because you said it took hours to run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@spadgett - From dev-console side, Will select the test scenarios for execution based on the tag names mentioned in cypress.json

@christianvogt - In CI, will run only smoke tests, now the execution time is also reduced from 2.5 hr to 1 hr.

Copy link
Contributor

Choose a reason for hiding this comment

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

This pr doesn't add 1hr of tests. I'd like to get the smoke tests down below 1hr down before adding all those tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

But how will this scale if every package adds a test?

@spadgett I was envisioning teams adding to test-cypress-headless in the same manor. Teams need to cd to their cypress dir, so it seems to be the correct method.

@andrewballantyne
Copy link
Contributor

@makambalaji frontend failed...

ESLint couldn't find the config "../.eslintrc" to extend from. Please check that the name of the config is correct.

The config "../.eslintrc" was referenced from the config file in "/go/src/github.com/openshift/console/frontend/packages/dev-console/integration-tests/.eslintrc".

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 5, 2020
@makambalaji
Copy link
Contributor Author

@makambalaji frontend failed...

ESLint couldn't find the config "../.eslintrc" to extend from. Please check that the name of the config is correct.
The config "../.eslintrc" was referenced from the config file in "/go/src/github.com/openshift/console/frontend/packages/dev-console/integration-tests/.eslintrc".

@andrewballantyne : updated the eslintrc file

@andrewballantyne
Copy link
Contributor

Re-adding the lgtm based on the linter change (last commit).

/lgtm

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

/approve

@makambalaji
Copy link
Contributor Author

As per the openshift-ci-robot comments, assigning pr to Sam for approval
/assign @spadgett

@dtaylor113
Copy link
Contributor

/retest

1 similar comment
@makambalaji
Copy link
Contributor Author

/retest

@spadgett
Copy link
Member

spadgett commented Oct 6, 2020

/approve

But you need to run prettier :)

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@dtaylor113
Copy link
Contributor

/hold so openshift-bot doesn't continually try to /retest and PR fills up with failed messages.

Failure seems to be this:

2020/10/30 12:29:58 Container test in pod frontend failed, exit code 1, reason Error
2020/10/30 12:29:58 No custom metadata found and prow metadata already exists. Not updating the metadata.
2020/10/30 12:29:58 Ran for 7m31s
error: some steps failed:
  * could not run steps: step frontend failed: test "frontend" failed: the pod ci-op-x0h566h0/frontend failed after 7m30s (failed containers: test): ContainerFailed one or more containers exited
Container test exited with code 1, reason Error

@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 Oct 30, 2020
@christianvogt
Copy link
Contributor

@makambalaji please update yarn.lock:
image

@dtaylor113
Copy link
Contributor

dtaylor113 commented Oct 30, 2020

Hi @makambalaji,
The yarn.lock issue is causing the CI failure due to this check. The yarn.lock has changed in master so the yarn.lock included in this PR is probably stale. You will need to rebase, build the frontend again, then include the resulting yarn.lock in this PR. -thanks

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

/retest

Copy link
Contributor

@dtaylor113 dtaylor113 left a comment

Choose a reason for hiding this comment

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

/lgtm

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

/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 2, 2020
@dtaylor113
Copy link
Contributor

dtaylor113 commented Nov 2, 2020

/hold until #7053 merges!

Apologies @makambalaji, there was a CI regression when we added OLM Cyrpess tests and we were blocking the cluster-authentication-operator repo from completing PR merges. #7053 fixes this regression.

Once #7053 merges you will need to uncomment this, and resolve merge conflicts in package.json (test-cypress-headless script should be removed/deleted) -thanks

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. 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. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 2, 2020
test-prow-e2e.sh Outdated
@@ -17,7 +17,7 @@ trap copyArtifacts EXIT

# don't log kubeadmin-password
set +x
BRIDGE_KUBEADMIN_PASSWORD="$(cat "${INSTALLER_DIR}/auth/kubeadmin-password")"
BRIDGE_KUBEADMIN_PASSWORD="$(cat "${KUBEADMIN_PASSWORD_FILE:-${INSTALLER_DIR}/auth/kubeadmin-password}")"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this change neccesary? Where is KUBEADMIN_PASSWORD_FILE set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somehow it got added, Now I removed it, Could you please review it now

updated Read-Me document, package.json and Report file name

updated test attributes in add page.ts and added data test atttibutes in dev code for monitoring page

commented a11y vaidations and raised the defects for the same

Updated .git-ignore with package-lock.json because it is not listed in our dependencies
Copy link
Contributor

@dtaylor113 dtaylor113 left a comment

Choose a reason for hiding this comment

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

/lgtm Thanks @makambalaji !

@dtaylor113
Copy link
Contributor

/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 3, 2020
@andrewballantyne
Copy link
Contributor

andrewballantyne commented Nov 3, 2020

/lgtm
(Dave's LGTM wasn't caught by the bot)

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, christianvogt, dtaylor113, makambalaji, sanketpathak, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 4a089a4 into openshift:master Nov 3, 2020
@spadgett spadgett added this to the v4.7 milestone Nov 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/ceph Related to ceph-storage-plugin component/core Related to console core functionality component/dashboard Related to dashboard component/dev-console Related to dev-console component/knative Related to knative-plugin component/kubevirt Related to kubevirt-plugin component/lso Related to local-storage-operator-plugin component/monitoring Related to monitoring component/noobaa Related to noobaa-storage-plugin component/olm Related to OLM component/shared Related to console-shared lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants