bug 1832371: CNV-4046: Add integration test for vm environment tab #5280

irosenzw · 2020-05-04T15:59:50Z

Signed-off-by: Ido Rosenzwig irosenzw@redhat.com

rhrazdil · 2020-05-05T07:25:51Z

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

+  ConfigMapKind,
+  SecretKind,
+  ServiceAccountKind,
+} from '../../../../../public/module/k8s/types';


Could you use @Console instead of relative path please?

rhrazdil · 2020-05-05T07:32:50Z

frontend/integration-tests/views/environment.view.ts

+  await dropDownBtn
+    .filter(async (elem) => {
+      const elemText = await elem.getText();
+      return elemText === 'Select a resource';


I'm not sure if this is a good way to select a dropdown button, if I understand, this doesn't allow to change the selection in the dropdown for the second time, does it?

it allows you to change the selection. this is why I need to select the dropdown which have no value.

rhrazdil · 2020-05-05T07:49:56Z

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

+    createResource(secret);
+    createResource(configmap);
+    createResource(serviceAccount);
+    createExampleVMViaYAML();


there should be await before createExampleVMViaYAML

rhrazdil · 2020-05-05T07:58:10Z

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

+const addVariableButton = element(by.buttonText('Add All From Config Map or Secret'));
+
+export const clickAddSourceButton = async () => {
+  await browser.wait(until.elementToBeClickable(addVariableButton), PAGE_LOAD_TIMEOUT_SECS);


Instead if this method you can use click method that does the wait until.elementToBeClickable
await click(element(by.buttonText('Add All From Config Map or Secret')))

rhrazdil · 2020-05-05T08:19:23Z

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

+
+export const timeout = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms));
+
+export const startVM = async (vmName: string, ns: string) => {


I think we shouldn't re-implement this, it's better to create a VirtualMachine instance and use it's action method
const vm = new VirtualMachine();
await vm.action(VM_ACTION.Start);

rhrazdil · 2020-05-05T08:29:23Z

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

+  });
+
+  it('Verify all the sources are present at the disks tab', async () => {
+    await browser.get(`${appHost}/k8s/ns/${testName}/virtualmachines/${vmName}/disks`);


With the VirtualMachine instance, this can be substituted with

const disks = await vm.getAttachedDisks(); expect(disks).toContain( <configMapDisk> ); // you need to have configMapDisk in StorageResource type, or you can just search by name

rhrazdil · 2020-05-05T08:33:51Z

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

+export const successAlert = $('.pf-c-alert.pf-m-inline.pf-m-success.co-alert');
+export const errorAlert = $('.pf-c-alert.pf-m-inline.pf-m-danger.co-alert.co-alert--scrollable');
+
+export const startVMChoice = $('[data-test-action="Start Virtual Machine"]');


Unnecessary, we hav VM actions in packages/kubevirt-plugin/integration-tests/tests/utils/consts.ts in VM_ACTION

rhrazdil · 2020-05-05T08:37:35Z

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

@@ -51,7 +51,7 @@ export async function createProject(name: string) {
 }

 export async function createExampleVMViaYAML() {


I think it would be better to refactor createExampleVMViaYAML to return VirtualMachine object instance.
You could then re-use all the VirtualMachine navigation and other useful method (which I refer to comments later)

return new VirtualMachine({ name: DEFAULT_YAML_VM_NAME, namespace: testName });

rhrazdil · 2020-05-06T11:01:31Z

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

+  });
+
+  beforeEach(async () => {
+    await browser.get(`${appHost}/k8s/ns/${testName}/virtualmachines/${vmName}/environment`);


Please, add Environment tab to packages/kubevirt-plugin/integration-tests/tests/utils/consts.ts
and then you can use await vm.navigateToTab(TAB.Environment)

rhrazdil · 2020-05-06T11:03:40Z

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

+  });
+
+  it('Verify vm object was created', () => {
+    expect(!!vm).toBeTruthy();


I don't think this is necessary, moreover, I'd like the it specs to be feature-related only.
You could put the check to the beforeAll if you want.

rhrazdil · 2020-05-06T11:06:31Z

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

+
+    // Add Service Account
+    await browser.wait(until.elementToBeClickable(vmEnv.addVariableButton), PAGE_LOAD_TIMEOUT_SECS);
+    await click(element(by.buttonText('Add All From Config Map or Secret')));


You can pass the timeout to click function:
await click(element(by.buttonText('Add All From Config Map or Secret')), PAGE_LOAD_TIMEOUT_SECS);

rhrazdil · 2020-05-06T11:11:54Z

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

+    await vmEnv.serialField.get(1).clear();
+    await vmEnv.serialField.get(1).sendKeys('i', Key.BACK_SPACE); // workaround: for some reason clear() is not enough
+    await browser.wait(until.elementToBeClickable(vmEnv.saveBtn), PAGE_LOAD_TIMEOUT_SECS);
+    await vmEnv.saveBtn.click();


you could use await click(vmEnv.saveBtn, PAGE_LOAD_TIMEOUT_SECS);

rhrazdil · 2020-05-06T11:15:08Z

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

+    await browser.wait(until.presenceOf(vmEnv.errorAlert), PAGE_LOAD_TIMEOUT_SECS);
+    const errorText = await vmEnv.errorAlert.getText();
+    expect(vmEnv.errorAlert.isDisplayed()).toEqual(true);
+    expect(errorText.includes(vmEnv.noSerialError)).toEqual(true);


Optional:
expect(errorText.includes(vmEnv.noSerialError)).toEqual(true);
can be written as
expect(errorText).toContain(vmEnv.noSerialError);
which is slightly nicer to read.

rhrazdil · 2020-05-06T11:15:48Z

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

+    const firstSerial = await vmEnv.serialField.get(0).getAttribute('value');
+    await vmEnv.serialField.get(1).clear();
+    await vmEnv.serialField.get(1).sendKeys(firstSerial);
+    await browser.wait(until.elementToBeClickable(vmEnv.saveBtn), PAGE_LOAD_TIMEOUT_SECS);


again, you could use click as mentioned before

rhrazdil · 2020-05-06T11:16:11Z

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

+    await browser.wait(until.presenceOf(vmEnv.errorAlert), PAGE_LOAD_TIMEOUT_SECS);
+    expect(vmEnv.errorAlert.isDisplayed()).toEqual(true);
+    const errorText = await vmEnv.errorAlert.getText();
+    expect(errorText.includes(vmEnv.dupSerialsError)).toEqual(true);


you could use toContain as mentioned before

rhrazdil · 2020-05-06T11:18:50Z

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

+
+  it('Cannot use the same resource more than once', async () => {
+    await browser.wait(until.elementToBeClickable(vmEnv.addVariableButton), PAGE_LOAD_TIMEOUT_SECS);
+    await click(element(by.buttonText('Add All From Config Map or Secret')));


you could use click as mentioned before

rhrazdil · 2020-05-06T13:36:41Z

Looks good
/lgtm

rawagner · 2020-05-06T14:35:55Z

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

@@ -51,14 +53,23 @@ export async function createProject(name: string) {
  }
 }

-export async function createExampleVMViaYAML() {
+export async function createExampleVMViaYAML(getVMObj?: boolean): Promise<any> {


this returns a VM object not a Promise

Signed-off-by: Ido Rosenzwig <irosenzw@redhat.com>

rawagner · 2020-05-06T15:02:50Z

/approve

openshift-ci-robot · 2020-05-06T15:25:12Z

@irosenzw: This pull request references Bugzilla bug 1832371, 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.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

bug 1832371: CNV-4046: Add integration test for vm environment tab

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.

irosenzw · 2020-05-06T15:26:44Z

/bugzilla refresh

openshift-ci-robot · 2020-05-06T15:26:47Z

@irosenzw: This pull request references Bugzilla bug 1832371, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

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.

irosenzw · 2020-05-06T15:28:44Z

/bugzilla refresh

openshift-ci-robot · 2020-05-06T15:28:47Z

@irosenzw: This pull request references Bugzilla bug 1832371, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

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.

irosenzw · 2020-05-06T15:31:09Z

/bugzilla refresh

openshift-ci-robot · 2020-05-06T15:31:13Z

@irosenzw: This pull request references Bugzilla bug 1832371, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

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.

rawagner · 2020-05-06T16:25:12Z

/lgtm

openshift-ci-robot · 2020-05-06T16:25:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: irosenzw, rawagner, rhrazdil

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~~ [rawagner]

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-05-06T18:12:20Z

@irosenzw: All pull requests linked via external trackers have merged: openshift/console#5280. Bugzilla bug 1832371 has been moved to the MODIFIED state.

In response to this:

bug 1832371: CNV-4046: Add integration test for vm environment tab

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 component/core Related to console core functionality label May 4, 2020

openshift-ci-robot requested review from dtaylor113 and rawagner May 4, 2020 16:00

openshift-ci-robot added the component/kubevirt Related to kubevirt-plugin label May 4, 2020

rhrazdil reviewed May 5, 2020

View reviewed changes

irosenzw force-pushed the vm-env-disk-integration-tests branch 2 times, most recently from 8c309ed to 5e77c20 Compare May 6, 2020 10:51

rhrazdil suggested changes May 6, 2020

View reviewed changes

irosenzw force-pushed the vm-env-disk-integration-tests branch from 5e77c20 to 100c529 Compare May 6, 2020 13:24

openshift-ci-robot assigned rhrazdil May 6, 2020

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

irosenzw force-pushed the vm-env-disk-integration-tests branch from 100c529 to 3cd0ce6 Compare May 6, 2020 14:12

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label May 6, 2020

rawagner suggested changes May 6, 2020

View reviewed changes

CNV-4046: Add integration test for vm environment tab

d83d103

Signed-off-by: Ido Rosenzwig <irosenzw@redhat.com>

irosenzw force-pushed the vm-env-disk-integration-tests branch from 3cd0ce6 to d83d103 Compare May 6, 2020 14:42

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

irosenzw changed the title ~~CNV-4046: Add integration test for vm environment tab~~ bug 1832371: CNV-4046: Add integration test for vm environment tab May 6, 2020

openshift-ci-robot added bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels May 6, 2020

openshift-ci-robot added bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. and removed bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. labels May 6, 2020

openshift-ci-robot assigned rawagner May 6, 2020

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

openshift-merge-robot merged commit 353036d into openshift:master May 6, 2020

spadgett added this to the v4.5 milestone May 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bug 1832371: CNV-4046: Add integration test for vm environment tab #5280

bug 1832371: CNV-4046: Add integration test for vm environment tab #5280

irosenzw commented May 4, 2020

rhrazdil May 5, 2020

rhrazdil May 5, 2020

irosenzw May 6, 2020

rhrazdil May 5, 2020

irosenzw May 6, 2020

rhrazdil May 5, 2020

rhrazdil May 5, 2020

rhrazdil May 5, 2020

rhrazdil May 5, 2020

rhrazdil May 5, 2020

rhrazdil May 6, 2020

rhrazdil May 6, 2020

rhrazdil May 6, 2020

rhrazdil May 6, 2020

rhrazdil May 6, 2020

rhrazdil May 6, 2020

rhrazdil May 6, 2020

rhrazdil May 6, 2020

rhrazdil commented May 6, 2020

rawagner May 6, 2020

rawagner commented May 6, 2020

openshift-ci-robot commented May 6, 2020

irosenzw commented May 6, 2020

openshift-ci-robot commented May 6, 2020

irosenzw commented May 6, 2020

openshift-ci-robot commented May 6, 2020

irosenzw commented May 6, 2020

openshift-ci-robot commented May 6, 2020

rawagner commented May 6, 2020

openshift-ci-robot commented May 6, 2020

openshift-ci-robot commented May 6, 2020


		export const timeout = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms));

		export const startVM = async (vmName: string, ns: string) => {

		@@ -51,7 +51,7 @@ export async function createProject(name: string) {
		}

		export async function createExampleVMViaYAML() {

bug 1832371: CNV-4046: Add integration test for vm environment tab #5280

bug 1832371: CNV-4046: Add integration test for vm environment tab #5280

Conversation

irosenzw commented May 4, 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

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 May 6, 2020

Choose a reason for hiding this comment

rawagner commented May 6, 2020

openshift-ci-robot commented May 6, 2020

irosenzw commented May 6, 2020

openshift-ci-robot commented May 6, 2020

irosenzw commented May 6, 2020

openshift-ci-robot commented May 6, 2020

irosenzw commented May 6, 2020

openshift-ci-robot commented May 6, 2020

rawagner commented May 6, 2020

openshift-ci-robot commented May 6, 2020

openshift-ci-robot commented May 6, 2020