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

Refactoring e2e tests #625

Merged
merged 1 commit into from
Sep 5, 2018

Conversation

ashetty1
Copy link
Contributor

@ashetty1 ashetty1 commented Aug 8, 2018

demo_test.go and e2e_test.go have a lot of duplicate tests. With this PR, I am moving all the component related tests will be moved to cmp_test.go, and have deleted demo_test.go.

Part of the refactoring effort at #576

@ashetty1 ashetty1 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 Aug 8, 2018
@kadel
Copy link
Member

kadel commented Aug 8, 2018

@ashetty1 Please add a link to the issue for every PR that you open. Also, some description would be nice ;-)

@ashetty1 ashetty1 force-pushed the update-tests-e2e branch 7 times, most recently from 818f66c to e5e53f9 Compare August 14, 2018 14:43
@ashetty1 ashetty1 changed the title [WIP] Refactoring e2e tests Refactoring e2e tests Aug 14, 2018
@@ -17,39 +17,43 @@ Run `make test-e2e` to execute the tests.
| `project create` | Y |
| `project get` | Y |
| `project delete` | Y |
| `project list` | N |
| `project list` | Y |
Copy link
Member

Choose a reason for hiding this comment

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

I guess that this should be N as it is commented out.

@ashetty1 ashetty1 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 Aug 16, 2018
@ashetty1 ashetty1 force-pushed the update-tests-e2e branch 6 times, most recently from 4df9bc0 to 498a7ef Compare August 20, 2018 09:53
@ashetty1
Copy link
Contributor Author

@surajnarwade @cdrage @anmolbabu @mik-dass @syamgk please review!

)

// SourceTest checks the component-source-type and the source url in the annotation of the bc and dc
// appTestName is the app of the app
Copy link
Member

Choose a reason for hiding this comment

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

name of the app?

// source is the source of the component i.e gitUrl or path to the directory or binary file
func SourceTest(appTestName string, sourceType string, source string) {
// checking for source-type in dc
getDc := runCmd("oc get dc wildfly-" + appTestName + " -o go-template='{{index .metadata.annotations \"app.kubernetes.io/component-source-type\"}}'")
Copy link
Contributor

Choose a reason for hiding this comment

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

can we extract out wildfly or the component type also to parameter in all the lines in this func?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@anmolbabu this is an old function. I just did some rearrangement of tests with this PR. Let me see if I can change that. I agree with you, it would be nice to do that.


// Failing: Issue #630
// It("should list the project", func() {
// listProj := runCmd("odo project list")
Copy link
Member

Choose a reason for hiding this comment

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

this test need not be commented
replace listProj := runCmd("odo project list") with listProj := runCmd("sleep 2s && odo project list")

refer comment on #630 (comment)

@@ -0,0 +1,370 @@
// +build !race
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also move the following:

Context("creating component without an application", func() {
It("should create the component in default application", func() {
runCmd("odo create php testcmp")
getCmp := runCmd("odo component get --short")
Expect(getCmp).To(Equal("testcmp"))
getApp := runCmd("odo app get --short")
Expect(getApp).To(Equal("app"))
})
It("should be able to delete the component", func() {
runCmd("odo delete testcmp -f")
getCmp := runCmd("odo list")
Expect(getCmp).NotTo(ContainSubstring("testcmp"))
})
})

odo/tests/e2e/e2e_test.go

Lines 120 to 189 in c4cab72

Describe("creating a component", func() {
Context("when application exists", func() {
It("should create a component", func() {
runCmd("git clone https://github.com/openshift/nodejs-ex " +
tmpDir + "/nodejs-ex")
// TODO: add tests for --git
runCmd("odo create nodejs --local " + tmpDir + "/nodejs-ex")
runCmd("odo push")
})
It("should be the get the component created as active component", func() {
cmp := runCmd("odo component get --short")
Expect(cmp).To(Equal("nodejs"))
})
It("should create the component within the application", func() {
getApp := runCmd("odo app get --short")
Expect(getApp).To(Equal(appTestName))
})
It("should list the components within the application", func() {
cmpList := runCmd("odo list")
Expect(cmpList).To(ContainSubstring("nodejs"))
})
It("should be able to create multiple components within the same application", func() {
runCmd("odo create php")
})
It("should list the newly created second component", func() {
cmpList := runCmd("odo list")
Expect(cmpList).To(ContainSubstring("php"))
})
It("should get the application "+appTestName, func() {
appGet := runCmd("odo app get --short")
Expect(appGet).To(Equal(appTestName))
})
It("should be able to set a component as active", func() {
cmpSet := runCmd("odo component set nodejs")
Expect(cmpSet).To(ContainSubstring("nodejs"))
})
It("should be able to retrieve logs", func() {
runCmd("odo log")
runCmd("odo log nodejs")
})
It("should be able to create git component with required ports", func() {
runCmd("odo create nodejs nodejs-git --git https://github.com/openshift/nodejs-ex --port 8080/tcp,9100/udp")
// checking port names
portsNames := runCmd("oc get services nodejs-git-" + appTestName + " -o go-template='{{range .spec.ports}}{{.name}}{{end}}'")
Expect(portsNames).To(ContainSubstring("8080-tcp"))
Expect(portsNames).To(ContainSubstring("9100-udp"))
// checking port numbers
ports := runCmd("oc get services nodejs-git-" + appTestName + " -o go-template='{{range .spec.ports}}{{.port}}{{end}}'")
Expect(ports).To(ContainSubstring("8080"))
Expect(ports).To(ContainSubstring("9100"))
// checking protocols
protocols := runCmd("oc get services nodejs-git-" + appTestName + " -o go-template='{{range .spec.ports}}{{.protocol}}{{end}}'")
Expect(protocols).To(ContainSubstring("TCP"))
Expect(protocols).To(ContainSubstring("UDP"))
})
})
})

also here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@anmolbabu nope. We need a few basic component tests in e2e_test.go too.

@ashetty1 ashetty1 force-pushed the update-tests-e2e branch 2 times, most recently from 521a00b to 3c5405b Compare August 30, 2018 11:12
@ashetty1
Copy link
Contributor Author

ashetty1 commented Sep 4, 2018

@surajnarwade @anmolbabu @mik-dass guys one final review and merge?

@cdrage
Copy link
Member

cdrage commented Sep 4, 2018

This LGTM!

@surajnarwade surajnarwade added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Sep 5, 2018
demo_test.go and e2e_test.go have a lot of duplicate tests. With this PR, I am moving all the component related tests to cmp_test.go, and am removing demo_test.go.

We will continue to add more component tests to cmp_test.go
@anmolbabu
Copy link
Contributor

@surajnarwade @syamgk @mik-dass Can you guys review this PR.
I think we can push it in if we manage to gain may be 1 more approval(It has 2 approvals already)

@ashetty1 ashetty1 removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Sep 5, 2018
Copy link
Contributor

@surajnarwade surajnarwade left a comment

Choose a reason for hiding this comment

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

LGTM 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants