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.

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

Merged
merged 1 commit into from
Nov 22, 2022
Merged

e2e test #421

merged 1 commit into from
Nov 22, 2022

Conversation

kannon92
Copy link
Contributor

@kannon92 kannon92 commented Oct 19, 2022

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.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 19, 2022
@k8s-ci-robot
Copy link
Contributor

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 19, 2022
@kannon92
Copy link
Contributor Author

/kind do-not-merge

@k8s-ci-robot
Copy link
Contributor

@kannon92: The label(s) kind/do-not-merge cannot be applied, because the repository doesn't have them.

In response to this:

/kind do-not-merge

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.

@kannon92
Copy link
Contributor Author

kannon92 commented Oct 19, 2022

/hold

@k8s-ci-robot
Copy link
Contributor

@kannon92: The label(s) kind/hold cannot be applied, because the repository doesn't have them.

In response to this:

/kind hold

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.

@kannon92 kannon92 changed the title e2e test WIP e2e test Oct 19, 2022
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 19, 2022
@alculquicondor
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 19, 2022
Copy link
Contributor

@alculquicondor alculquicondor left a 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?

Makefile Outdated Show resolved Hide resolved
test/e2e/e2e_test.go Outdated Show resolved Hide resolved
test/e2e/e2e_test.go Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
config/components/manager/manager.yaml Outdated Show resolved Hide resolved
test/e2e/suite_test.go Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@kannon92 kannon92 changed the title WIP e2e test e2e test Oct 21, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 21, 2022
@kannon92
Copy link
Contributor Author

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.

@kannon92 kannon92 requested review from alculquicondor and kerthcet and removed request for denkensk, ArangoGutierrez and alculquicondor October 21, 2022 20:46
@kannon92 kannon92 requested review from kerthcet and removed request for alculquicondor October 21, 2022 20:47
Makefile Outdated Show resolved Hide resolved
test/e2e/suite_test.go Outdated Show resolved Hide resolved
test/e2e/suite_test.go Outdated Show resolved Hide resolved
test/e2e/suite_test.go Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@kannon92
Copy link
Contributor Author

kannon92 commented Nov 2, 2022

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.

@kannon92
Copy link
Contributor Author

kannon92 commented Nov 2, 2022

/kind productionization
/kind feature

@k8s-ci-robot k8s-ci-robot added kind/productionization kind/feature Categorizes issue or PR as related to a new feature. labels Nov 2, 2022
Copy link
Contributor

@kerthcet kerthcet left a 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.

test/e2e/config/kustomization.yaml Outdated Show resolved Hide resolved
test/e2e/config/image_pull_policy.yaml Outdated Show resolved Hide resolved
test/e2e/e2e_test.go Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
test/e2e/framework/framework.go Outdated Show resolved Hide resolved
test/e2e/e2e_test.go Outdated Show resolved Hide resolved
test/e2e/e2e_test.go Outdated Show resolved Hide resolved
test/e2e/e2e_test.go Outdated Show resolved Hide resolved
test/e2e/e2e_test.go Outdated Show resolved Hide resolved
test/e2e/e2e_test.go Outdated Show resolved Hide resolved
@kerthcet
Copy link
Contributor

kerthcet commented Nov 4, 2022

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.

Copy link
Contributor

@kerthcet kerthcet left a 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/e2e_test.go Show resolved Hide resolved
test/e2e/e2e_test.go Outdated Show resolved Hide resolved
test/e2e/e2e_test.go Outdated Show resolved Hide resolved
)

func CreateClientUsingCluster() client.Client {

Copy link
Contributor

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 Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
test/e2e/e2e_test.go Outdated Show resolved Hide resolved
test/e2e/e2e_test.go Outdated Show resolved Hide resolved
test/e2e/e2e_test.go Show resolved Hide resolved
test/e2e/framework/framework.go Outdated Show resolved Hide resolved
test/e2e/suite_test.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 18, 2022
Makefile Outdated Show resolved Hide resolved
hack/e2e-test.sh Outdated Show resolved Hide resolved
hack/e2e-test.sh Outdated Show resolved Hide resolved
hack/e2e-test.sh Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
hack/e2e-test.sh Show resolved Hide resolved
pkg/util/testing/wrappers.go Outdated Show resolved Hide resolved
test/e2e/e2e_test.go Outdated Show resolved Hide resolved
test/e2e/e2e_test.go Show resolved Hide resolved
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
Copy link
Contributor

Choose a reason for hiding this comment

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

why not false?

Copy link
Contributor Author

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.

Copy link
Contributor

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)

Copy link
Contributor

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 :)

Copy link
Contributor Author

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
Comment on lines 147 to 149
.PHONY: test-e2e
test-e2e: USE_EXISTING_CLUSTER=true
test-e2e: test-e2e-kind
Copy link
Contributor

@alculquicondor alculquicondor Nov 22, 2022

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.

Copy link
Contributor

@alculquicondor alculquicondor left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 22, 2022
@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 22, 2022
@k8s-ci-robot k8s-ci-robot merged commit ae528ba into kubernetes-sigs:main Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. kind/productionization lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add E2E test setup and first test
6 participants