Refactor devfile delete tests and add validity checks for delete command #4793

valaparthvi · 2021-06-10T10:51:48Z

What type of PR is this?
/kind cleanup
/kind code-refactoring

What does this PR do / why we need it:

Add validity checks for the delete command as mentioned in issue "odo delete -a" with "--app" & "--project" flags deletes the component under cwd/pwd #4451 (comment)
Add tests for validity checks
Add missing test cases for issue "odo delete -a" with "--app" & "--project" flags deletes the component under cwd/pwd #4451
Refactor tests with the new testbook approach

Which issue(s) this PR fixes:

Fixes #4451

PR acceptance criteria:

How to test changes / Special notes to the reviewer:

…ty checks

valaparthvi · 2021-06-11T06:57:12Z

/test v4.7-integration-e2e

Reason -


: Run multi-stage test integration-e2e - integration-e2e-ipi-deprovision-deprovision container test&nbsp;expand_less | 31m58s
-- | --
DependencyViolation: The dhcpOptions 'dopt-0544ee0ee0b2278c2' has dependencies and cannot be deleted. arn=arn:aws:ec2:us-west-2:460538899914:dhcp-options/dopt-0544ee0ee0b2278c2 level=warning msg=	status code: 400, request id: 4aef4723-323d-4ef0-9787-211c664125ec arn=arn:aws:ec2:us-west-2:460538899914:dhcp-options/dopt-0544ee0ee0b2278c2 level=info msg=Deleted id=sg-06b6d12af801070a9 level=info msg=Deleted id=Z0962381306F5FMSNLEIV record set=A api.ci-op-pwrd3446-89922.origin-ci-int-aws.dev.rhcloud.com. level=info msg=Deleted id=Z0962381306F5FMSNLEIV record set=A \052.apps.ci-op-pwrd3446-89922.origin-ci-int-aws.dev.rhcloud.com. level=info msg=Deleted id=Z0962381306F5FMSNLEIV level=info msg=Deleted id=igw-0e12f384fb74c2924 level=info msg=Released id=eipalloc-0092c7c3b3d1fb6d5 level=info msg=Deleted id=sg-012d00814ecedd613 level=info msg=Deleted NAT gateway=nat-023e177355a8f01d6 id=vpc-0ee31ee0c8abe6e24 level=info msg=Deleted NAT gateway=nat-0542c90c59e82b47b id=vpc-0ee31ee0c8abe6e24 level=info msg=Deleted id=vpc-0ee31ee0c8abe6e24 table=rtb-052c5b2b80f8cec93 level=info msg=Deleted id=vpc-0ee31ee0c8abe6e24 subnet=subnet-06f83c0f22c8e6682 level=info msg=Deleted id=vpc-0ee31ee0c8abe6e24 subnet=subnet-0e3ba88ab9fd0cd05 level=info msg=Deleted id=vpc-0ee31ee0c8abe6e24 level=info msg=Released id=eipalloc-0291e68eafe2e757e level=info msg=Deleted id=ci-op-pwrd3446-89922-cndzg-aext/572a695e5038c3bc level=info msg=Deleted id=dopt-0544ee0ee0b2278c2 level=info msg=Time elapsed: 31m59s {"component":"entrypoint","file":"prow/entrypoint/run.go:252","func":"k8s.io/test-infra/prow/entrypoint.gracefullyTerminate","level":"error","msg":"Process gracefully exited before 10m0s grace period","severity":"error","time":"2021-06-10T17:25:11Z"} {"component":"entrypoint","error":"process timed out","file":"prow/entrypoint/run.go:80","func":"k8s.io/test-infra/prow/entrypoint.Options.Run","level":"error","msg":"Error executing test process","severity":"error","time":"2021-06-10T17:25:11Z"} error: failed to execute wrapped command: exit status 127

tests/integration/devfile/cmd_devfile_delete_test.go

feloy · 2021-06-11T07:39:53Z

tests/integration/devfile/cmd_devfile_delete_test.go


-			commonVar.CliRunner.WaitAndCheckForExistence("deployments", commonVar.Project, 1)
+			Context("deleting a component from other component directory", func() {


Describe seems more appropriate here

Is it alright to nest Describe inside a Context block?

Yes, for me it's alright. Describes explains what we are testing in a high level, Context explains how we are testing it

https://onsi.github.io/ginkgo/#organizing-specs-with-containers-describe-and-context

feloy · 2021-06-11T07:41:48Z

tests/integration/devfile/cmd_devfile_delete_test.go

+							helper.CmdShouldPass("odo", "push", "--project", commonVar.Project)
+						})
+						JustAfterEach(func() {})
+						Context("delete with --context flag", func() {


Is this information/Context necessary?

I am not sure whether I understand your question correctly, but if what you mean to ask is whether we need a separate ginkgo.Context for this, then I would say yes, because in order to test this command with --context flag correctly, we either need to create /move to a new directory or move out of the component's directory.

In line 246, you are using this Context, because you are changing the current directory in JustBeforeEach, which is fine.

But here, there is no JustBeforeEach, so the context would not change.

Oh right right, it doesn't make much sense to have that Context. I'll remove it, thanks.

tests/integration/devfile/cmd_devfile_delete_test.go

feloy · 2021-06-11T12:23:52Z

/lgtm

valaparthvi · 2021-06-11T14:54:40Z

/retest

valaparthvi · 2021-06-14T06:22:12Z

/retest

valaparthvi · 2021-06-14T09:58:15Z

/test psi-unit-test-mac

girishramnani · 2021-06-14T10:09:34Z

/approve

openshift-ci · 2021-06-14T10:09:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: girishramnani

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:

~~OWNERS~~ [girishramnani]

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

openshift-bot · 2021-06-14T10:31:03Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-06-14T10:43:04Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-06-14T10:55:02Z

/retest

Please review the full test history for this PR and help us cut down flakes.

valaparthvi · 2021-06-14T11:22:44Z

/override ci/prow/psi-unit-test-mac
This job has been disabled atm.

openshift-ci · 2021-06-14T11:22:52Z

@valaparthvi: Overrode contexts on behalf of valaparthvi: ci/prow/psi-unit-test-mac

In response to this:

/override ci/prow/psi-unit-test-mac
This job has been disabled atm.

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 bot added kind/cleanup labels Jun 10, 2021

openshift-ci bot requested review from dharmit and mohammedzee1000 June 10, 2021 10:51

valaparthvi mentioned this pull request Jun 10, 2021

WIP: Refactor tests and add more to cmd_devfile_delete_test #4641

Closed

4 tasks

valaparthvi force-pushed the delete_tests branch from 8cb5283 to 921e5ed Compare June 10, 2021 10:54

valaparthvi mentioned this pull request Jun 10, 2021

"odo delete -a" with "--app" & "--project" flags deletes the component under cwd/pwd #4451

Closed

valaparthvi force-pushed the delete_tests branch from 921e5ed to f273413 Compare June 10, 2021 11:03

valaparthvi requested a review from feloy June 10, 2021 11:16

valaparthvi added 2 commits June 10, 2021 19:25

Refactor devfile delete integration tests, add more tests and invalid…

17156f6

…ty checks

Fix CI failures

01411bc

valaparthvi force-pushed the delete_tests branch from f273413 to 01411bc Compare June 10, 2021 13:55

feloy reviewed Jun 11, 2021

View reviewed changes

valaparthvi added 2 commits June 11, 2021 15:34

Address requests

92e2671

Remove unnecessary context

7c4340a

openshift-ci bot assigned feloy Jun 11, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jun 11, 2021

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Jun 14, 2021

valaparthvi mentioned this pull request Jun 14, 2021

Improve tests readability and spec clean up #4741

Closed

openshift-merge-robot merged commit c5728e0 into redhat-developer:main Jun 14, 2021

rm3l added the area/refactoring Issues or PRs related to code refactoring label Jun 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor devfile delete tests and add validity checks for delete command #4793

Refactor devfile delete tests and add validity checks for delete command #4793

valaparthvi commented Jun 10, 2021

valaparthvi commented Jun 11, 2021

feloy Jun 11, 2021

valaparthvi Jun 11, 2021

feloy Jun 11, 2021

feloy Jun 11, 2021

valaparthvi Jun 11, 2021

feloy Jun 11, 2021

valaparthvi Jun 11, 2021 •

edited

feloy commented Jun 11, 2021

valaparthvi commented Jun 11, 2021

valaparthvi commented Jun 14, 2021

valaparthvi commented Jun 14, 2021

girishramnani commented Jun 14, 2021

openshift-ci bot commented Jun 14, 2021

openshift-bot commented Jun 14, 2021

openshift-bot commented Jun 14, 2021

openshift-bot commented Jun 14, 2021

valaparthvi commented Jun 14, 2021

openshift-ci bot commented Jun 14, 2021


		commonVar.CliRunner.WaitAndCheckForExistence("deployments", commonVar.Project, 1)
		Context("deleting a component from other component directory", func() {

Refactor devfile delete tests and add validity checks for delete command #4793

Refactor devfile delete tests and add validity checks for delete command #4793

Conversation

valaparthvi commented Jun 10, 2021

valaparthvi commented Jun 11, 2021

feloy Jun 11, 2021

Choose a reason for hiding this comment

valaparthvi Jun 11, 2021

Choose a reason for hiding this comment

feloy Jun 11, 2021

Choose a reason for hiding this comment

feloy Jun 11, 2021

Choose a reason for hiding this comment

valaparthvi Jun 11, 2021

Choose a reason for hiding this comment

feloy Jun 11, 2021

Choose a reason for hiding this comment

valaparthvi Jun 11, 2021 • edited

Choose a reason for hiding this comment

feloy commented Jun 11, 2021

valaparthvi commented Jun 11, 2021

valaparthvi commented Jun 14, 2021

valaparthvi commented Jun 14, 2021

girishramnani commented Jun 14, 2021

openshift-ci bot commented Jun 14, 2021

openshift-bot commented Jun 14, 2021

openshift-bot commented Jun 14, 2021

openshift-bot commented Jun 14, 2021

valaparthvi commented Jun 14, 2021

openshift-ci bot commented Jun 14, 2021

valaparthvi Jun 11, 2021 •

edited