E2E test for v2 controller #399
Conversation
alculquicondor
force-pushed
the
e2e
branch
3 times, most recently
from
af8e12a
to
d4186b7
Compare
|
Ready for review |
v2/test/e2e/e2e_suite_test.go
Outdated
| ) | ||
|
|
||
| const ( | ||
| envTestUseExistingCluster = "TEST_USE_EXISTING_CLUSTER" |
There was a problem hiding this comment.
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.
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.
nit: dot import (. "github.com/onsi/gomega") will allow you shorten some code: "Expect(err).ToNot(HaveOccurred())"
There was a problem hiding this comment.
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.
If will be empty if namespace creation fails.
There was a problem hiding this comment.
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.
It does stop the test from executing, but AfterEach will still be called.
| } | ||
| }) | ||
|
|
||
| ginkgo.BeforeEach(func() { |
There was a problem hiding this comment.
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.
The command changes a little too. Moved to Context
|
/hold cancel |
|
[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.