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.

Sign up for GitHub

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

Jump to bottom

chore: add e2e test helper for container use spec #640

Merged
merged 15 commits into from
Jul 21, 2022
Merged

chore: add e2e test helper for container use spec #640

merged 15 commits into from
Jul 21, 2022

Conversation

aseaday
Copy link
Member

@aseaday aseaday commented Jul 20, 2022 •
edited
Loading

This PR is for a high level tests focuse on:

  • Users could enter into the container and execute the script or comands correctly
  • Robust tests such as old version compatibility
  • Provide some DSL for write e2e test easily

aseaday added 2 commits July 20, 2022 15:58
@aseaday
WIP: add remove image in docker client
0ef6a90 
Signed-off-by: Qi Chen <aseaday@hotmail.com>
@aseaday
WIP chore: Add e2e test
a4487e2 
Signed-off-by: Qi Chen <aseaday@hotmail.com>
@aseaday
Copy link
Member Author

aseaday commented Jul 20, 2022 •
edited
Loading

Write a e2e test code like:

package e2e

import (
	. "github.com/onsi/ginkgo/v2"
	. "github.com/onsi/gomega"
)

var _ = Describe("e2e quickstart", Ordered, func() {
	exampleName := "quick-start"
	BeforeAll(BuildImage(exampleName))
	BeforeEach(RunContainer(exampleName))
	It("execute python demo.py", func() {
		Expect(example(exampleName).Exec("python demo.py")).To(Equal("[2 3 4]"))
	})
	AfterEach(DestoryContainer(exampleName))
	AfterAll(RemoveImage(exampleName))
})

@aseaday aseaday requested a review from gaocegege July 20, 2022 09:59
gaocegege
gaocegege reviewed Jul 20, 2022
View reviewed changes
Copy link
Member

@gaocegege gaocegege left a comment

Choose a reason for hiding this comment

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

Could you please move the test cases in pkg/app into the e2e, then update the CI.yaml?

e2e/quick_start_test.go
)

var _ = Describe("e2e quickstart", Ordered, func() {
exampleName := "quick-start"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should place the example in testdata/.

Copy link
Member Author

Choose a reason for hiding this comment

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

LGTM

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we should place the example in testdata/.

I just want to test all examples

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I could move the extra envd-quick-start to testdata. And other tests shoule be wrote to cover the examples

Copy link
Member

Choose a reason for hiding this comment

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

examples may be complex. Personally, I prefer to test different features in different test cases. WDYT

Copy link
Member Author

Choose a reason for hiding this comment

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

LGTM

@gaocegege
Copy link
Member

Thanks for the contributions! I like the DSL!

aseaday added 5 commits July 20, 2022 18:12
@aseaday
WIP: move the e2e tests
f940ec7 
Signed-off-by: Qi Chen <aseaday@hotmail.com>
@aseaday
WIP: move the quick start
ac6e351 
Signed-off-by: Qi Chen <aseaday@hotmail.com>
@aseaday
WIP: fix lint
0e7e480 
Signed-off-by: Qi Chen <aseaday@hotmail.com>
@aseaday
WIP: Merge branch 'main' into chore/add_e2e_test
2a28b73
@aseaday
WIP: fix e2e test script
de55149 
Signed-off-by: Qi Chen <aseaday@hotmail.com>
@aseaday aseaday changed the title WIP: chore/add e2e test chore/add e2e test helper for container use spec Jul 20, 2022
@aseaday aseaday changed the title chore/add e2e test helper for container use spec chore: add e2e test helper for container use spec Jul 20, 2022
@aseaday aseaday requested a review from gaocegege July 20, 2022 10:51
gaocegege
gaocegege reviewed Jul 20, 2022
View reviewed changes
Makefile Outdated
@@ -155,6 +155,7 @@ test: generate ## Run the tests

e2e-test: generate
@go test -race -coverpkg=./pkg/app -coverprofile=e2e-coverage.out ./pkg/app
Copy link
Member

Choose a reason for hiding this comment

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

Can we move the test suite to e2e/ directory directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

I move these files to the app/, But I think it is ok for now but If we have more and more spec test may not be related those commands directly. we still need a independent location to put those specs.

Copy link
Member

Choose a reason for hiding this comment

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

I mean move the tests in pkg/app/ to e2e/

Then all the e2e test cases are placed in the same dir.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, LOL, I am a little stupid today...

@aseaday
WIP: spec location adjust
a27bff0 
Signed-off-by: Qi Chen <aseaday@hotmail.com>
@aseaday aseaday requested a review from gaocegege July 20, 2022 13:39
@aseaday
WIP: spec location adjust
554bf54 
Signed-off-by: Qi Chen <aseaday@hotmail.com>
gaocegege
gaocegege reviewed Jul 20, 2022
View reviewed changes
pkg/app/e2e/build_test.go Outdated
@@ -12,14 +12,15 @@
// See the License for the specific language governing permissions and
// limitations under the License.

package app
package e2e
Copy link
Member

Choose a reason for hiding this comment

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

I was expecting to move pkg/app to e2e/, actually. Or maybe test/e2e.

Copy link
Member Author

Choose a reason for hiding this comment

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

e2e seems better after I look throught some projects.

aseaday added 6 commits July 20, 2022 22:52
@aseaday
WIP: spec location adjust
7847070 
Signed-off-by: Qi Chen <aseaday@hotmail.com>
@aseaday
WIP: fix ci
a3310b2 
Signed-off-by: Qi Chen <aseaday@hotmail.com>
@aseaday
WIP: fix ci
3b7c3cc 
Signed-off-by: Qi Chen <aseaday@hotmail.com>
@aseaday
WIP: fix ci
5b3488f 
Signed-off-by: Qi Chen <aseaday@hotmail.com>
@aseaday
WIP: fix ci
e68464b 
Signed-off-by: Qi Chen <aseaday@hotmail.com>
@aseaday
WIP: fix ci
37bad88 
Signed-off-by: Qi Chen <aseaday@hotmail.com>
@aseaday
Copy link
Member Author

aseaday commented Jul 20, 2022

I don't know why e2e test falled and I test it local

@gaocegege
Copy link
Member

Sometimes it's flakey. I triggered a new build.

@gaocegege gaocegege merged commit 566a235 into tensorchord:main Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gaocegege gaocegege gaocegege approved these changes

Assignees
No one assigned
Labels
None yet
Projects
No open projects
envd sprint board
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aseaday @gaocegege