Bug 1798862: VMI list integration tests #4209

yaacov · 2020-02-05T15:45:18Z

A test to check that we see VMs and VMIs in the Virtual Machine list page

yaacov · 2020-02-05T15:45:40Z

@rhrazdil please review

openshift-ci-robot · 2020-02-06T07:28:45Z

@yaacov: This pull request references Bugzilla bug 1798862, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Bug 1798862: VMI list integration tests

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 · 2020-02-06T11:49:07Z

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

@@ -28,8 +28,8 @@ import { KubevirtDetailView } from './kubevirtDetailView';
 import { ImportWizard } from './importWizard';

 export class VirtualMachine extends KubevirtDetailView {
-  constructor(config) {
-    super({ ...config, kind: 'virtualmachines' });
+  constructor(config, kind?: 'virtualmachines' | 'virtualmachineinstances') {


It's an interesting idea. I'm just wondering whether we should or should not differentiate the type of VirtualMachine and VirtualMachineInstance more explicitly.
@suomiy wdyt?

whether we should or should not differentiate the type of VirtualMachine and VirtualMachineInstance more explicitly.

The page rendering VM and VMI share the same component, it renders a vmiLike object that can be VM or VMI. When rendering we have small changes, mainly page title, how status is calculated, and what is allowed to be edit:
https://github.com/openshift/console/blob/master/frontend/packages/kubevirt-plugin/src/components/vms/vm-details.tsx#L72

IMHO it makes sense to share the testing methods of this components too.

VM and VMI objects are very similar, not the same though. You can't do the same (or any basically) actions with VMI, which would be nice to have reflected in our testing framework.
But it could be achieved this way as well.

yes there will be differences in actions and also editing and we should reflect that and not use the same class for them. Let's figure out the best approach

ok; we decided to keep it simple for now after offline discussion.

Radim will revisits this once his VMBuilder PR is up.

Let's just think of it as temporary solution for now.

atiratree · 2020-02-06T15:22:57Z

frontend/packages/kubevirt-plugin/integration-tests/tests/utils/mocks.ts

@@ -237,6 +238,49 @@ export function getVMManifest(
      throw Error('Provision source not Implemented');
  }

+  const spec = {


can you please rename this to vmiSpec ?

atiratree · 2020-02-06T15:27:23Z

frontend/packages/kubevirt-plugin/integration-tests/tests/utils/mocks.ts

+    spec,
+  };
+
+  return kind === 'VirtualMachineInstance' ? vmiResource : vmResource;


can we rather have two functions getVMManifest and getVMIManifest which would use common settings of them?

atiratree · 2020-02-06T15:30:08Z

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

@@ -28,8 +28,8 @@ import { KubevirtDetailView } from './kubevirtDetailView';
 import { ImportWizard } from './importWizard';

 export class VirtualMachine extends KubevirtDetailView {
-  constructor(config) {
-    super({ ...config, kind: 'virtualmachines' });
+  constructor(config, kind?: 'virtualmachines' | 'virtualmachineinstances') {


yes there will be differences in actions and also editing and we should reflect that and not use the same class for them. Let's figure out the best approach

atiratree · 2020-02-06T15:33:14Z

frontend/packages/kubevirt-plugin/integration-tests/tests/vmi.list-filtering.scenario.ts

+  );
+
+  beforeAll(async () => {
+    await waitForVM(testVM, VM_STATUS.Off, leakedResources);


should this be really in before all block? Shouldn't we also check the vmi status change?

should this be really in before all block?

The test is to filter by status, knowing the the vmi is Running while the vm is Off
So we need one vm in Off status and one vmi in Running status, before we start the tests.

Shouldn't we also check the vmi status change?

vmi is always running ( if evrything is ok :-) )

atiratree · 2020-02-06T15:35:21Z

frontend/packages/kubevirt-plugin/integration-tests/tests/vmi.list-filtering.scenario.ts

+  await isLoaded();
+};
+
+describe('Test List View Filtering (VMI)', () => {


not sure if the naming applies ^^, we are not filtering anything - we show all of the VMs and VMIs at once

we are not filtering anything

We are filtering by status, we have 1 Off vm and 1 Running vmi,
A side affect of this filtering procedure is the we know for sure the we list both vm and vmi in one list.

I mean the name implies that we are testing the filtering feature of the UI. We are just reading the values and not testing it

Correct, we are actually testing vmStatusFilter that filter the vm/vmi list, and not the actual filtering feature of the UI , implemented in MultiListPage.

This actually makes sense here because the UI filtering is a core feature that we should not test in our module, maybe we should rename the test to make it clear we test vmStatusFilter and not MultiListPage that does the UI filtering ?

@rhrazdil @suomiy WDYT ?

It's true that name like: Test List View Filter counts or similar would probably better describe what is done here.
I would keep the test names e2e oriented from user's perspective. IMO there is no need to make distinction betweem our plugin vs core in the test cases names.

yaacov · 2020-02-10T10:09:00Z

/test e2e-gcp-console

yaacov · 2020-02-11T11:46:38Z

/test e2e-gcp-console

rhrazdil · 2020-02-12T08:22:29Z

frontend/packages/kubevirt-plugin/integration-tests/tests/vmi.list-filtering.scenario.ts

+  it('Displays correct count of Running VMIs', async () => {
+    const vmiImportingCount = await filterBoxCount(VM_STATUS.Running);
+    expect(vmiImportingCount).toEqual(1);
+  });


Now when I think about it, it may be a good idea to add one more test case that the VMI is displayed in the list.

Something like:

import { resourceRowsPresent, textFilter, } from '@console/internal-integration-tests/views/crud.view'; it('Displays VMIs in the list of VirtualMachines', async () => { await fillInput(textFilter, testVMI.metadata.name); await resourceRowsPresent(); });

Thanks 👍

yaacov · 2020-02-12T11:50:47Z

/test e2e-gcp-console

yaacov · 2020-02-12T13:40:56Z

/test e2e-gcp-console

rhrazdil · 2020-02-12T15:57:14Z

/lgtm

openshift-ci-robot · 2020-02-12T16:06:21Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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/packages/kubevirt-plugin/OWNERS~~ [rhrazdil,suomiy,yaacov]

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

openshift-ci-robot · 2020-02-12T17:48:52Z

@yaacov: All pull requests linked via external trackers have merged. Bugzilla bug 1798862 has been moved to the MODIFIED state.

In response to this:

Bug 1798862: VMI list integration tests

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.

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 5, 2020

yaacov force-pushed the vmi-list-tests branch from 726226a to 0c39097 Compare February 5, 2020 15:52

openshift-ci-robot added the component/kubevirt Related to kubevirt-plugin label Feb 5, 2020

openshift-ci-robot requested review from rhrazdil and vojtechszocs February 5, 2020 16:21

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 5, 2020

yaacov changed the title ~~VMI list integration tests~~ Bug 1798862: VMI list integration tests Feb 6, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Feb 6, 2020

rhrazdil reviewed Feb 6, 2020

View reviewed changes

atiratree reviewed Feb 6, 2020

View reviewed changes

yaacov mentioned this pull request Feb 10, 2020

Bug 1801142: Vmi dashboard tests #4265

Merged

rhrazdil reviewed Feb 12, 2020

View reviewed changes

yaacov added 7 commits February 12, 2020 10:24

vmi list integration tests

ee7c694

cutifier

0baf950

rename spec to vimSpec, not overloading getVMManifest

40695b3

fix spec to vmiSpec

b0ab5cd

restrict provisionSource

44b1cc6

no finilazers for vmi

40fad2e

add actuall is vmi listed test

a7ee6c7

yaacov force-pushed the vmi-list-tests branch from c90e8f8 to a7ee6c7 Compare February 12, 2020 09:42

atiratree approved these changes Feb 12, 2020

View reviewed changes

openshift-ci-robot assigned rhrazdil Feb 12, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 12, 2020

openshift-merge-robot merged commit 0d80328 into openshift:master Feb 12, 2020

spadgett added this to the v4.4 milestone Feb 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1798862: VMI list integration tests #4209

Bug 1798862: VMI list integration tests #4209

yaacov commented Feb 5, 2020

yaacov commented Feb 5, 2020

openshift-ci-robot commented Feb 6, 2020

rhrazdil Feb 6, 2020

yaacov Feb 6, 2020

rhrazdil Feb 6, 2020

atiratree Feb 6, 2020

atiratree Feb 6, 2020

atiratree Feb 6, 2020 •

edited

yaacov Feb 6, 2020

atiratree Feb 6, 2020

yaacov Feb 6, 2020

atiratree Feb 6, 2020

atiratree Feb 6, 2020

yaacov Feb 6, 2020

atiratree Feb 7, 2020

atiratree Feb 6, 2020

yaacov Feb 6, 2020

atiratree Feb 7, 2020

yaacov Feb 10, 2020

rhrazdil Feb 11, 2020 •

edited

yaacov commented Feb 10, 2020

yaacov commented Feb 11, 2020

rhrazdil Feb 12, 2020

yaacov Feb 12, 2020

yaacov commented Feb 12, 2020

yaacov commented Feb 12, 2020

rhrazdil commented Feb 12, 2020

openshift-ci-robot commented Feb 12, 2020

openshift-ci-robot commented Feb 12, 2020

Bug 1798862: VMI list integration tests #4209

Bug 1798862: VMI list integration tests #4209

Conversation

yaacov commented Feb 5, 2020

yaacov commented Feb 5, 2020

openshift-ci-robot commented Feb 6, 2020

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

atiratree Feb 6, 2020 • edited

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 Feb 11, 2020 • edited

Choose a reason for hiding this comment

yaacov commented Feb 10, 2020

yaacov commented Feb 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaacov commented Feb 12, 2020

yaacov commented Feb 12, 2020

rhrazdil commented Feb 12, 2020

openshift-ci-robot commented Feb 12, 2020

openshift-ci-robot commented Feb 12, 2020

atiratree Feb 6, 2020 •

edited

rhrazdil Feb 11, 2020 •

edited