Adding test cases for VM actions #2125

rhrazdil · 2019-07-23T11:11:57Z

Adding test cases for VM actions

start, stop, restart, delete
migration
adding/removing disks on Disks tab
adding/removing nics on Netwok interfaces tab

This PR depends on #2053.
Currently, there are the following bugs that are blocking these test from passing:
https://bugzilla.redhat.com/show_bug.cgi?id=1731480 - fix merged
https://bugzilla.redhat.com/show_bug.cgi?id=1731340 - fix merged

rhrazdil · 2019-07-24T10:55:32Z

/test e2e-aws
/test e2e-aws-console-olm
/test e2e-aws-console

rhrazdil · 2019-07-25T13:14:52Z

/test e2e-aws-console-olm

rhrazdil · 2019-08-06T07:06:56Z

/test e2e-aws-console
/test e2e-aws

droslean · 2019-09-03T17:52:41Z

/test e2e-aws-console-olm

droslean · 2019-09-03T17:52:50Z

/test e2e-aws-console

mareklibra · 2019-09-04T08:06:22Z

frontend/packages/console-shared/src/test-utils/utils.ts

+ * @returns   {boolean}             True if found, false otherwise.
+ */
+export function searchYAML(needle: string, name: string, namespace: string, kind: string): boolean {
+  const result = execSync(`oc get -o yaml -n ${namespace} ${kind} ${name}`).toString();


I understand the power of a function like that but I am a little bit worried about this dependency on the oc tool.
It requires extra configuration step which should be documented.

At least path to it should be configurable (env variable?).

Anyway, I think it would be better to rely on existing API connection, i.e. via k8sGet() function. Wdyt?

I'm not sure if it's necessary to use API because openshift core integration-tests use kubectl quite often. But I agree we should use kubectl rather than oc here.

ok, makes sense. As long as we don't need specifics of the oc, the kubectl is better option. I see it used in the core tests now, so we would not introduce new dependency this way.

I am a little bit worried about this dependency on the oc tool.

openshift core integration-tests use kubectl quite often.

@mareklibra @rhrazdil @jelkosz @spadgett

Console integration tests should use kubectl instead of oc - this reflects the expectation that Console should work without issues on vanilla k8s clusters. (Console's primary target is OpenShift, but we want to keep compatibility with vanilla k8s as well.)

AFAIK, when you install oc it also creates a link to kubectl so the same tests should work for both vanilla k8s and OpenShift.

As long as we don't need specifics of the oc, the kubectl is better option.

👍

mareklibra · 2019-09-04T08:07:21Z

frontend/packages/kubevirt-plugin/integration-tests/tests/models/detailView.ts

@@ -25,12 +25,15 @@ export class DetailView {
      await browser.get(`${appHost}/k8s/ns/${this.namespace}/${this.kind}/${this.name}`);
      await isLoaded();
    }
-    await clickHorizontalTab(tabName);
-    await isLoaded();
+    if ((await $('.co-m-horizontal-nav-item--active').getText()) !== tabName) {


This can be wrapped by a view function, like the rest of the code is doing.

mareklibra · 2019-09-04T08:09:48Z

frontend/packages/kubevirt-plugin/integration-tests/tests/models/kubevirtDetailView.ts

    await kubevirtDetailView.selectKebabOption(name, 'Delete');
    await confirmAction();
+    do {
+      await browser.sleep(50);


Why 50? Can we find a more reliable check than just a delay?

mareklibra · 2019-09-04T08:13:07Z

frontend/packages/kubevirt-plugin/integration-tests/tests/vm.actions.scenario.ts

+  });
+});
+
+describe('Test VM Migration', () => {


This should go either under Test VM actions scenario or be moved in a separate file (which is preferred, imo).

mareklibra · 2019-09-04T08:13:55Z

frontend/packages/kubevirt-plugin/integration-tests/tests/vm.actions.scenario.ts

+  );
+});
+
+describe('Add/remove disks and NICs on respective VM pages', () => {


mareklibra · 2019-09-04T08:14:15Z

frontend/packages/kubevirt-plugin/integration-tests/tests/vm.actions.scenario.ts

+    VM_ACTIONS_TIMEOUT_SECS * 2,
+  );
+
+  xit(


xit skips a spec and highlights it in the test report as skipped.
It's skipped due to the opened BZ

If possible, please link to the BZ blocking this test case via comment.

Edit: nevermind, it's part of test description 😄 my bad.

mareklibra · 2019-09-04T08:14:59Z

frontend/packages/kubevirt-plugin/integration-tests/tests/vm.actions.scenario.ts

+    // Verify the NIC is added in VM Manifest
+    const resource = getResourceObject(vm.name, vm.namespace, vm.kind);
+    const nic = _.find(
+      _.get(resource, 'spec.template.spec.domain.devices.interfaces', []),


I believe we have a selector for this. If not, can we add?

Could you point to the selectors? I don't know what you mean

https://github.com/openshift/console/tree/master/frontend/packages/kubevirt-plugin/src/selectors

In particular, this one seems to be relevant:

console/frontend/packages/kubevirt-plugin/src/selectors/vm/selectors.ts

Line 22 in c0c95dd

export const getInterfaces = (vm: VMKind) =>

Either that or console-shared selectors, might check out that one as well.

mareklibra · 2019-09-04T08:18:47Z

frontend/packages/kubevirt-plugin/integration-tests/tests/vm.actions.scenario.ts

+      vmi = await vm.navigateToVmi(TABS.OVERVIEW);
+      expect((await vmi.getVolumes()).includes(hddDisk.name)).toBe(false);
+    },
+    VM_ACTIONS_TIMEOUT_SECS * 2,


Just to understand: why * 2 ?

VM is restarted twice

VM_ACTIONS_TIMEOUT_SECS * 2, // VM is restarted twice

mareklibra · 2019-09-04T08:21:42Z

frontend/packages/kubevirt-plugin/integration-tests/tests/vm.actions.scenario.ts

+
+    // Try to add the NIC again
+    await click(createNic, 1000);
+    await browser.sleep(1000).then(() => browser.wait(until.presenceOf($(networkTypeDropdownId))));


I have hard time to understand this flow without spending time on trying it. Can you please briefly describe these 2 lines (like what is the starting point and what is being tested here) ?

We test that it's not possible to add 2 NICs that use the same network (net-attach-def)

mareklibra · 2019-09-04T08:25:40Z

frontend/packages/kubevirt-plugin/integration-tests/views/kubevirtDetailView.view.ts

-    .$$('a')
+  await tableRowForName(name)
+    .$('.dropdown-menu-right')
+    .$$('button')


I think this function is used in other tests as well or at least in other pending PRs.
Can you please make sure that existing tests are not affected? Is this "new" structure generic for a kebab or comes from the just considered scenario?

This is generic, all other PRs need to use this implementation.

atiratree · 2019-09-04T10:31:33Z

frontend/packages/kubevirt-plugin/integration-tests/tests/models/kubevirtDetailView.ts

+    const resources: StorageResource[] = [];
+    const rows = await kubevirtDetailView.tableRows();
+    rows.forEach((line) => {
+      const cols = line.split(/\s+/);


can you please explain how is the table text and rows are resolved? How come we get the columns just by splitting it by spaces?

Well calling getText() on the table returns string in the following format

attr00 attr01 ... attr0n\n attr10 attr11 ... attr1n\n

So forEach iterates over lines and thus I can split the columns by spaces.

atiratree · 2019-09-04T10:32:19Z

frontend/packages/kubevirt-plugin/integration-tests/tests/models/kubevirtDetailView.ts

-      const cells = row.$$('div');
+    const resources: StorageResource[] = [];
+    const rows = await kubevirtDetailView.tableRows();
+    rows.forEach((line) => {


we can use a map function now

I find forEach easier to read

Okay I used map after all.

atiratree · 2019-09-04T10:36:24Z

frontend/packages/kubevirt-plugin/integration-tests/tests/models/kubevirtDetailView.ts

    await kubevirtDetailView.selectKebabOption(name, 'Delete');
    await confirmAction();
+    do {
+      await browser.sleep(50);
+    } while ((await resourceRows.count()) !== count - 1);


is there some timeout which would trigger failure if this action fails to remove the disk?

Yes, the timeout of the spec that executes this, but I have changed it to adress Marek's review, so this is no longer relevant

atiratree · 2019-09-04T10:46:05Z

frontend/packages/kubevirt-plugin/integration-tests/tests/vm.actions.scenario.ts

+            until.presenceOf(statusIcon(statusIcons.error)),
+          ),
+        );
+        await browser.wait(


why is the running -> error -> running flow ok? Shouldn't we check against that?

I have noticed that sometimes container VM ends up briefly in error state during restart and since container VM boots quite quickly and the status is not updated so often, the VM goes from error straight to running, which broke the automation because it was waiting for starting phase.

atiratree · 2019-09-04T10:47:58Z

frontend/packages/kubevirt-plugin/integration-tests/tests/vm.actions.scenario.ts

+    it(
+      'Starts VM',
+      async () => {
+        await listViewAction(vmName)('Start', true);


can the detail actions be reused here?

rhrazdil · 2019-09-04T14:29:16Z

/test e2e-aws-console

mareklibra · 2019-09-05T13:20:02Z

/lgtm

mareklibra · 2019-09-05T13:21:12Z

/assign @kyoto

vojtechszocs · 2019-09-05T14:56:54Z

frontend/packages/kubevirt-plugin/integration-tests/tests/models/virtualMachine.ts


 export class VirtualMachine extends KubevirtDetailView {
  constructor(config) {
    super({ ...config, kind: 'virtualmachines' });
  }

+  async navigateToVmi(vmiTab: string): Promise<VirtualMachineInstance> {


Nit: should be navigateToVMI.

vojtechszocs · 2019-09-05T15:27:01Z

Change diff looks OK, thanks for addressing all review comments.

/lgtm
/approve

vojtechszocs · 2019-09-05T15:32:12Z

/lgtm

We missed some lint issues, it's fixed now.

openshift-ci-robot · 2019-09-05T15:32:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mareklibra, rhrazdil, suomiy, vojtechszocs

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:

~~frontend/OWNERS~~ [vojtechszocs]

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

openshift-bot · 2019-09-05T19:31:08Z

/retest

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

spadgett · 2020-04-03T17:26:31Z

frontend/packages/console-shared/src/test-utils/utils.ts

+ * @param     {string}    kind      Kind of the resource.
+ * @returns   {boolean}             True if found, false otherwise.
+ */
+export function searchYAML(needle: string, name: string, namespace: string, kind: string): boolean {


I know this is an old PR that's already merged, but this isn't the way we should test for content in the resources. Instead, we should

Get the JSON with -o json

Parse the JSON

Check the specific property we're interested in

The searchYAML approach is bad because it can result in incorrect failures due to different quoting styles in the YAML or it can result in false positives where the string is matched somewhere else in the document.

Can we look at removing this utility and replacing its usage? At the very least, I'd like to move this out of console-shared to discourage its use elsewhere.

@rhrazdil

I agree, it's currently only used in old kubevirt tests, I'll update those and remove this method from console-shared

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 Jul 23, 2019

openshift-ci-robot requested review from mareklibra and rhamilto July 23, 2019 11:12

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 23, 2019

rhrazdil force-pushed the console-vmactions-tests branch from b57cbc5 to 375a3c8 Compare August 5, 2019 07:21

openshift-ci-robot added the component/kubevirt Related to kubevirt-plugin label Aug 5, 2019

rhrazdil force-pushed the console-vmactions-tests branch 3 times, most recently from 9a31e50 to 740ba14 Compare August 5, 2019 12:10

rhrazdil mentioned this pull request Aug 8, 2019

[RFR] Add tests for kubevirt VM Clone dialog #2298

Merged

rhrazdil force-pushed the console-vmactions-tests branch from 740ba14 to 9ae25c4 Compare August 9, 2019 11:39

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

rhrazdil force-pushed the console-vmactions-tests branch from 9ae25c4 to 7defeb1 Compare September 2, 2019 11:55

rhrazdil force-pushed the console-vmactions-tests branch from 7defeb1 to 2bf53ac Compare September 3, 2019 14:48

rhrazdil changed the title ~~[WIP] Adding test cases for VM actions~~ Adding test cases for VM actions Sep 3, 2019

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 Sep 3, 2019

rhrazdil force-pushed the console-vmactions-tests branch from 2bf53ac to 7f5db6d Compare September 3, 2019 15:07

mareklibra reviewed Sep 4, 2019

View reviewed changes

atiratree reviewed Sep 4, 2019

View reviewed changes

rhrazdil force-pushed the console-vmactions-tests branch from 7f5db6d to 764a0b0 Compare September 4, 2019 12:32

rhrazdil force-pushed the console-vmactions-tests branch 3 times, most recently from c63dd25 to 743049a Compare September 4, 2019 13:30

atiratree approved these changes Sep 4, 2019

View reviewed changes

openshift-ci-robot assigned mareklibra Sep 5, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 5, 2019

openshift-ci-robot assigned kyoto Sep 5, 2019

vojtechszocs suggested changes Sep 5, 2019

View reviewed changes

rhrazdil force-pushed the console-vmactions-tests branch from 743049a to 3cb0e8f Compare September 5, 2019 15:24

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 5, 2019

openshift-ci-robot assigned vojtechszocs Sep 5, 2019

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 5, 2019

Adding VM actions tests

7b16958

rhrazdil force-pushed the console-vmactions-tests branch from 3cb0e8f to 7b16958 Compare September 5, 2019 15:31

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 5, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 5, 2019

openshift-merge-robot merged commit 0680813 into openshift:master Sep 6, 2019

spadgett added this to the v4.2 milestone Sep 6, 2019

spadgett reviewed Apr 3, 2020

View reviewed changes

rhrazdil mentioned this pull request Apr 16, 2020

Remove method searchYAML from console-shared #5074

Merged

Adding test cases for VM actions #2125

Adding test cases for VM actions #2125

Conversation

rhrazdil commented Jul 23, 2019 • edited Loading

rhrazdil commented Jul 24, 2019

rhrazdil commented Jul 25, 2019

rhrazdil commented Aug 6, 2019

droslean commented Sep 3, 2019

droslean commented Sep 3, 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

vojtechszocs Sep 5, 2019 • edited Loading

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

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

rhrazdil commented Sep 4, 2019

mareklibra commented Sep 5, 2019

mareklibra commented Sep 5, 2019

Choose a reason for hiding this comment

vojtechszocs commented Sep 5, 2019

vojtechszocs commented Sep 5, 2019

openshift-ci-robot commented Sep 5, 2019

openshift-bot commented Sep 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhrazdil commented Jul 23, 2019 •

edited

Loading

vojtechszocs Sep 5, 2019 •

edited

Loading