-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
🏃 Add common utilities for provider-based testing #1474
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: detiber 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 |
/assign @chuckha ptal |
@detiber: GitHub didn't allow me to assign the following users: ptal. Note that only kubernetes-sigs members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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 looks fine to me, were you looking for any particular feedback?
I'd probably factor out the ginkgo.GinkgoWriter because I hate globals but it really doesn't matter for test code. Same with all the var flags, but again, this is totally fine since it's test code.
Also lately i've been favoring three sections of go imports, stdlib, external imports, current project imports.
test/e2e/util/kind/setup.go
Outdated
buffer := &bytes.Buffer{} | ||
defer pipe.Close() | ||
for { | ||
n, _ := buffer.ReadFrom(pipe) |
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.
should this be reading until io.EOF or is n==0 the right thing?
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.
Oy, this rabbit hole led me down another, which is that we shouldn't be using exec.Cmd.Run with pipes, and instead should be using Start/Wait... working on cleaning up.
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.
Updated this, but still need to test the changes.
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.
hold off on reviewing the changes, it won't work as is 😂
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.
not a problem. i'm just starting to work on some of the trickier technical bits of the test proposal and this PR is a fairly crucial aspect :D
This has been my preference as well, but as a group we've been prioritizing for two groups, so I've been trying to follow the group preference. |
Aside from the visual aspect, there isn't any difference if you order imports one way or another, right? My personal preference is to ignore import grouping and let the tools do it however they want. IMHO there are more important things to be reviewing 😄 |
@ncdc yep, I guess I was making small talk in the PR. I didn't intend for that comment to be blocking nor do I have any interest in changing how we do things. I was asked for a review and since the code is test code and written in a readable way there really wasn't much to review. |
de4f9da
to
dddb2cf
Compare
test/e2e/util/kind/setup.go
Outdated
|
||
// Cluster represents the running state of a KIND cluster. | ||
// An empty struct is enough to call Setup() on. | ||
type Cluster struct { |
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 can see this becoming a ManagementCluster concept and we end up with constructors such as NewManagementClusterFromKind() or similar that allows us to keep the cluster concept and abstract out kind. I don't always want kind to be my management 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.
I should mention this is future work, i think this is totally fine as is.
2f126ef
to
42c6627
Compare
/test pull-cluster-api-integration |
42c6627
to
deb1375
Compare
@chuckha I believe this should be good to go now. |
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.
We should add a doc.go
example of how to use these test utilities, or a README for developers
"github.com/onsi/ginkgo" | ||
) | ||
|
||
var _ = ginkgo.Describe("functional tests", 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.
Do we need this file?
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 mainly added this to facilitate testing of the utilities, but after CAPD and CABPK are in tree I think we'd want to have E2E testing.
Not directly related, but after this is merged I want to refactor the integration tests to use these utilities as well, so I think I'll move them up a directory to avoid confusion for why the integration tests are importing a package from the e2e dir.
deb1375
to
1134f61
Compare
Agreed, I can do that after I've updated CAPA to use these utilities and create an example based off of that. |
- Add common kind utility for provider-based testing - Add flag helper - Add scheme helper - Add component helper
1134f61
to
fe32969
Compare
suiteTmpDir string | ||
) | ||
|
||
var _ = ginkgo.BeforeSuite(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.
Is there a way to opt out of the before and after suites?
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.
Not that I'm aware of, but I wouldn't expect other providers to inherit this suite directly, but instead we'd continue to move common logic into the helpers, and have each provider implement their own suites.
} | ||
|
||
gomega.Expect(cmd.Start()).NotTo(gomega.HaveOccurred()) | ||
wg.Wait() |
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 wait groups and go routines can be avoided completely by:
- getting a reference to the stderr and stdout pipes
- Running start
- Running ioutil.ReadAll on the pipes
- waiting for the command to finish
This example is a decent start: https://golang.org/pkg/os/exec/#Cmd.StderrPipe
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 ioutil.ReadAll
example seems geared towards a single pipe, if we used it here with both pipes, wouldn't it output the streams separately instead of attempting to interleave them?
limitations under the License. | ||
*/ | ||
|
||
package kind |
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.
Is there any room for non-kind based e2e tests? it would be good if we didn't bind ourselves to kind in the package name when it's an implementation detail. The one way I could see a package name of kind
being good is if we have multiple ways to spin up a management cluster and kind is one of those ways.
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 believe there may be, but I would expect them to implement their own helper.
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.
/lgtm
/hold
/hold cancel |
What this PR does / why we need it:
Starts to build out common testing utilities for use across providers