-
Notifications
You must be signed in to change notification settings - Fork 216
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
E2E test for v2 controller #399
Conversation
af8e12a
to
d4186b7
Compare
Ready for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution! 🎉 👍
v2/test/e2e/e2e_suite_test.go
Outdated
) | ||
|
||
const ( | ||
envTestUseExistingCluster = "TEST_USE_EXISTING_CLUSTER" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Does this follow a convention for other tests? If not, I suggest to have something like "TEST_CREATE_CLUSTER" (negation of this).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to USE_EXISTING_CLUSTER
to match the variable that controller-runtime's testing framework uses
|
||
clientset "github.com/kubeflow/mpi-operator/v2/pkg/client/clientset/versioned" | ||
"github.com/onsi/ginkgo" | ||
"github.com/onsi/gomega" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: dot import (. "github.com/onsi/gomega") will allow you shorten some code: "Expect(err).ToNot(HaveOccurred())"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dot imports are generally considered an anti-pattern in go https://github.com/golang/go/wiki/CodeReviewComments#import-dot
gingko and gomega recommend it in their official documentation. But, as another point of reference, k8s doesn't do that. Linters also don't like it.
}) | ||
|
||
ginkgo.AfterEach(func() { | ||
if namespace != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can it be empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If will be empty if namespace creation fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, so "Expect(err).ToNot(HaveOccurred())" doesn't stop the test from executing?
Then I think it's worth to break the test execution when it clearly doesn't make sense anymore, for example when the MPI Job's creation fails. Even when namespace creation fails, continuation gives little hope for any useful information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does stop the test from executing, but AfterEach
will still be called.
} | ||
}) | ||
|
||
ginkgo.BeforeEach(func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's maybe more philosophical, but I think that all logic specific to openMPI should be encapsulated within "ginkgo.Context("with OpenMPI implementation", func() { ... }). Is the mpiJob's spec identical for intelMPI apart from the image? Would it work to have a function "func getPiMPIJob(image string)" and use it as "getPiMPIJob(openMPIImage)"? If not, maybe just move it to the OpenMPI context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The command changes a little too. Moved to Context
/hold cancel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/assign @terrytangyuan @zw0610
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. Thanks!
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kawych, terrytangyuan 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:
Approvers can indicate their approval by writing |
Part of #373. Related to #296
Add first E2E test for the v2 controller: Tests that an OpenMPI sample runs to completion as non-root user.
The tests run locally on a kind cluster.
The tests run as a new workflow job.
Notes to reviewer
Other tests combinations (IntelMPI, run-as-root) coming in follow up PRs.