-
Notifications
You must be signed in to change notification settings - Fork 300
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 #421
e2e test #421
Conversation
Hi @kannon92. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
/kind do-not-merge |
@kannon92: The label(s) 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. |
/hold |
@kannon92: The label(s) 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. |
/ok-to-test |
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.
Have you tried setting a sleep before starting the tests?
I wonder if the controller is just starting up. Once you have confirmed if that's the problem, we could query the Available
condition of the Deployment before starting the controller?
This is ready for review. I figured out the webhook issue and I have a passing test. Please review the logic and add what you think you'd like to see in our first round of e2e tests. I also could use some guidance on a better way to write the kustomization configs for the e2e tests. |
I created kubernetes/test-infra#27868 once this gets merged. I think this should run the e2e tests but I'll leave the above PR as WIP until we merge this one. |
/kind productionization |
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 @kannon92 for bringing us e2e test framework, I think we're heading to the right way. Just some comments left.
One more thing, better to append the commits not squashing them, it's hard to see what changes after. We can squash at the last. |
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.
Just some nits, others LGTM.
test/e2e/framework/framework.go
Outdated
) | ||
|
||
func CreateClientUsingCluster() client.Client { | ||
|
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.
Remove the blank line here.
Makefile
Outdated
@@ -135,6 +143,15 @@ test-integration: manifests generate fmt vet envtest ginkgo ## Run tests. | |||
KUBEBUILDER_ASSETS="$(shell $(ENVTEST) --arch=amd64 use $(ENVTEST_K8S_VERSION) -p path)" \ | |||
$(GINKGO) --junit-report=junit.xml --output-dir=$(ARTIFACTS) -v $(INTEGRATION_TARGET) | |||
|
|||
USE_EXISTING_CLUSTER ?= true | |||
.PHONY: test-e2e-kind | |||
test-e2e-kind: USE_EXISTING_CLUSTER=true |
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.
why not false?
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.
Maybe I misunderstood.
I thought test-e2e
would be the e2e test in the CI (with creation/deletion of kind) and then test-e2e-kind would be for existing cluster.
To be honest, both require kind as I am loading the image into kind. The main difference is whether or not I assume a kind cluster exists.
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 think the CI should call test-e2e-kind
.
test-e2e
can be for an existing cluster (which might or might not be 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.
To be honest, both require kind as I am loading the image into kind
Ah right... Ideally we shouldn't make that assumption. I might want to upload an image to gcr and test on a GKE 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.
Ah right... Ideally we shouldn't make that assumption. I might want to upload an image to gcr and test on a GKE cluster :)
My feeling is that we file that as a followup. It wouldn't be difficult to do but I'd like to get these tests merged so we can get these running on PRs.
If you don't object, I'll create an issue to make the e2e tests (or another target) to run on an exisiting cluster where the images are stored in a repo somewhere.
Makefile
Outdated
.PHONY: test-e2e | ||
test-e2e: USE_EXISTING_CLUSTER=true | ||
test-e2e: test-e2e-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.
Given that we will follow up on running tests that don't imply kind, let's remove these lines for 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.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, kannon92 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 |
What type of PR is this?
Closes #61
Add a framework for running e2e tests
What this PR does / why we need it:
This allows us to run e2e tests in Prow (test-infra). We need to create a PR that references this make target for running e2e tests.
Which issue(s) this PR fixes:
Fixes #
#61
Special notes for your reviewer:
I think I need this PR merged and then I will add a e2e test to the testinfra project in Kubernetes.