Speeds up integration test for deletion of owned resources #2599

mik-dass · 2020-02-13T07:05:11Z

What type of PR is this?

/kind code-refactoring

What does does this PR do / why we need it:

OC helper decalration is happening twice for the context, so removing it.

Which issue(s) this PR fixes:

Speeds up integration test for deletion of owned resources

How to test changes / Special notes to the reviewer:

odo component delete should clean owned resources integration test should pass.

amitkrout · 2020-02-13T07:27:10Z

tests/integration/component.go

@@ -816,18 +816,6 @@ func componentTests(args ...string) {
 	Context("odo component delete should clean owned resources", func() {
 		appName := "app"
 		cmpName := "nodejs"
-		var oc helper.OcRunner


oc is already defined globally. so +1 for removing this.

amitkrout · 2020-02-13T07:27:17Z

tests/integration/component.go

-
-		JustBeforeEach(func() {
-			project = helper.CreateRandProject()
-			originalDir = helper.Getwd()


+1 for removing it because i do not see any use of originalDir in the test spec

amitkrout · 2020-02-13T07:27:20Z

tests/integration/component.go

-
-		JustAfterEach(func() {
-			helper.DeleteProject(project)
-			helper.Chdir(originalDir)


amitkrout · 2020-02-13T07:30:53Z

tests/integration/component.go

-		var oc helper.OcRunner
-
-		JustBeforeEach(func() {
-			project = helper.CreateRandProject()


The project is being used locally to the spec and moreover there is no such project is created in BeforeEach(func(). So you can not remove it.

amitkrout · 2020-02-13T07:34:28Z

tests/integration/component.go

@@ -816,18 +816,6 @@ func componentTests(args ...string) {
 	Context("odo component delete should clean owned resources", func() {
 		appName := "app"


Can you please add a dynamic string instead of hardcode to avoid name collision in future. Something like

appName = helper.RandString(5)

amitkrout

@mik-dass Can you please replace

appName := "app"
		cmpName := "nodejs"
		var oc helper.OcRunner

		JustBeforeEach(func() {
			project = helper.CreateRandProject()
			originalDir = helper.Getwd()
			oc = helper.NewOcRunner("oc")
		})

		JustAfterEach(func() {
			helper.DeleteProject(project)
			helper.Chdir(originalDir)
		})

with

appName := "app"
		cmpName := "nodejs"

		JustBeforeEach(func() {
			project = helper.CreateRandProject()
		})

		JustAfterEach(func() {
			helper.DeleteProject(project)
		})

Signed-off-by: mik-dass <mrinald7@gmail.com>

codecov · 2020-03-20T07:12:22Z

Codecov Report

Merging #2599 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #2599      +/-   ##
=========================================
- Coverage   43.73%   43.7%   -0.04%     
=========================================
  Files          87      87              
  Lines        8043    8043              
=========================================
- Hits         3518    3515       -3     
- Misses       4176    4179       +3     
  Partials      349     349

Impacted Files	Coverage Δ
pkg/component/watch.go	`65.33% <0%> (-2%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f940c55...60040a2. Read the comment docs.

amitkrout · 2020-03-20T07:22:16Z

tests/integration/component.go

-		cmpName := "nodejs"
-		var oc helper.OcRunner
+		appName := helper.RandString(5)
+		cmpName := helper.RandString(5)


Cool!!!
Thanks for making it dynamic

amitkrout · 2020-03-20T07:25:56Z

Codecov Report

Merging #2599 into master will decrease coverage by 1.08%.
The diff coverage is n/a.
@@            Coverage Diff             @@
##           master    #2599      +/-   ##
==========================================
- Coverage   43.73%   42.65%   -1.09%     
==========================================
  Files          87       74      -13     
  Lines        8043     7089     -954     
==========================================
- Hits         3518     3024     -494     
+ Misses       4176     3773     -403     
+ Partials      349      292      -57
Impacted Files Coverage Δ
pkg/devfile/parser/fs.go 0% <0%> (-100%) ⬇️
pkg/kclient/kclient.go 0% <0%> (-26.93%) ⬇️
pkg/storage/storage.go 44.44% <0%> (-5.03%) ⬇️
pkg/occlient/occlient.go 49.48% <0%> (-3.41%) ⬇️
pkg/preference/preference.go 66.35% <0%> (-2.62%) ⬇️
pkg/url/url.go 44.69% <0%> (-1.52%) ⬇️
pkg/devfile/parser/content.go 82.6% <0%> (-0.73%) ⬇️
pkg/component/component.go 24.05% <0%> (-0.25%) ⬇️
pkg/devfile/parse.go 0% <0%> (ø) ⬆️
pkg/testingutil/fileutils.go 0% <0%> (ø) ⬆️
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f940c55...88a86e7. Read the comment docs.

ATM don't believe this report. Will fix this in #2575

amitkrout · 2020-03-20T08:42:01Z

/lgtm

kadel · 2020-03-20T09:14:17Z

/approve

openshift-ci-robot · 2020-03-20T09:14:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kadel

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:

~~tests/OWNERS~~ [kadel]

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 added kind/code-refactoring labels Feb 13, 2020

openshift-ci-robot requested review from amitkrout and prietyc123 February 13, 2020 07:06

mik-dass force-pushed the fix_own_test branch from e2ea864 to af23b37 Compare February 13, 2020 07:06

amitkrout reviewed Feb 13, 2020

View reviewed changes

amitkrout suggested changes Feb 13, 2020

View reviewed changes

openshift-ci-robot assigned amitkrout Feb 13, 2020

amitkrout suggested changes Feb 13, 2020

View reviewed changes

mik-dass closed this Feb 13, 2020

mik-dass deleted the fix_own_test branch February 13, 2020 08:36

mik-dass restored the fix_own_test branch February 13, 2020 10:10

mik-dass reopened this Feb 13, 2020

mik-dass changed the title ~~Speeds up integration test for deletion of owned resources~~ [WIP] Speeds up integration test for deletion of owned resources Feb 13, 2020

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Feb 13, 2020

amitkrout suggested changes Mar 20, 2020

View reviewed changes

Speeds up integration test for deletion of owned resources

aa87902

Signed-off-by: mik-dass <mrinald7@gmail.com>

mik-dass force-pushed the fix_own_test branch from af23b37 to 93d1617 Compare March 20, 2020 07:12

mik-dass force-pushed the fix_own_test branch from 93d1617 to 88a86e7 Compare March 20, 2020 07:13

mik-dass changed the title ~~[WIP] Speeds up integration test for deletion of owned resources~~ Speeds up integration test for deletion of owned resources Mar 20, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Mar 20, 2020

amitkrout reviewed Mar 20, 2020

View reviewed changes

Fixes CheckForExistence

60040a2

mik-dass force-pushed the fix_own_test branch from 88a86e7 to 60040a2 Compare March 20, 2020 07:38

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Mar 20, 2020

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

openshift-merge-robot merged commit a2cdfe5 into redhat-developer:master Mar 20, 2020

mik-dass deleted the fix_own_test branch April 13, 2020 07:32

rm3l added estimated-size/S (5-10) Rough sizing for Epics. Less then one sprint of work for one person area/refactoring Issues or PRs related to code refactoring labels Jun 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speeds up integration test for deletion of owned resources #2599

Speeds up integration test for deletion of owned resources #2599

mik-dass commented Feb 13, 2020 •

edited

amitkrout Feb 13, 2020

amitkrout Feb 13, 2020

amitkrout Feb 13, 2020

amitkrout Feb 13, 2020

amitkrout Feb 13, 2020

amitkrout left a comment

codecov bot commented Mar 20, 2020 •

edited

amitkrout Mar 20, 2020

amitkrout commented Mar 20, 2020

Codecov Report

amitkrout commented Mar 20, 2020

kadel commented Mar 20, 2020

openshift-ci-robot commented Mar 20, 2020

		@@ -816,18 +816,6 @@ func componentTests(args ...string) {
		Context("odo component delete should clean owned resources", func() {
		appName := "app"

Speeds up integration test for deletion of owned resources #2599

Speeds up integration test for deletion of owned resources #2599

Conversation

mik-dass commented Feb 13, 2020 • edited

amitkrout Feb 13, 2020

Choose a reason for hiding this comment

amitkrout Feb 13, 2020

Choose a reason for hiding this comment

amitkrout Feb 13, 2020

Choose a reason for hiding this comment

amitkrout Feb 13, 2020

Choose a reason for hiding this comment

amitkrout Feb 13, 2020

Choose a reason for hiding this comment

amitkrout left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 20, 2020 • edited

Codecov Report

amitkrout Mar 20, 2020

Choose a reason for hiding this comment

amitkrout commented Mar 20, 2020

Codecov Report

amitkrout commented Mar 20, 2020

kadel commented Mar 20, 2020

openshift-ci-robot commented Mar 20, 2020

mik-dass commented Feb 13, 2020 •

edited

codecov bot commented Mar 20, 2020 •

edited