Move Federation e2e test code to independent package #39738

Merged
merged 4 commits into from Jan 19, 2017

Projects

None yet

9 participants

@shashidharatd
Member

What this PR does / why we need it: Move federation e2e test code to an independent package called e2e_federation. This will help in multiple ways.

  • easy to move the federation related code to a separate repo from core.
  • one step closer to register/unregister clusters to federation only once during e2e instead of every test case. we need to introduce singleton to register cluster during framework creation which will be handled in subsequent PR.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
Optimize federation e2e suite which takes long time to execute currently.

Special notes for your reviewer: I have tried to segregate into multiple commits. request to review commit by commit. also mostly the change is about moving the functions to a new location/package.

Release note:

@madhusudancs @nikhiljindal @colhom

@k8s-reviewable

This change is Reviewable

@k8s-ci-robot
Collaborator

Jenkins GCI GKE smoke e2e failed for commit 3f988bf. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake 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.

@shashidharatd
Member

@k8s-bot gci gke e2e test this

@k8s-ci-robot
Collaborator

Jenkins GKE smoke e2e failed for commit 3f988bf. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake 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.

@shashidharatd
Member

@k8s-bot cvm gke e2e test this

@csbell
Contributor
csbell commented Jan 12, 2017

Hi @shashidharatd, do you anticipate building upon this PR to revisit #35078 ? Thanks.

@k8s-ci-robot
Collaborator

Jenkins unit/integration failed for commit 7917f13. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake 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.

@shashidharatd
Member

Hi @csbell, #35078 and this PR are not related. #35078 is about e2e upgrade tests and not particular to federation. I would revisit that PR over weekend and start testing.
For federation upgrade testing, we need to think through on how to implement it. mostly will be similar to the e2e upgrade testing. I will be working along with @quinton-hoole on federation upgrade testing in coming days.

@shashidharatd
Member

@k8s-bot unit test this

test/e2e_federation/framework/util.go
+ })
+}
+
+func LoadFederationClientset_1_5() (*federation_clientset.Clientset, error) {
@nikhiljindal
nikhiljindal Jan 18, 2017 Member

Can drop "_1_5" at the end

+ . "github.com/onsi/ginkgo"
+ . "github.com/onsi/gomega"
+ yaml "gopkg.in/yaml.v2"
+ //"github.com/golang/glog"
+type Framework struct {
+ *framework.Framework
+
+ // Federation specific params. These are set only if federated = true.
@nikhiljindal
nikhiljindal Jan 18, 2017 Member

obsolete comment. federated param doesnt exist now

+ *framework.Framework
+
+ // Federation specific params. These are set only if federated = true.
+ FederationClientset_1_5 *federation_clientset.Clientset
@nikhiljindal
nikhiljindal Jan 18, 2017 Member

Can drop "_1_5" at the end

"k8s.io/kubernetes/test/e2e/framework"
+ fedframework "k8s.io/kubernetes/test/e2e_federation/framework"
@nikhiljindal
nikhiljindal Jan 18, 2017 Member

We can just call this framework and then we wont need replace framework -> fedframework changes.

@shashidharatd
shashidharatd Jan 18, 2017 Member

some of the utility functions from e2e.framework package are being used in federation e2e tests like ExpectNoError, Failf, these are not methods of framework type, so we need to include both these packages e2e.framework and e2e_federation.framework for now.

@@ -153,6 +154,9 @@ var _ = ginkgo.SynchronizedBeforeSuite(func() []byte {
// Reference common test to make the import valid.
commontest.CurrentSuite = commontest.E2E
+ // Reference federation test to make the import valid.
+ federationtest.FederationSuite = commontest.FederationE2E
@nikhiljindal
nikhiljindal Jan 18, 2017 Member

Should this be only if FEDERATION=true?

@shashidharatd
shashidharatd Jan 18, 2017 Member

This variable federationtest.FederationSuite is created and called just so that the e2e_federation package is included in e2e package during compilation. It wouldn't have any impact at runtime, so using this variable inside if FEDERATION=true will not be of any use.

@nikhiljindal
Member
nikhiljindal commented Jan 18, 2017 edited

Thanks for the PR!
Since you are moving files, can you also fix the names so that all of them are of the form federated-*. Right now some are federation-* (federation-deployment.go) and some are federated-* (federated-secret.go).

@shashidharatd
Member

Hi @nikhiljindal, I have handled the comments and have given justification for couple of comments. PTAL.
Also I have renamed few files from federation-* to federated-*, but for some files like federation-util.go, it didn't make since, so the filenames are not changed for few files. Do let me know if it is OK.

@k8s-ci-robot
Collaborator

Jenkins verification failed for commit 1d8c7ca. Full PR test history. cc @shashidharatd

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake 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.

@shashidharatd
Member

@k8s-bot verify test this

@nikhiljindal
Member

LGTM, thanks for all the fixes.
Feel free to self apply the label after rebasing

shashidharatd added some commits Jan 11, 2017
@shashidharatd shashidharatd Move all federation e2e testacases to independent package 62c9267
@shashidharatd shashidharatd Move some common functions in e2e to e2e.framework for reusability 79ce4cb
@shashidharatd shashidharatd segrageted federation related test code from e2e.framework d515eb4
@shashidharatd shashidharatd auto-generated bazel BUILD file changes
85c3f0d
@k8s-ci-robot
Collaborator

Jenkins Bazel Build failed for commit 85c3f0d. Full PR test history. cc @shashidharatd

The magic incantation to run this job again is @k8s-bot bazel test this. Please help us cut down flakes by linking to an open flake 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.

@nikhiljindal
Member

@k8s-bot bazel test this

@nikhiljindal nikhiljindal added the lgtm label Jan 19, 2017
@shashidharatd
Member

Thanks @nikhiljindal for review and applying the label. I cannot apply the labels myself yet, would be troubling you guys for some more time ;)


Review status: 0 of 26 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@k8s-merge-robot
Collaborator

Automatic merge from submit-queue

@k8s-merge-robot k8s-merge-robot merged commit 3166a30 into kubernetes:master Jan 19, 2017

15 of 16 checks passed

code-review/reviewable 26 files left
Details
Jenkins Bazel Build Build succeeded.
Details
Jenkins CRI GCE Node e2e Build succeeded.
Details
Jenkins CRI GCE e2e Build succeeded.
Details
Jenkins GCE Node e2e Build succeeded.
Details
Jenkins GCE e2e Build succeeded.
Details
Jenkins GCE etcd3 e2e Build succeeded.
Details
Jenkins GCI GCE e2e Build succeeded.
Details
Jenkins GCI GKE smoke e2e Build succeeded.
Details
Jenkins GKE smoke e2e Build succeeded.
Details
Jenkins Kubemark GCE e2e Build succeeded.
Details
Jenkins kops AWS e2e Build succeeded.
Details
Jenkins unit/integration Build succeeded.
Details
Jenkins verification Build succeeded.
Details
Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation shashidharatd authorized
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment