Bug 1883766: Adjust tests for UI changes #6788

gouyang · 2020-09-30T07:58:16Z

No description provided.

openshift-ci-robot · 2020-09-30T07:58:20Z

@gouyang: This pull request references Bugzilla bug 1883766, which is invalid:

expected the bug to target the "4.6.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

Bug 1883766: Adjust tests for UI changes

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 · 2020-09-30T07:58:38Z

Hi @gouyang. 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.

yaacov · 2020-09-30T13:08:53Z

/ok-to-test

frontend/packages/kubevirt-plugin/integration-tests/tests/dialogs/diskDialog.ts

atiratree · 2020-10-01T10:08:00Z

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

  },
-  [ProvisionSource.PXE]: { method: ProvisionSource.PXE },
-  [ProvisionSource.DISK]: { method: ProvisionSource.DISK },
+  [ProvisionSource.PXE]: { method: ProvisionSourceItem.PXE },


what benefit do we get from splitting ProvisionSource into 2 constants? This will result in worse maintainability.

Can we either

rename the original to what we need: URL (adds disk), etc.

refactor the ProvisionSource to become ObjectEnum so it has all the required information in one place (name, label and source)

I added ProvisionSourceItem because string like URL (adds disk) cannot be used as the key.
Will look into ObjectEnum.

Why not? Is there a specific reason why it cannot be used as a key?

If using URL (adds disk) , it throws errors very quickly:

[10:33:24] E/launcher - Error: TypeError: Cannot read property 'source' of undefined at Object.exports.getTestDataVolume (/home/gouyang/console/frontend/packages/kubevirt-plugin/integration-tests/tests/mocks/mocks.ts:102:37)

@suomiy Could you give me an example how to use ObjectEnum?

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

atiratree · 2020-10-01T10:13:43Z

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

-    await selectOptionByText(view.provisionSourceSelect, provisionOptions.method);
+    await selectItemFromDropdown(
+      view.provisionSourceSelect,
+      view.bootSourceDropDownItem(provisionOptions.method),


can we call it provisionSourceDropDownItem for consistency reasons? I know we renamed it in the UI. but we should be consistent here IMO.

Or we should rename all the occurrences here

I used the code listed here:

async selectOperatingSystem(operatingSystem: string) { await selectItemFromDropdown(view.operatingSystemSelect, view.dropDownItem(operatingSystem)); }

and it couldn't select OS when running VMWare wizard

can we call it provisionSourceDropDownItem for consistency reasons? I know we renamed it in the UI. but we should be consistent here IMO.

ok.

and it couldn't select OS when running VMWare wizard

I guess the style of OS list in VMWare wizard is different from vm Wizard, if so, I think the best way is making the style consistent.

It is using the same component in the code. @ibragins can you check if it renders different html/styles for you?

atiratree · 2020-10-01T10:13:59Z

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

+    );
    if (Object.prototype.hasOwnProperty.call(provisionOptions, 'source')) {
-      await fillInput(view.provisionSourceInputs[provisionOptions.method], provisionOptions.source);
+      if (provisionOptions.method === ProvisionSourceItem.URL) {


is this necessary? can we keep the original form?

ok. will do

The reason is the value of provisionOptions.method for URL is URL (adds disk), provisionSourceInputs only has URL and Container, so do it this way.

The original form does not work.

atiratree · 2020-10-01T10:16:20Z

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

-      // ProvisionSource, OS and workload are prefilled and disabled - ignoring them
-    } else if (template) {
-      await this.selectTemplate(template);
+    if (os) {


please can you show me, where is the logic for the template now? We are still offering this option.

The wizard field is removed by 1871103, so remove it here as well.

the field was removed, but the wizard is still used for creating VMs from the template. So it should still not be possible to select os/workload/etc for these VMs. This applies for other logic as well. Only the selection should be removed.

Still we could automate this and add the template field tot the build and it would just go to the template page and select the create VM action

Removed field template works well for create vm from template or not from template based on the new changes and the tests are PASS for vm.wizard and vmtemplate.wizard:

template is removed in vm wizard

the new link/button to create vm from template

atiratree · 2020-10-01T10:16:44Z

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

      await this.addNIC(resource);
    }
-    if (provisionSource?.method === ProvisionSource.PXE && template === undefined) {
+    if (provisionSource?.method === ProvisionSourceItem.PXE) {


I used the code listed here:

async selectOperatingSystem(operatingSystem: string) { await selectItemFromDropdown(view.operatingSystemSelect, view.dropDownItem(operatingSystem)); }

and it couldn't select OS when running VMWare wizard

No need to use template here based on the actual test results.

atiratree · 2020-10-01T10:20:05Z

frontend/packages/kubevirt-plugin/integration-tests/tests/vmtemplate.resources.scenario.ts

-    .setTemplate(vmt.name)
-    .setName('from-template')
-    .build();
+  const vm = new VMBuilder(getBasicVMBuilder()).setName('from-template').build();


there is a new flow for creating vm's from templates: through /k8s/ns/default/virtualization/templates or k8s/ns/default/vmtemplates/vm-template-example.

We should test the new way of creating vms from templates in all our scenarios

we have the test ID(CNV-4290) Creates VM using VM Template create virtual machine link in vmtemplate.wizard.scenario.ts

yes, but this test tests resources (disks/nics) and should also do that with template flow in mind

No need to use template here based on the actual test results.

can you please specify, why is there no need to test templates?

It does not skip/ignore template test, I mean no need to use template field.

} else if (template) { await this.selectTemplate(template); } else {

await this.selectTemplate(template); will never be executed as we don't have this field. If remove this line and leave this block empty does not make sense and is not accepted by eslint.

To create vm from template should use:

await vmt.createVMFromRowLink(); await wizard.processWizard(vm.getData());

It does not need to have template at all.

I get that the template option is not in the wizard, but it is still possible to create a VM from the template

We have tests to create vm from template and it does not require template field to create vm from template.

you need to know the name of the template to select it. The implementation (wizard field or clicking on the template row) is less important.
But having one config in one place is always better IMO.

atiratree · 2020-10-01T10:21:18Z

frontend/packages/kubevirt-plugin/integration-tests/tests/vmtemplate.wizard.scenario.ts

          const vm = new VMBuilder()
            .setNamespace(testName)
            .setDescription(`VM from template ${vmt.name}`)
            .setFlavor(flavorConfigs.Tiny)


we will still need to know the name of the template to create the VM from

Will check this late.

No need to use template here based on the actual test results.

openshift-ci-robot · 2020-10-01T10:22:35Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: gouyang
To complete the pull request process, please assign suomiy after the PR has been reviewed.
You can assign the PR to them by writing /assign @suomiy 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/packages/kubevirt-plugin/OWNERS

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

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

irosenzw · 2020-10-01T11:17:50Z

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

 Object.freeze(networkTabCol);

 // Storage
 export enum DISK_SOURCE {


I don't think its a good idea maintaining these enum s here...
Every time something changes in src/ and it isn't updated here the tests break.

maybe we can have these enum s declared in src/ and imported from there.
I created a small PR for the disks as an example of what I think should be done: 7d8179c

It might be out of scope for this PR but I think it is worth doing it soon.

Please let's not mix src and tests code right now. This needs a proper discussion and evaluation of pros/cons.

The main issue with this is that the tests should not depend and be bound on the implementation. This can hurt when refactoring things and eg adding i18n.

If you bring bugs into the code it can bring bugs to the tests and it might be hard to discover them. For example we had a discussion about bringing VMWrappers into the tests, but decided against it for this exact reason.

The main hardship is that we are not commiting code with the tests at the same. Once we have the CI, such renaming issue would get picked up immediately and such code would not get merged in the first place. So there is less pressure for sharing the constants IMO.

that's a good point. although I didn't referred to wrappers/classes/interfaces/types but only to enums that have text in the them. Those things can change more rapidly in the code - we can see how many strings had changed in this PR only.
IMO it would be nice to update these text variables in one place.

I know it would be easier for now to do it like you suggest. But will get harder once you start introducing i18n.
Also, it would start a precedence with sharing the code, which might be confusing for newcomers.

Anyway, we can discuss this in a meeting next week if you want.

atiratree · 2020-10-09T08:04:03Z

@gouyang btw @irosenzw started working on this patch as well in #6850 . Can you guys please sync, so you don't do the same work twice?

openshift-ci-robot · 2020-10-09T08:59:55Z

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

Test name	Commit	Details	Rerun command
ci/prow/kubevirt-plugin	`8c3d8c4`	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.

gouyang · 2020-10-09T09:21:04Z

@gouyang btw @irosenzw started working on this patch as well in #6850 . Can you guys please sync, so you don't do the same work twice?

Thanks for the reminder, I will review #6850 and close this.

openshift-ci-robot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Sep 30, 2020

openshift-ci-robot requested review from mareklibra and vojtechszocs September 30, 2020 07:58

openshift-ci-robot added the component/kubevirt Related to kubevirt-plugin label Sep 30, 2020

openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 30, 2020

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 Sep 30, 2020

atiratree suggested changes Oct 1, 2020

View reviewed changes

irosenzw reviewed Oct 1, 2020

View reviewed changes

gouyang force-pushed the fix_wizard_nic_disk branch from 0db325b to afdf8ee Compare October 9, 2020 06:58

Bug 1883766: Adjust tests for UI changes

8c3d8c4

gouyang force-pushed the fix_wizard_nic_disk branch from afdf8ee to 8c3d8c4 Compare October 9, 2020 07:33

gouyang closed this Oct 13, 2020

gouyang deleted the fix_wizard_nic_disk branch February 7, 2021 02:45

Bug 1883766: Adjust tests for UI changes #6788

Bug 1883766: Adjust tests for UI changes #6788

Uh oh!

Conversation

gouyang commented Sep 30, 2020

Uh oh!

openshift-ci-robot commented Sep 30, 2020

Uh oh!

openshift-ci-robot commented Sep 30, 2020

Uh oh!

yaacov commented Sep 30, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gouyang Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gouyang Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gouyang Oct 2, 2020 •

edited

Loading

gouyang Oct 9, 2020 •

edited

Loading

irosenzw Oct 1, 2020 •

edited

Loading