Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CDI Upload integration test #6338

Closed
wants to merge 1 commit into from

Conversation

glekner
Copy link
Contributor

@glekner glekner commented Aug 17, 2020

No description provided.

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: glekner
To complete the pull request process, please assign mareklibra
You can assign the PR to them by writing /assign @mareklibra in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

@glekner
Copy link
Contributor Author

glekner commented Aug 18, 2020

/retest

@gouyang
Copy link
Contributor

gouyang commented Aug 19, 2020

} from '@console/ceph-storage-plugin/integration-tests/views/pvc.view';

describe('KubeVirt CDI Upload', () => {
const pvc = { name: 'upload-pvc', size: 1, unit: 'Gi', storageClassName: 'rook-ceph' };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rook-ceph is not on 4.5/4.6, we can use 'standard' for safety.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use the STORAGE_CLASS env variable?

Copy link
Contributor

@gouyang gouyang Aug 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

STORAGE_CLASS is defined in utils/constants/common.ts, can just use it.

await click(saveChangesBtn);
await browser.wait(until.textToBePresentInElement(cdiUploadView.uploadProgress, '100%'));
},
VM_CREATE_AND_EDIT_TIMEOUT_SECS,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should use a more proper timeout constants here, maybe CDI_UPLOAD_TIMEOUT_SECS.

it(
'ID() Upload data to CDI',
async () => {
const fileToUpload = '../views/cdiUploadView.ts';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should have a dedicated file for the upload

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm against making the console repo larger in size, if we're talking about creating a new txt file its fine by me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a simple text file is fine. We just don't want to depend on a random source file, which could change a location for example due to refactoring.

});
await cdiUploadView.uploadInput.sendKeys(absolutePath);
await click(saveChangesBtn);
await browser.wait(until.textToBePresentInElement(cdiUploadView.uploadProgress, '100%'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we start a VM/container and check the file/content is there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is outside our scope, the responsibility of this UI section is to upload a file, what you're talking about is the CDI responsibility.
We can however go to the PVCs page and check that the pvc is there and bounded

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

start a VM needs the upload file is a truly vm image, we can do it in tier2 test for this card: https://issues.redhat.com/browse/CNV-6020.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is not out of the scope, because we are doing final integration tests here. And we can't just depend on a simple status to check that everything went well.

ok we can move it to tier2 tests then @gouyang can you create a description for this test please?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added comment @gouyang

} from '@console/ceph-storage-plugin/integration-tests/views/pvc.view';

describe('KubeVirt CDI Upload', () => {
const pvc = { name: 'upload-pvc', size: 1, unit: 'Gi', storageClassName: 'rook-ceph' };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the unit is not used. Anyway we could be using a smaller size, since 1Gi is a bit of overkill for a small text file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need at least 100MB for the dv to initiate succesfully

await selectItemFromDropdown(pvc.storageClassName, storageclassDropdown);

// firefox needs the input to be shown
await browser.executeAsyncScript(function(callback) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you have some more info on this? The input is visible by default, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default is:
Screen Shot 2020-08-19 at 12 38 48

firefox needs the input to be shown like this: (just for tests)
Screen Shot 2020-08-19 at 12 38 59

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our tests is running against chrome only, it's can be removed if it's firefox specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but developers can run this test locally with firefox?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it has no side affect to run it with chrome, I'm okay with it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay

@glekner
Copy link
Contributor Author

glekner commented Aug 19, 2020

  • now selects the first available storage class
  • size changed to 100 MiB
  • added ID

@gouyang @suomiy


await inputPVCName.sendKeys(pvc.name);
await inputPVCSize.sendKeys(pvc.size);
await click(cdiUploadView.unitDropdown);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use selectDropdownOption here instead?

Copy link
Contributor Author

@glekner glekner Aug 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

selectDropdownOption needs a specific option.
since the classes are dynamic, its safer to just pick the first option.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems the same function is used by selectNetworkTypeByID which is testing the same underlying component Dropdown.
Is there some issue with using the same approach here?

@glekner
Copy link
Contributor Author

glekner commented Aug 19, 2020

@suomiy

  • added a mock text file for upload

@atiratree
Copy link
Member

@glekner highlighting #6338 (comment)

@openshift-ci-robot
Copy link
Contributor

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

Test name Commit Details Rerun command
ci/prow/e2e-gcp-console 70eefc5 link /test e2e-gcp-console
ci/prow/images 70eefc5 link /test images

Full PR test history. Your PR dashboard.

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
Copy link
Contributor

gouyang commented Sep 11, 2020

I passed the test locally with a few changes.

  1. await inputPVCName.sendKeys(pvc.name); this line does not work, it says element not interactable. I use fillInput from @console/shared/src/test-utils/utils.
  2. The first item of storage class is csi-manila-ceph in my cluster, it does not work. Can leave it and use the default value which comes from kubevirt-storage-class-defaults, or specify via STORAGE_CLASS.
  3. The text file is not supported format, I use execSync pulling down cirros image to /tmp and use it for test.
  4. The size should be 1Gib for cirros image.
  5. Use upload-pvc-${testName} for PVC name.

@gouyang
Copy link
Contributor

gouyang commented Sep 11, 2020

And it would be better to clean the created dv/pvc after the test.

@gouyang
Copy link
Contributor

gouyang commented Sep 24, 2020

I created a follow-up PR #6735 for this.

@glekner
Copy link
Contributor Author

glekner commented Sep 24, 2020

Thanks @gouyang . closing in favour of #6735

@glekner glekner closed this Sep 24, 2020
@glekner glekner deleted the test-cdi-upload branch September 24, 2020 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/kubevirt Related to kubevirt-plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants