Testing infra for metalkubed

[ukalifon]

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]

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.

Details

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]

/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.

[ukalifon]

/retest

[ukalifon]

/test ci/prow/e2e-aws

[spadgett]

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

[rhrazdil]

@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]

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

[spadgett]

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

[spadgett]

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]

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]

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

[rhrazdil]

@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]

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

[spadgett]

Better to use -o json and check the JSON values

[spadgett]

Same comment here. Please use -o json

[spadgett]

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

[rhrazdil]

maybe just compareCounter since it only takes one element.

[rhrazdil]

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]

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]

Rebase leftover

[knowncitizen]

Fixed

[jtomasek]

This one should not have been removed.

[knowncitizen]

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

[knowncitizen]

I was wrong, fixed it.

[jtomasek]

The second class should not be there

[knowncitizen]

Why not? I didn't add it

[jtomasek]

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.

[knowncitizen]

Fixed. Thanks

[jtomasek]

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.

[knowncitizen]

Done

[jtomasek]

This needs to get updated to conform to #2051

[openshift-ci-robot]

[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.

Details

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]

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]

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

Test name	Commit	Details	Rerun command
ci/prow/e2e-gcp	aee82a47fad590476970e0664166f969f4b8387b	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.

Details

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.