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

ODC-6023-update devconsole-ci-tests #9361

Merged
merged 2 commits into from Jul 15, 2021
Merged

ODC-6023-update devconsole-ci-tests #9361

merged 2 commits into from Jul 15, 2021

Conversation

makambalaji
Copy link
Contributor

@makambalaji makambalaji commented Jun 28, 2021

Jira Id: https://issues.redhat.com/browse/ODC-6023

Description:
CI implemented for entire add flow

Browser & its version : Chrome 91

Pre-conditions/setup:

Checks for approving Epic scenarios Automation PR:

  • Execute the @to-do tagged gherkin scripts manually
  • Convert the @to-do gherkin scripts to cypress automation scripts
  • Once scripts are automated, replace tag @to-do with @epic-number
  • Execute the scripts in Remote cluster

Refactoring criteria for approving Automation PR:

Execution Commands:
In below commands, please update url and password based on the cluster

export NO_HEADLESS=true && export CHROME_VERSION=$(/usr/bin/google-chrome-stable --version)
BRIDGE_KUBEADMIN_PASSWORD=YH3jN-PRFT2-Q429c-5KQDr
BRIDGE_BASE_ADDRESS=https://console-openshift-console.apps.dev-svc-4.8-042801.devcluster.openshift.com
export BRIDGE_KUBEADMIN_PASSWORD
export BRIDGE_BASE_ADDRESS
oc login -u kubeadmin -p $BRIDGE_KUBEADMIN_PASSWORD https://api.gamore48installer7thjune001.devcluster.openshift.com:6443  --insecure-skip-tls-verify
oc apply -f ./frontend/integration-tests/data/htpasswd-secret.yaml
oc patch oauths cluster --patch "$(cat ./frontend/integration-tests/data/patch-htpasswd.yaml)" --type=merge
./test-cypress.sh -p dev-console -h true

Results: 18 test cases are executed in 19 min [duration]
Screenshots
image

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

@openshift-ci openshift-ci bot added component/dev-console Related to dev-console approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 28, 2021
@makambalaji
Copy link
Contributor Author

/retest

1 similar comment
@makambalaji
Copy link
Contributor Author

/retest

@@ -11,7 +11,7 @@
"clean-reports": "rm -rf ../../../gui_test_screenshots",
"cypress-merge": "mochawesome-merge ../../../gui_test_screenshots/cypress_report*.json > ../../../gui_test_screenshots/cypress.json",
"cypress-generate": "marge -o ../../../gui_test_screenshots/ -f cypress-report -t 'OpenShift DevConsole Cypress Test Results' -p 'OpenShift Cypress Test Results' --showPassed false --assetsDir ../../../gui_test_screenshots/cypress/assets ../../../gui_test_screenshots/cypress.json",
"test-headless": "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\";",
"test-headless": "node --max-old-space-size=4096 ../../../node_modules/.bin/cypress run --env openshift=true --browser ${BRIDGE_E2E_BROWSER_NAME:=chrome} --headless --spec \"features/addFlow/*.feature\";",
Copy link
Member

Choose a reason for hiding this comment

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

Should this package.json script test all features (smoke tests)??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes @jerolimov, it executes all Add flow test cases based on the tags mentioned in cypress.json

Copy link
Member

Choose a reason for hiding this comment

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

Buy why? Why not all? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we need to execute all, it takes more time.. which is not required for CI

Copy link
Member

@jerolimov jerolimov left a comment

Choose a reason for hiding this comment

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

Verified this with a local run of cypress via yarn test-cypress-devconsole

With this PR:
with-pr

Before:
master

Error details on master branch:
error

/lgtm
/retest

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 7, 2021
@jerolimov
Copy link
Member

/retest

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 7, 2021
Copy link
Member

@jerolimov jerolimov left a comment

Choose a reason for hiding this comment

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

If you like you can also add the following data-tests attributes and change this here to this new data-test references. Wdyt?

  • In CatalogCategories.tsx I would recommend:
    return (
      <VerticalTabsTab
        key={id}
        title={label}
        active={active}
        className={tabClasses}
        onActivate={() => onSelectCategory(id)}
        hasActiveDescendant={hasActiveDescendant(selectedCategory, category)}
        shown={toplevelCategory}
+        data-test={`tab ${id}`}
      >
        {subcategories && (
          <VerticalTabs restrictTabs activeTab={isActiveTab(selectedCategoryID, category)}>
            {_.map(subcategories, (subcategory) =>
              renderTabs(subcategory, selectedCategoryID, false),
            )}
          </VerticalTabs>
        )}
      </VerticalTabsTab>
    );
  };

  return (
-    <VerticalTabs restrictTabs activeTab={activeTab}>
+    <VerticalTabs restrictTabs activeTab={activeTab} data-test="catalog-categories">
      {_.map(categories, (category) => renderTabs(category, selectedCategory, true))}
    </VerticalTabs>
  );
  • In CatalogTypeSelector.tsx:
-      <VerticalTabs>
+      <VerticalTabs data-test="catalog-types">
        {catalogTypes.map((type) => {
          const { value, label } = type;
          const typeCount = catalogTypeCounts[value];
          const queryParams = new URLSearchParams(search);
          queryParams.set(CatalogQueryParams.TYPE, type.value);

          const to = {
            pathname,
            search: `?${queryParams.toString()}`,
          };

          return typeCount > 0 ? (
-            <li key={value} className="vertical-tabs-pf-tab">
+            <li key={value} className="vertical-tabs-pf-tab" data-test={`tab ${value}`}>
              <Link to={to}>{`${label} (${typeCount})`}</Link>
            </li>
          ) : null;
        })}
      </VerticalTabs>

Wdyt?

Comment on lines 114 to 116
helmCharts: 'a[href$="catalogType=HelmChart"]',
builderImage: 'a[href$="catalogType=BuilderImage"]',
template: 'a[href$="catalogType=Template"]',
Copy link
Member

@jerolimov jerolimov Jul 7, 2021

Choose a reason for hiding this comment

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

With the recommended change above you can simplify the sectors here:

Suggested change
helmCharts: 'a[href$="catalogType=HelmChart"]',
builderImage: 'a[href$="catalogType=BuilderImage"]',
template: 'a[href$="catalogType=Template"]',
helmCharts: '[data-test="tab HelmChart"]',
builderImage: '[data-test="tab BuilderImage"]',
template: '[data-test="tab Template"]',

serviceClass: '[data-test="kind-cluster-service-class"]',
managedServices: '[data-test="kind-managed-service"]',
eventSources: 'a[href="/?catalogType=EventSource"]',
eventSources: 'a[href$="catalogType=EventSource"]',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
eventSources: 'a[href$="catalogType=EventSource"]',
eventSources: '[data-test="tab EventSource"]',

Copy link
Member

@jerolimov jerolimov left a comment

Choose a reason for hiding this comment

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

Run the cypress tests locally again, and run successfully. Selectors looks good.
/lgtm

Approve the test-data frontend changes.
/approve

/retest

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2021
@openshift-bot
Copy link
Contributor

/retest

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

5 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@jerolimov
Copy link
Member

/hold
Looks like CI a11y checks are broken. I will take a look.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 8, 2021
@jerolimov
Copy link
Member

/retest

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 12, 2021
@jerolimov
Copy link
Member

The E2E tests fails because of an OperatorHub I fixed now in #9479. We need to wait for this PR and after that this PR should be rebased and tested after that again.

@makambalaji As mentioned in slack, I still got an issue with multiple entries in the OperatorHub. As we discussed it might be good to use full qualified data-test ids here

https://github.com/openshift/console/blob/master/frontend/packages/dev-console/integration-tests/support/pageObjects/operators-po.ts#L16-L25

@makambalaji
Copy link
Contributor Author

The E2E tests fails because of an OperatorHub I fixed now in #9479. We need to wait for this PR and after that this PR should be rebased and tested after that again.

@makambalaji As mentioned in slack, I still got an issue with multiple entries in the OperatorHub. As we discussed it might be good to use full qualified data-test ids here

https://github.com/openshift/console/blob/master/frontend/packages/dev-console/integration-tests/support/pageObjects/operators-po.ts#L16-L25

updated the page objects

@jerolimov
Copy link
Member

/retest

@jerolimov
Copy link
Member

Tested this PR together with this two PRs. After that the headless tests run successfully on a shared cluster and a local cluster (crc). Sorry but I think we should wait until both are merged and then this PR should be rebased again.

cd packages/dev-console/integration-tests
yarn test-cypress-headless

@openshift-ci openshift-ci bot added component/core Related to console core functionality kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated labels Jul 15, 2021
@makambalaji
Copy link
Contributor Author

Tested this PR together with this two PRs. After that the headless tests run successfully on a shared cluster and a local cluster (crc). Sorry but I think we should wait until both are merged and then this PR should be rebased again.

cd packages/dev-console/integration-tests
yarn test-cypress-headless

Looks like pr's are merged

@jerolimov
Copy link
Member

/retest

@openshift-ci openshift-ci bot added the component/helm Related to helm-plugin label Jul 15, 2021
@makambalaji
Copy link
Contributor Author

data-test attributes are included in this pr. due to that reason, scripts are verified on localhost instead of remote cluster once this pr is merged. will raise a separate pr for modifying devconsole/integration-tests/package.json to execute all addFlow scripts

@makambalaji
Copy link
Contributor Author

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 15, 2021
@jerolimov
Copy link
Member

Re run the tests locally, and works fine.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 15, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerolimov, makambalaji

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 73e5757 into openshift:master Jul 15, 2021
@spadgett spadgett added this to the v4.9 milestone Aug 30, 2021
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/core Related to console core functionality component/dev-console Related to dev-console component/helm Related to helm-plugin kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated 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

5 participants