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

[Federation] Add type-agnostic e2e crud test #43194

Merged
merged 1 commit into from
Apr 17, 2017

Conversation

marun
Copy link
Contributor

@marun marun commented Mar 16, 2017

This PR proposes an e2e test that reuses the type-agnostic crudtester already used for integration testing to validate crud against a deployed cluster. It is intended to to eventually replace the existing e2e tests for simple types like secrets, but for now will run in addition to the existing testing to gain confidence in the coverage it provides.

The deletion corner cases - when orphanDependents is nil or true - do not involve operations in the member clusters and are already well-tested in integration testing and so are not reimplemented here.

Where it can be applied, this approach of abstracting a test from its execution environment - making the test 'retargetable' - can reduce the cost of test development since the bulk of the work can be iterated on as an integration test. It can also serve as a check on assumptions made in the integration test about how a deployed environment will behave.

cc: @kubernetes/sig-federation-pr-reviews @kubernetes/sig-testing-misc @smarterclayton @derekwaynecarr

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 16, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Mar 16, 2017
@madhusudancs
Copy link
Contributor

@nikhiljindal assigning it to you because you are reviewing other integration test PRs and have better context about it.

@marun
Copy link
Contributor Author

marun commented Mar 16, 2017

cc: @perotinus

@@ -31,7 +31,7 @@ import (
)

type SecretAdapter struct {
client federationclientset.Interface
Client federationclientset.Interface
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be public? You could add a NewSecretAdapter method instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason to choose one approach over another? I've generally preferred to use factory methods only when more than simple assignment is required.

Copy link
Contributor

@perotinus perotinus left a comment

Choose a reason for hiding this comment

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

This looks good overall. I do question whether the CRUDHelper should be in a separate package that is depended on by both integration and e2e tests: it seems more like a generic testing harness than something directly tied to either kind of test.

@marun
Copy link
Contributor Author

marun commented Mar 17, 2017

@perotinus I agree that CRUDHelper should be separated, if only because otherwise any e2e using it will be pulling in everything needed to run federation and kubernetes api servers. I think having it live in something like test/integration/federation/crud could be ok, though. I don't think there is precedent for sharing test code between integration and e2e, and I'm not sure it makes sense to specialize its placement just yet.

@perotinus
Copy link
Contributor

@marun Yes, that's kind of what I had in mind too. I think your idea of package structure is reasonable for now. I'm not familiar enough with the test tree to really know of a better place, though I could see either a federation subdir fixtures or utils being appropriate.

This LGTM.

@marun marun changed the title WIP [Federation] Refactor secret e2e to use integration crud [Federation] Add generic crud e2e and enable for secrets Mar 22, 2017
@marun marun force-pushed the fed-refactor-secret-e2e branch 2 times, most recently from 5ead989 to 1e55903 Compare March 22, 2017 20:43
@marun
Copy link
Contributor Author

marun commented Mar 22, 2017

I've rebased this on top of #43500 to benefit from the refactoring it did, and switched to adding a new test instead of replacing the old one. My intent is to reduce the risk of losing coverage while the kinks in this approach are worked out.

@marun marun force-pushed the fed-refactor-secret-e2e branch 2 times, most recently from 438b33f to 48caf3d Compare March 24, 2017 22:31
It("should be created, read, updated and deleted successfully", func() {
fedframework.SkipUnlessFederated(f.ClientSet)

// Only need to load clients once
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need to do this in the test block? Will GetClusterClients not work in the outer scope?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Getting the cluster clients is an expensive operation. No point in doing it if the test will be skipped.

@marun marun force-pushed the fed-refactor-secret-e2e branch 2 times, most recently from 27a3e09 to 06675be Compare March 24, 2017 23:59
@perotinus
Copy link
Contributor

LGTM.

@perotinus
Copy link
Contributor

@csbell @nikhiljindal @madhusudancs Can someone else approve this?

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 30, 2017
@marun marun changed the title [Federation] Add generic crud e2e and enable for secrets WIP [Federation] Add generic crud e2e and enable for secrets Mar 30, 2017
@marun marun changed the title WIP [Federation] Add generic crud e2e and enable for secrets [Federation] Add generic crud e2e and enable for secrets Mar 30, 2017
@k8s-github-robot k8s-github-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 7, 2017
@marun marun changed the title [Federation] Add generic crud e2e and enable for secrets [Federation] Add type-agnostic e2e crud test Apr 7, 2017
@marun marun added the release-note-none Denotes a PR that doesn't merit a release note. label Apr 7, 2017
@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed release-note-label-needed labels Apr 7, 2017
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 7, 2017
@madhusudancs
Copy link
Contributor

@k8s-bot pull-kubernetes-federation-e2e-gce test this

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 13, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 13, 2017
@marun
Copy link
Contributor Author

marun commented Apr 13, 2017

@k8s-bot bazel test this

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 17, 2017
@marun
Copy link
Contributor Author

marun commented Apr 17, 2017

rebased

@k8s-bot pull-kubernetes-federation-e2e-gce test this

@marun
Copy link
Contributor Author

marun commented Apr 17, 2017

@csbell
Copy link
Contributor

csbell commented Apr 17, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 17, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: csbell, marun

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 44519, 43194, 44513)

@k8s-github-robot k8s-github-robot merged commit 99b2ab0 into kubernetes:master Apr 17, 2017
@k8s-ci-robot
Copy link
Contributor

@marun: The following test(s) failed:

Test name Commit Details Rerun command
Jenkins GCE e2e d979210 link @k8s-bot cvm gce e2e test this

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@marun marun deleted the fed-refactor-secret-e2e branch April 17, 2017 21:20
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. 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.

None yet

8 participants