Testing infra for metalkubed #1886

ukalifon · 2019-07-02T12:42:53Z

This is the basic Protractor (Selenium) infrastructure we use to test
the baremetal features in the web-ui fork. It is dependent on some
kubevirt code which is shared among the infrastructures, so those deps
are part of this PR as well.

The actual test is in tests/metalkube/dashboard.scenario.ts and it only
reads some of the counters in the dashboards page, and compares them to
what we get from the CLI.

The test might currently fail because not all functionality from the
web-ui has been merged into console yet (it's a long WIP). The test
should pass when the merge is done, as you can see when running the
test against a web-ui instance:

yarn install
yarn webdriver-update
export BRIDGE_BASE_ADDRESS='https://...redhat.com'
export BRIDGE_AUTH_USERNAME=kubeadmin
export BRIDGE_AUTH_PASSWORD=22Z....
oc login -s ...redhat.com:6443 -u kubeadmin -p 22ZW....
export KUBECONFIG=/home/ukalifon/.kube/config
yarn run test-suite --suite baremetalSmokeTests --params.openshift true

openshift-ci-robot · 2019-07-02T12:43:08Z

Hi @ukalifon. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

rhrazdil · 2019-07-23T08:33:57Z

/ok-to-test

spadgett

We should remove uses of xpath and avoid complicated, fragile selectors. Add data-test-id attributes where needed to simplify the tests.

frontend/integration-tests/views/metalkube/dashboards.view.ts

ukalifon · 2019-07-23T14:22:05Z

/retest

ukalifon · 2019-08-09T15:00:26Z

/test ci/prow/e2e-aws

spadgett · 2019-08-10T15:31:21Z

frontend/integration-tests/tests/kubevirt/kubevirt.login.scenario.ts

+import { execSync } from 'child_process';
+import { logIn } from './utils/utils';
+
+describe('Authentication', () => {


I'm unclear why we need a special login scenario for kubevirt. Can you elaborate?

@spadgett We had to add special login because the login page was different in web-ui and openshift/console at certain point in the past, so login scenario from console didn't work.
Although this is not an issue now, I don't see why plugin related test suites should be running whole console login scenario.
I would prefer having some prerequisite that handles the login quickly rather than executing the whole console login scenario that is executed as a part of different suite anyway.

spadgett · 2019-08-10T15:32:19Z

frontend/integration-tests/tests/kubevirt/kubevirt.login.scenario.ts

+    await browser.get(appHost);
+    if (process.env.BRIDGE_BASE_ADDRESS !== undefined) {
+      await logIn();
+      execSync(`oc login -u ${process.env.BRIDGE_AUTH_USERNAME} -p ${process.env.BRIDGE_AUTH_PASSWORD} --config=${process.env.KUBECONFIG}`);


config=${process.env.KUBECONFIG} should be unnecessary. oc will already read the env var.

spadgett · 2019-08-10T15:33:03Z

frontend/integration-tests/tests/kubevirt/utils/consts.ts

+export const WIZARD_TABLE_FIRST_ROW = 1;
+
+// Tab names
+export const TABS = {


I would use Object.freeze on all of these to avoid accidentally mutating them in the tests

spadgett · 2019-08-10T15:34:09Z

frontend/integration-tests/tests/kubevirt/utils/consts.ts

+  NICS: 'Network Interfaces',
+};
+
+// Network tab columns in VM Wizard


It would be much better to include data-test-id in the columns so we don't need to hard-code each index. It makes the test more robust when things change.

spadgett · 2019-08-10T15:34:58Z

frontend/integration-tests/tests/kubevirt/utils/consts.ts

+
+// TIMEOUTS
+const SEC = 1000;
+export const CLONE_VM_TIMEOUT = 300 * SEC;


I would put SECONDS in each of these names to make it obvious what the unit is where it's used.

Suggested change

export const CLONE_VM_TIMEOUT = 300 * SEC;

export const CLONE_VM_TIMEOUT_SECONDS = 300 * SEC;

spadgett · 2019-08-10T15:44:31Z

frontend/integration-tests/tests/kubevirt/utils/utils.ts

+};
+
+// eslint-disable-next-line eqeqeq
+export const resolveTimeout = (timeout, defaultTimeout) => timeout != undefined ? timeout : defaultTimeout;


Doesn't protractor use the default timeout already if none is specified? I'm unclear why this is needed at all.

@spadgett This was not intended to use protractor default value as default.
Use case: consider an action that usually takes certain amount of time (starting cirros VM for example), but sometimes we want to start Windows or CentOS VM, which takes more time. So start action has defined default timeout (different from the protractor default) for starting a VM and for special cases a different timeout can be passed to the method.

spadgett · 2019-08-10T15:44:57Z

frontend/integration-tests/tests/metalkube/dashboard.scenario.ts

+  beforeAll(async() => {
+    await browser.get(`${appHost}/dashboards`);
+    await dashboardView.isLoaded();
+    browser.sleep(10000); // the counters on the page start updating late...


arbitrary sleeps are not good. we should wait until the data appears

spadgett · 2019-08-10T15:45:58Z

frontend/integration-tests/tests/metalkube/dashboard.scenario.ts

+    const output = execSync('oc get nodes', { encoding: 'utf-8' });
+    const lines = output.split('\n');
+    lines.forEach(function(line) {
+      if (line.indexOf(' Ready ') > 0) {


Better to use -o json and check the JSON values

spadgett · 2019-08-10T15:46:30Z

frontend/integration-tests/tests/metalkube/dashboard.scenario.ts

+    const lines = output.split('\n').slice(1); // slice(1) to ignore the 1st line of output
+    lines.forEach(function(line) {
+      if (line.trim().length > 1) {
+        if (line.indexOf(' OK ') > 0) {


Same comment here. Please use -o json

spadgett · 2019-08-10T15:47:13Z

frontend/integration-tests/views/metalkube/dashboards.view.ts

+ * await dashboardView.sysEventTime(0).getText()
+ */
+export function sysEventTime(rowNumber) {
+  return element(by.xpath(`(//div[@class="co-sysevent__header"])[${rowNumber + 1}]/small`));


avoid xpath, here and below. add data-test-id if needed

rhrazdil · 2019-09-18T08:56:14Z

frontend/integration-tests/tests/metalkube/dashboard.scenario.ts

+import { appHost } from '../../protractor.conf';
+import * as dashboardView from '../../views/metalkube/dashboards.view';
+
+async function compareCounters(elem, expectedValue) {


maybe just compareCounter since it only takes one element.

rhrazdil · 2019-09-19T09:22:15Z

frontend/integration-tests/tests/metalkube/dashboard.scenario.ts

+    await browser.get(`${appHost}/dashboards`);
+    await dashboardView.isLoaded();
+    // wait until the counters in the inventory card show up
+    while (!(dashboardView.inventoryNodesUpCounter.isPresent() || dashboardView.inventoryNodesDownCounter.isPresent()) ||


The negations are quite hard to read. Are we waiting for all the counters to be present?
I think ExpectedConditions is the way to go

await browser.wait( until.and( dashboardView.inventoryNodesUpCounter.isPresent(), dashboardView.inventoryNodesDownCounter.isPresent(), dashboardView.inventoryHostsUpCounter.isPresent(), dashboardView.inventoryHostsDownCounter.isPresent(), ), 15000 // the timeout that we're willing to wait for all these elements );

jtomasek

Is there any dashboard specific tests suite already? It seems that the scenario which you're introducing could be split into a dashboard suite and metal3-plugin suite. metal3-plugin suite should hold tests which are specific to the baremetal environment.

jtomasek · 2019-10-24T13:15:23Z

frontend/integration-tests/protractor.conf.ts

@@ -113,6 +113,7 @@ export const config: Config = {
    return new Promise((resolve) => htmlReporter.afterLaunch(resolve.bind(this, exitCode)));
  },
  suites: {
+    baremetalSmokeTests: ['tests/metalkube/dashboard.scenario.ts'],


To follow the convention which kubevirt-plugin set it would be good to call this metal3-plugin and colocate the metal3-plugin tests into the plugin - move the test modules into frontend/packages/metal3-plugin/integration-tests and reference them here as

metal3-plugin: ['../packages/metal3-plugin/integration-tests/tests/dashboard.scenario.ts'],

jtomasek · 2019-10-25T16:48:36Z

frontend/packages/console-shared/src/components/dashboard/inventory-card/InventoryItem.tsx

@@ -102,7 +111,14 @@ const InventoryItem: React.FC<InventoryItemProps> = React.memo(
        </AccordionItem>
      </Accordion>
    ) : (
+<<<<<<< HEAD


Rebase leftover

jtomasek · 2019-10-25T16:50:16Z

frontend/public/components/dashboard/dashboards-page/overview-dashboard/inventory-card.tsx

-        <ClusterInventoryItem
-          model={PersistentVolumeClaimModel}
-          mapper={getPVCStatusGroups}
-          useAbbr
-        />


This one should not have been removed.

It wasn't. it's down below at line 142

I was wrong, fixed it.

jtomasek · 2019-10-25T17:11:59Z

frontend/packages/console-shared/src/components/dashboard/inventory-card/InventoryItem.tsx

@@ -102,7 +111,10 @@ const InventoryItem: React.FC<InventoryItemProps> = React.memo(
        </AccordionItem>
      </Accordion>
    ) : (
-      <div className="co-inventory-card__item">
+      <div
+        className="co-inventory-card__item co-inventory-card__item--border"


The second class should not be there

Why not? I didn't add it

This diff clearly shows you did :) It is a result of an incorrect rebase. The co-inventory-card__item--border class was recently removed by another patch and when you rebased, it is being unintentionally added back.

Fixed. Thanks

jtomasek · 2019-10-25T20:47:52Z

frontend/public/components/dashboard/dashboards-page/overview-dashboard/inventory-card.tsx

        <ClusterInventoryItem
          model={PersistentVolumeClaimModel}
          mapper={getPVCStatusGroups}
          useAbbr
        />


To preserve the order of the ClusterInventoryItems, this should go after StorageClassModel ClusterInventoryItem. Also data-test-id prop should get added to this item as well.

jtomasek · 2019-10-31T12:48:04Z

This needs to get updated to conform to #2051

This is the basic Protractor (Selenium) infrastructure to test the dashboards. Included is also a small test to read some of the counters in the dashboards page, and compare them to what we get from the CLI. This PR includes PRs 2287 and 2288 from Dan Trainor, who added custom data-test-id attributes on the elements inthe dashboards, to support the automation so it will be easier to find the elements. Also huge thanks to Honza Pokorny, Jason Rist and Jiri Tomasek who helped with the code and with rebases. Basic steps to run the tests: yarn install yarn webdriver-update export BRIDGE_BASE_ADDRESS='https://*.*.*.redhat.com' export BRIDGE_AUTH_USERNAME=kubeadmin export BRIDGE_AUTH_PASSWORD=22Z.... oc login -s *.*.*.redhat.com:6443 -u kubeadmin -p 22ZW.... export KUBECONFIG=/home/ukalifon/.kube/config export NO_HEADLESS=true # optional - if you want to see the browser yarn run test-suite --suite baremetalSmokeTests --params.openshift true

openshift-ci-robot · 2019-11-14T16:18:37Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ukalifon
To complete the pull request process, please assign spadgett
You can assign the PR to them by writing /assign @spadgett in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

frontend/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2020-09-21T11:09:28Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

openshift-ci-robot · 2020-10-06T12:44:56Z

@ukalifon: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-gcp	`aee82a4`	link	`/test e2e-gcp`
ci/prow/backend	`7a7209d`	link	`/test backend`
ci/prow/frontend	`7a7209d`	link	`/test frontend`
ci/prow/analyze	`7a7209d`	link	`/test analyze`
ci/prow/e2e-gcp-console	`7a7209d`	link	`/test e2e-gcp-console`
ci/prow/images	`7a7209d`	link	`/test images`
ci/prow/kubevirt-plugin	`7a7209d`	link	`/test kubevirt-plugin`

Full PR test history. Your PR dashboard.

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/test-infra repository. I understand the commands that are listed here.

openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 2, 2019

openshift-ci-robot requested review from jeff-phillips-18 and jhadvig July 2, 2019 12:43

spadgett added this to the v4.2 milestone Jul 2, 2019

openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 23, 2019

spadgett requested changes Jul 23, 2019

View reviewed changes

frontend/integration-tests/views/metalkube/dashboards.view.ts Outdated Show resolved Hide resolved

frontend/integration-tests/views/metalkube/dashboards.view.ts Outdated Show resolved Hide resolved

This was referenced Aug 7, 2019

[metal3] add data-test-id for dashboard components #2288

Closed

add data-test-id for console dashboard components #2287

Closed

ukalifon force-pushed the metal3-tests branch from cedad9a to 62e12e2 Compare August 8, 2019 13:23

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. component/core Related to console core functionality component/metal3 Related to metal3-plugin and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 8, 2019

ukalifon force-pushed the metal3-tests branch 2 times, most recently from 2a13aec to e7d846d Compare August 9, 2019 08:16

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2019

spadgett requested changes Aug 10, 2019

View reviewed changes

spadgett removed this from the v4.2 milestone Sep 9, 2019

ukalifon force-pushed the metal3-tests branch from e7d846d to d8b0101 Compare September 17, 2019 11:03

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2019

ukalifon force-pushed the metal3-tests branch from d8b0101 to b5430ca Compare September 17, 2019 12:59

rhrazdil suggested changes Sep 18, 2019

View reviewed changes

ukalifon force-pushed the metal3-tests branch from b5430ca to 50607a5 Compare September 18, 2019 14:45

rhrazdil suggested changes Sep 19, 2019

View reviewed changes

openshift-ci-robot added component/monitoring Related to monitoring component/noobaa Related to noobaa-storage-plugin component/olm Related to OLM component/sdk Related to console-plugin-sdk component/shared Related to console-shared labels Oct 23, 2019

knowncitizen force-pushed the metal3-tests branch from be8d78e to 2e47a2c Compare October 23, 2019 19:50

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 23, 2019

ukalifon force-pushed the metal3-tests branch from 2e47a2c to aee82a4 Compare October 24, 2019 12:43

jtomasek suggested changes Oct 24, 2019

View reviewed changes

ukalifon force-pushed the metal3-tests branch from aee82a4 to 04357dc Compare October 25, 2019 11:23

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 25, 2019

knowncitizen force-pushed the metal3-tests branch from 04357dc to 0bb41be Compare October 25, 2019 16:46

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 25, 2019

jtomasek suggested changes Oct 25, 2019

View reviewed changes

knowncitizen force-pushed the metal3-tests branch from 0bb41be to d6450fe Compare October 25, 2019 17:01

jtomasek suggested changes Oct 25, 2019

View reviewed changes

knowncitizen force-pushed the metal3-tests branch from d6450fe to 97d84f6 Compare October 25, 2019 19:58

jtomasek reviewed Oct 25, 2019

View reviewed changes

knowncitizen force-pushed the metal3-tests branch 3 times, most recently from c17a5dd to 2657874 Compare October 25, 2019 21:31

knowncitizen force-pushed the metal3-tests branch from 2657874 to 7a7209d Compare November 14, 2019 16:18

openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 21, 2020

ukalifon closed this Oct 7, 2020

	export const CLONE_VM_TIMEOUT = 300 * SEC;
	export const CLONE_VM_TIMEOUT_SECONDS = 300 * SEC;

Testing infra for metalkubed #1886

Testing infra for metalkubed #1886

Conversation

ukalifon commented Jul 2, 2019

openshift-ci-robot commented Jul 2, 2019

rhrazdil commented Jul 23, 2019

spadgett left a comment

Choose a reason for hiding this comment

ukalifon commented Jul 23, 2019

ukalifon commented Aug 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtomasek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtomasek commented Oct 31, 2019

openshift-ci-robot commented Nov 14, 2019

openshift-bot commented Sep 21, 2020

openshift-ci-robot commented Oct 6, 2020