Add integration tests for devfile storage and exec #2838

maysunfaisal · 2020-04-07T17:56:34Z

Signed-off-by: Maysun J Faisal maysun.j.faisal@ibm.com

What type of PR is this?
/kind feature

What does does this PR do / why we need it:
This PR adds integration tests for

devfile exec
devfile storage

since the original feature PRs for these user stories did not cover integration tests and had only the unit tests.

Which issue(s) this PR fixes:

Fixes #2818

How to test changes / Special notes to the reviewer:
make test-cmd-devfile-push
make test-cmd-devfile-url

maysunfaisal · 2020-04-08T00:38:38Z

/retest

maysunfaisal · 2020-04-08T06:38:19Z

/retest

codecov · 2020-04-08T08:57:57Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #2838      +/-   ##
==========================================
- Coverage   43.06%   42.97%   -0.09%     
==========================================
  Files          97       97              
  Lines        8917     8917              
==========================================
- Hits         3840     3832       -8     
- Misses       4713     4718       +5     
- Partials      364      367       +3

Impacted Files	Coverage Δ
pkg/watch/watch.go	`64.70% <0.00%> (-4.71%)`	⬇️

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 11e04b6...8cddc4f. Read the comment docs.

johnmcollier

Overall tests look good. Mostly minor comments and one question around testing that the app is running.

johnmcollier · 2020-04-08T14:27:32Z

tests/examples/source/devfiles/nodejs/app/app.js

+ * available under the terms of the Eclipse Public License 2.0
+ * which is available at https://www.eclipse.org/legal/epl-2.0/
+ *
+ * SPDX-License-Identifier: EPL-2.0


Does the license for this need to be apache2 to match the rest of odo?

Removed, git cloning it instead

johnmcollier · 2020-04-08T14:31:28Z

tests/examples/source/devfiles/nodejs/README.md

@@ -0,0 +1,18 @@
+Template from https://github.com/che-samples/web-nodejs-sample


Maybe rather than storing this in the odo repo we just do a git clone in our tests? I think some of the other odo integration tests clone repos

git cloning it right now

johnmcollier · 2020-04-08T14:43:11Z

tests/integration/devfile/cmd_devfile_push_test.go

+		helper.CmdShouldPass("odo", "preference", "set", "Experimental", "true")
+
+		cmpName = helper.RandString(6)
+		helper.CmdShouldPass("odo", "create", "nodejs", "--project", namespace, cmpName)


I have some concerns about always creating a nodejs component for all of the tests, and baking it into the test suite setup. Could we have some integration tests that do test devfiles from https://github.com/elsony/devfile-registry? (Such as the maven or springboot ones)

I will put some springboot tests here and there since it has two component containers.

johnmcollier · 2020-04-08T14:47:11Z

tests/integration/devfile/cmd_devfile_push_test.go

 			helper.CmdShouldPass("odo", "push", "--devfile", "devfile.yaml", "--namespace", namespace)

 			// use the force build flag and push
 			output := helper.CmdShouldPass("odo", "push", "--devfile", "devfile.yaml", "--namespace", namespace, "-f")
 			Expect(output).To(Not(ContainSubstring("No file changes detected, skipping build")))
 		})

+		It("should execute the default devbuild and devrun commands if present", func() {


Can some of these test cases test that the built/running application is actually running?

We might need to ask the odo folks (tag @amitkrout) if it's possible to get the ingress domain and pass it into odo url create, if not we can do one of three things:

Wait for odo url create to support creating routes for devfile components on OpenShift (so we don't have to pass in the ingress domain)

Exec into the runtime container and do a curl/wget there

Handle later as part of the end-to-end tests

Thinking aloud, how do we configure ingress host between running on CI and running on ppl's machines?

@maysunfaisal It looks like #2833 is the issue we wanna keep an eye on. So maybe best to wait?

yeah url endpoint testing can be done with that issue!

johnmcollier · 2020-04-08T14:49:34Z

tests/integration/devfile/cmd_devfile_url_test.go

@@ -41,22 +45,21 @@ var _ = Describe("odo devfile url command tests", func() {
 	Context("Listing urls", func() {
 		It("should list url after push", func() {
 			var stdout string
-			componentName := helper.RandString(6)
 			url1 := helper.RandString(5)
 			host := helper.RandString(5) + ".com"


Not an issue with your tests, but I just realized we're using a random URL for the ingress host, which would cause issues if we wanted to test that the URL actually exposed the app

Nothing for you to do, but something we need to keep in mind.

yep, we need to provide a correct host when we're testing the ingress domain

johnmcollier · 2020-04-08T14:52:43Z

tests/integration/devfile/cmd_devfile_push_test.go

+
+			output := helper.CmdShouldFail("odo", "push", "--devfile", "devfile.yaml", "--namespace", namespace)
+			Expect(output).NotTo(ContainSubstring("Executing devrun command"))
+			Expect(output).To(ContainSubstring("Failed to start component with name"))


You can probably remove some of the output checks in the test cases you added, such as the middle one here.

I.e. it's probably sufficient to just check that Executing devrun command doesn't exist and that The command \"devrun\" was not found in the devfile was found.

amitkrout · 2020-04-09T12:04:32Z

@maysunfaisal rebase please.

maysunfaisal · 2020-04-09T23:53:54Z

@johnmcollier @amitkrout branch has been rebased and updated with feedback

johnmcollier

Changes look good and we're greatly increasing our test coverage, nice work. We'll use #2833 to cover testing the running app over ingress.

/lgtm

amitkrout · 2020-04-11T05:32:36Z

now flag flake - #2642
/test v4.2-integration-e2e-benchmark

maysunfaisal · 2020-04-12T23:49:21Z

@johnmcollier FYI I had to add another commit to fix another test that I missed, the travis and CI tests have passed now.

johnmcollier

/lgtm

Adding the label back as the tests were updated

kadel · 2020-04-14T13:32:01Z

/approve

openshift-ci-robot · 2020-04-14T13:32:27Z

[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-bot · 2020-04-14T18:10:53Z

/retest

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

maysunfaisal · 2020-04-14T21:44:48Z

wow merge conflict, lgtm will have to be added again :/

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

johnmcollier

Adding back the label

/lgtm

openshift-bot · 2020-04-15T02:25:26Z

/retest

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

openshift-bot · 2020-04-15T02:50:46Z

/retest

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

openshift-bot · 2020-04-15T03:42:44Z

/retest

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

openshift-bot · 2020-04-15T08:15:14Z

/retest

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

openshift-bot · 2020-04-15T09:33:15Z

/retest

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

openshift-ci-robot added the kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation label Apr 7, 2020

openshift-ci-robot requested review from amitkrout and mohammedzee1000 April 7, 2020 17:56

maysunfaisal changed the title ~~Add integration tests for devfile storage and exec~~ {WIP} Add integration tests for devfile storage and exec Apr 7, 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 Apr 7, 2020

maysunfaisal changed the title ~~{WIP} Add integration tests for devfile storage and exec~~ Add integration tests for devfile storage and exec Apr 7, 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 Apr 7, 2020

johnmcollier suggested changes Apr 8, 2020

View reviewed changes

openshift-ci-robot assigned johnmcollier Apr 8, 2020

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Apr 9, 2020

maysunfaisal force-pushed the 2818-1 branch from 08bd146 to 15eebf8 Compare April 9, 2020 23:52

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Apr 9, 2020

johnmcollier approved these changes Apr 10, 2020

View reviewed changes

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

maysunfaisal force-pushed the 2818-1 branch from 15eebf8 to 4657cdf Compare April 10, 2020 15:42

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 10, 2020

johnmcollier approved these changes Apr 13, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 13, 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 Apr 14, 2020

maysunfaisal added 5 commits April 14, 2020 19:43

Add integration tests for devfile storage and exec

8e30fb6

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

Update devfile integration tests for port

0bbd8a8

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

Devfile integration tests, clone project

9768abd

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

Update devfile integration tests to have springboot

78574c7

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

update devfile delete integration test

8cddc4f

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

maysunfaisal force-pushed the 2818-1 branch from 4657cdf to 8cddc4f Compare April 15, 2020 00:00

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 15, 2020

johnmcollier approved these changes Apr 15, 2020

View reviewed changes

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

openshift-merge-robot merged commit 0da6ff4 into redhat-developer:master Apr 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add integration tests for devfile storage and exec #2838

Add integration tests for devfile storage and exec #2838

maysunfaisal commented Apr 7, 2020 •

edited

maysunfaisal commented Apr 8, 2020

maysunfaisal commented Apr 8, 2020

codecov bot commented Apr 8, 2020 •

edited

johnmcollier left a comment

johnmcollier Apr 8, 2020

maysunfaisal Apr 8, 2020

johnmcollier Apr 8, 2020

maysunfaisal Apr 8, 2020

johnmcollier Apr 8, 2020

maysunfaisal Apr 9, 2020

johnmcollier Apr 8, 2020

maysunfaisal Apr 8, 2020

johnmcollier Apr 8, 2020

maysunfaisal Apr 9, 2020

johnmcollier Apr 8, 2020

maysunfaisal Apr 8, 2020

johnmcollier Apr 8, 2020

amitkrout commented Apr 9, 2020

maysunfaisal commented Apr 9, 2020

johnmcollier left a comment

amitkrout commented Apr 11, 2020

maysunfaisal commented Apr 12, 2020

johnmcollier left a comment

kadel commented Apr 14, 2020

openshift-ci-robot commented Apr 14, 2020

openshift-bot commented Apr 14, 2020

maysunfaisal commented Apr 14, 2020 •

edited

johnmcollier left a comment

openshift-bot commented Apr 15, 2020

openshift-bot commented Apr 15, 2020

openshift-bot commented Apr 15, 2020

openshift-bot commented Apr 15, 2020

openshift-bot commented Apr 15, 2020

		@@ -0,0 +1,18 @@
		Template from https://github.com/che-samples/web-nodejs-sample

Add integration tests for devfile storage and exec #2838

Add integration tests for devfile storage and exec #2838

Conversation

maysunfaisal commented Apr 7, 2020 • edited

maysunfaisal commented Apr 8, 2020

maysunfaisal commented Apr 8, 2020

codecov bot commented Apr 8, 2020 • edited

Codecov Report

johnmcollier left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amitkrout commented Apr 9, 2020

maysunfaisal commented Apr 9, 2020

johnmcollier left a comment

Choose a reason for hiding this comment

amitkrout commented Apr 11, 2020

maysunfaisal commented Apr 12, 2020

johnmcollier left a comment

Choose a reason for hiding this comment

kadel commented Apr 14, 2020

openshift-ci-robot commented Apr 14, 2020

openshift-bot commented Apr 14, 2020

maysunfaisal commented Apr 14, 2020 • edited

johnmcollier left a comment

Choose a reason for hiding this comment

openshift-bot commented Apr 15, 2020

openshift-bot commented Apr 15, 2020

openshift-bot commented Apr 15, 2020

openshift-bot commented Apr 15, 2020

openshift-bot commented Apr 15, 2020

maysunfaisal commented Apr 7, 2020 •

edited

codecov bot commented Apr 8, 2020 •

edited

maysunfaisal commented Apr 14, 2020 •

edited