Bug 1861957: Overhaul kubevirt plugin tests #6152

gouyang · 2020-07-30T02:21:41Z

Overhaul failure tests, verified here: https://cnv-qe-jenkins.rhev-ci-vms.eng.rdu2.redhat.com/job/test-kubevirt-console-4.5-cnv-2.4-prod-gouyang/5/testReport/.

Note:
The verify job is running against http://0.0.0.0:9000/ which has login disabled, so non-admin test is failed.
If run non-admin test against https://console-openshift-console.apps.gouyang0727.cnv-qe.rhcloud.com/, it's PASS.

openshift-ci-robot · 2020-07-30T02:22:03Z

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.

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.

gouyang · 2020-07-31T11:24:27Z

will rebase on #6132.

atiratree · 2020-08-03T14:22:06Z

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

@@ -46,6 +48,8 @@ export function removeLeakableResource(leakedResources: Set<string>, resource) {
 }

 export function createResource(resource) {
+  // ensure not return error even the source is existed by deleting it before adding it


where is this necessary?

IMO we should have a specific method for this like recreate so the use case would be apparent by looking at the name.

We should not use this in places where it isn't necessary, so it would not hide bugs in the future

I've seen some tests failed because of existed resource when I run tests locally, I was think it would be safe to make sure they're not returning error as they're used in setup/teardown.

I don't want to introduce recreate as they're not on the same purpose. I can revert this.

they are not, but we could have both createResource and reCreateResource

ok, let's revert if you are not seeing this anymore. If you think it is necessary in some case, I would vote for having already mentioned functions and using it just in place where it is needed.

What about add a check via kubectl get, if the resource is existing, do nothing. Then it doesn't throw error.

could we use kubectl apply for this instead?
again, can we still make a 2nd method for that and use applyResource in places where this is needed?

Are you meaning to use kubectl create for createResource and use kubectl apply for applyResource?
I changed it to kubectl apply in createResource.

yes, IMO it would be nicer to have both createResource and applyResource and use them accordingly

I added applyResource, I will use it accordingly in future based on test experience.

atiratree · 2020-08-03T14:23:04Z

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

@@ -56,7 +60,7 @@ export function createResources(resources) {
 export function deleteResource(resource) {
  const kind = resource.kind === 'NetworkAttachmentDefinition' ? 'net-attach-def' : resource.kind;
  execSync(
-    `kubectl delete -n ${resource.metadata.namespace} --cascade ${kind} ${resource.metadata.name}`,
+    `kubectl delete --ignore-not-found=true -n ${resource.metadata.namespace} --cascade ${kind} ${resource.metadata.name}`,


the same. Where is this used?

we could also pass an additional argument for this if needed

If the resource is not created successfully, the test is failed earlier and will have an error in tear down step in deleteResource. This is trying to make it not throwing error, as it seems always been used in tear down step.

ok, can we at least add the additional optional argument ignoreNotFound ?

Added additional optional argument ignoreNotFound with default value true because it's used in tear down, in case it's called somewhere else, it can specify it to false.

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

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

frontend/packages/kubevirt-plugin/integration-tests/tests/vm.tab.console.rdp.scenario.ts

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

atiratree · 2020-08-04T08:43:34Z

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

@@ -46,6 +48,8 @@ export function removeLeakableResource(leakedResources: Set<string>, resource) {
 }

 export function createResource(resource) {
+  // ensure not return error even the source is existed by deleting it before adding it


they are not, but we could have both createResource and reCreateResource

ok, let's revert if you are not seeing this anymore. If you think it is necessary in some case, I would vote for having already mentioned functions and using it just in place where it is needed.

frontend/packages/kubevirt-plugin/integration-tests/tests/vm.tab.console.rdp.scenario.ts

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

frontend/packages/kubevirt-plugin/integration-tests/tests/vm.tab.console.rdp.scenario.ts

atiratree · 2020-08-07T11:56:10Z

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

-  );
+  if (ignoreNotFound) {
+    execSync(
+      `kubectl delete --ignore-not-found=true -n ${resource.metadata.namespace} --cascade ${kind} ${resource.metadata.name}`,


Suggested change

`kubectl delete --ignore-not-found=true -n ${resource.metadata.namespace} --cascade ${kind} ${resource.metadata.name}`,

`kubectl delete --ignore-not-found=${ignoreNotFound} -n ${resource.metadata.namespace} --cascade ${kind} ${resource.metadata.name}`,

It seems better to don't add if esle in this way, changed to use --ignore-not-found=${ignoreNotFound} and not use if else.

atiratree · 2020-08-10T10:46:32Z

/approve

atiratree · 2020-08-10T10:48:18Z

/ok-to-test

atiratree · 2020-08-10T13:08:11Z

@rawagner could you take a look?

openshift-ci-robot · 2020-08-17T08:27:27Z

@gouyang: An error was encountered adding this pull request to the external tracker bugs for bug 1861957 on the Bugzilla server at https://bugzilla.redhat.com:

JSONRPC error 32000: There was an error reported for a GitHub REST call. URL: https://api.github.com/repos/openshift/console/pulls/6152 Error: 403 Forbidden at /loader/0x559669dd8440/Bugzilla/Extension/ExternalBugs/Type/GitHub.pm line 111. at /loader/0x559669dd8440/Bugzilla/Extension/ExternalBugs/Type/GitHub.pm line 111. eval {...} called at /loader/0x559669dd8440/Bugzilla/Extension/ExternalBugs/Type/GitHub.pm line 98 Bugzilla::Extension::ExternalBugs::Type::GitHub::_do_rest_call('Bugzilla::Extension::ExternalBugs::Type::GitHub=HASH(0x559673...', 'https://api.github.com/repos/openshift/console/pulls/6152', 'GET') called at /loader/0x559669dd8440/Bugzilla/Extension/ExternalBugs/Type/GitHub.pm line 62 Bugzilla::Extension::ExternalBugs::Type::GitHub::get_data('Bugzilla::Extension::ExternalBugs::Type::GitHub=HASH(0x559673...', 'Bugzilla::Extension::ExternalBugs::Bug=HASH(0x559673028880)') called at /loader/0x559669dd8440/Bugzilla/Extension/ExternalBugs/Bug.pm line 302 eval {...} called at /loader/0x559669dd8440/Bugzilla/Extension/ExternalBugs/Bug.pm line 302 Bugzilla::Extension::ExternalBugs::Bug::update_ext_info('Bugzilla::Extension::ExternalBugs::Bug=HASH(0x559673028880)', 1) called at /loader/0x559669dd8440/Bugzilla/Extension/ExternalBugs/Bug.pm line 125 Bugzilla::Extension::ExternalBugs::Bug::create('Bugzilla::Extension::ExternalBugs::Bug', 'HASH(0x559672f8ce70)') called at /var/www/html/bugzilla/extensions/ExternalBugs/Extension.pm line 877 Bugzilla::Extension::ExternalBugs::bug_start_of_update('Bugzilla::Extension::ExternalBugs=HASH(0x5596734221c0)', 'HASH(0x559672a636d0)') called at /var/www/html/bugzilla/Bugzilla/Hook.pm line 21 Bugzilla::Hook::process('bug_start_of_update', 'HASH(0x559672a636d0)') called at /var/www/html/bugzilla/Bugzilla/Bug.pm line 1170 Bugzilla::Bug::update('Bugzilla::Bug=HASH(0x55967320e1d0)') called at /loader/0x559669dd8440/Bugzilla/Extension/ExternalBugs/WebService.pm line 88 Bugzilla::Extension::ExternalBugs::WebService::add_external_bug('Bugzilla::WebService::Server::JSONRPC::Bugzilla::Extension::E...', 'HASH(0x5596727ee460)') called at (eval 2526) line 1 eval ' $procedure->{code}->($self, @params) ;' called at /usr/share/perl5/vendor_perl/JSON/RPC/Legacy/Server.pm line 220 JSON::RPC::Legacy::Server::_handle('Bugzilla::WebService::Server::JSONRPC::Bugzilla::Extension::E...', 'HASH(0x559673598d00)') called at /var/www/html/bugzilla/Bugzilla/WebService/Server/JSONRPC.pm line 295 Bugzilla::WebService::Server::JSONRPC::_handle('Bugzilla::WebService::Server::JSONRPC::Bugzilla::Extension::E...', 'HASH(0x559673598d00)') called at /usr/share/perl5/vendor_perl/JSON/RPC/Legacy/Server.pm line 126 JSON::RPC::Legacy::Server::handle('Bugzilla::WebService::Server::JSONRPC::Bugzilla::Extension::E...') called at /var/www/html/bugzilla/Bugzilla/WebService/Server/JSONRPC.pm line 70 Bugzilla::WebService::Server::JSONRPC::handle('Bugzilla::WebService::Server::JSONRPC::Bugzilla::Extension::E...') called at /var/www/html/bugzilla/jsonrpc.cgi line 31 ModPerl::ROOT::Bugzilla::ModPerl::ResponseHandler::var_www_html_bugzilla_jsonrpc_2ecgi::handler('Apache2::RequestRec=SCALAR(0x5596726b9e38)') called at /usr/lib64/perl5/vendor_perl/ModPerl/RegistryCooker.pm line 207 eval {...} called at /usr/lib64/perl5/vendor_perl/ModPerl/RegistryCooker.pm line 207 ModPerl::RegistryCooker::run('Bugzilla::ModPerl::ResponseHandler=HASH(0x559672dd9718)') called at /usr/lib64/perl5/vendor_perl/ModPerl/RegistryCooker.pm line 173 ModPerl::RegistryCooker::default_handler('Bugzilla::ModPerl::ResponseHandler=HASH(0x559672dd9718)') called at /usr/lib64/perl5/vendor_perl/ModPerl/Registry.pm line 32 ModPerl::Registry::handler('Bugzilla::ModPerl::ResponseHandler', 'Apache2::RequestRec=SCALAR(0x5596726b9e38)') called at /var/www/html/bugzilla/mod_perl.pl line 139 Bugzilla::ModPerl::ResponseHandler::handler('Bugzilla::ModPerl::ResponseHandler', 'Apache2::RequestRec=SCALAR(0x5596726b9e38)') called at (eval 2526) line 0 eval {...} called at (eval 2526) line 0
Please contact an administrator to resolve this issue, then request a bug refresh with /bugzilla refresh.

In response to this:

Bug 1861957: Overhaul kubevirt plugin 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.

rawagner · 2020-08-17T14:58:23Z

/lgtm

openshift-ci-robot · 2020-08-17T14:58:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gouyang, rawagner, suomiy

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/console-shared/OWNERS~~ [rawagner]
~~frontend/packages/kubevirt-plugin/OWNERS~~ [rawagner,suomiy]

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-08-17T19:09:00Z

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

In response to this:

Bug 1861957: Overhaul kubevirt plugin 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 component/kubevirt Related to kubevirt-plugin label Jul 30, 2020

openshift-ci-robot requested review from dtaylor113 and vojtechszocs July 30, 2020 02:21

openshift-ci-robot added component/shared Related to console-shared needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 30, 2020

gouyang force-pushed the fix_bootorder branch from 7212156 to 1946191 Compare July 30, 2020 02:27

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

gouyang force-pushed the fix_bootorder branch from 1946191 to cab4a83 Compare July 31, 2020 01:44

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 31, 2020

atiratree suggested changes Aug 3, 2020

View reviewed changes

atiratree reviewed Aug 4, 2020

View reviewed changes

gouyang force-pushed the fix_bootorder branch 5 times, most recently from f223b49 to a162f5a Compare August 7, 2020 05:39

atiratree reviewed Aug 7, 2020

View reviewed changes

gouyang force-pushed the fix_bootorder branch 2 times, most recently from 04cee14 to da6dba6 Compare August 10, 2020 03:25

Bug 1861957: Overhaul kubevirt plugin tests

4560b49

gouyang force-pushed the fix_bootorder branch from da6dba6 to 4560b49 Compare August 10, 2020 05:41

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 Aug 10, 2020

gouyang changed the title ~~Overhaul kubevirt plugin tests~~ Bug 1861957: Overhaul kubevirt plugin tests Aug 17, 2020

openshift-ci-robot assigned rawagner Aug 17, 2020

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

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

openshift-merge-robot merged commit 0c53fd2 into openshift:master Aug 17, 2020

gouyang deleted the fix_bootorder branch August 18, 2020 02:23

spadgett added this to the v4.6 milestone Aug 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1861957: Overhaul kubevirt plugin tests #6152

Bug 1861957: Overhaul kubevirt plugin tests #6152

gouyang commented Jul 30, 2020

openshift-ci-robot commented Jul 30, 2020

gouyang commented Jul 31, 2020

atiratree Aug 3, 2020

gouyang Aug 4, 2020

atiratree Aug 4, 2020

gouyang Aug 5, 2020 •

edited

atiratree Aug 6, 2020

gouyang Aug 6, 2020

atiratree Aug 7, 2020

gouyang Aug 10, 2020

atiratree Aug 3, 2020

gouyang Aug 5, 2020

atiratree Aug 6, 2020

gouyang Aug 6, 2020

atiratree Aug 7, 2020

atiratree Aug 4, 2020

atiratree Aug 7, 2020

gouyang Aug 10, 2020

atiratree commented Aug 10, 2020

atiratree commented Aug 10, 2020

atiratree commented Aug 10, 2020

openshift-ci-robot commented Aug 17, 2020

rawagner commented Aug 17, 2020

openshift-ci-robot commented Aug 17, 2020

openshift-ci-robot commented Aug 17, 2020

	`kubectl delete --ignore-not-found=true -n ${resource.metadata.namespace} --cascade ${kind} ${resource.metadata.name}`,
	`kubectl delete --ignore-not-found=${ignoreNotFound} -n ${resource.metadata.namespace} --cascade ${kind} ${resource.metadata.name}`,

Bug 1861957: Overhaul kubevirt plugin tests #6152

Bug 1861957: Overhaul kubevirt plugin tests #6152

Conversation

gouyang commented Jul 30, 2020

openshift-ci-robot commented Jul 30, 2020

gouyang commented Jul 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gouyang Aug 5, 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

Choose a reason for hiding this comment

atiratree commented Aug 10, 2020

atiratree commented Aug 10, 2020

atiratree commented Aug 10, 2020

openshift-ci-robot commented Aug 17, 2020

rawagner commented Aug 17, 2020

openshift-ci-robot commented Aug 17, 2020

openshift-ci-robot commented Aug 17, 2020

gouyang Aug 5, 2020 •

edited