New controller tests #2580

piotrmiskiewicz · 2019-03-12T07:45:16Z

This PR is a

Feature Implementation
Bug Fix
Documentation

What this PR does / why we need it:
This PR introduces controller tests, which will replace integration test after migration to CRDs. All new tests can be run with an existing implementation (API Server) because it tests only the controller. The main difference between existing controller unit tests and new tests is the unit tests are testing the reconcileXXX method. New tests are testing the controller as a black box, but do not need to start API Server. After the migration to CRDs - current integration tests will be removed.

This PR provides first part of migrated tests, see the table:

Existing integration test method	done	description
TestBasicFlows	Yes	all subtests migrated to TestBasicFlow
TestBasicFlowsWithOriginatingIdentity	No
TestBindingFailure	Yes	migrated to TestServiceBindingFailure
TestServiceBindingOrphanMitigation	Yes	migrated to TestServiceBindingOrphanMitigation
TestServiceBindingDeleteWithAsyncBindInProgress	No
TestServiceInstanceDeleteWithAsyncProvisionInProgress	No
TestServiceInstanceDeleteWithAsyncUpdateInProgress	No
TestAsyncProvisionWithMultiplePolls	Yes	covered by `TestBasicFlow/async instances with multiple polls`
TestCreateServiceInstanceAsynchronous	Yes	it is simplified scenario tested in `TestBasicFlow/async instances with multiple polls`
TestCreateServiceInstanceFailsWithNonexistentPlan	No
TestCreateServiceInstanceNonExistentClusterServiceBroker	No
TestCreateServiceInstanceNonExistentClusterServiceClassOrPlan	No
TestCreateServiceInstanceWithInvalidParameters	No
TestCreateServiceInstanceWithParameters	No
TestCreateServiceInstanceWithProvisionFailure	No
TestCreateServiceInstanceWithRetries	Yes	migrated to TestProvisionInstanceWithRetries
TestDeleteServiceInstance	partially	some of scenarios covered by TestBasicFlows
TestRetryAsyncDeprovision	No
TestUpdateServiceInstanceChangePlans	No
TestUpdateServiceInstanceChangePlansToNonexistentPlan	No
TestUpdateServiceInstanceNewDashboardResponse	No
TestUpdateServiceInstanceUpdateParameters	No
TestCreateServiceBindingInstanceNotReady	No
TestCreateServiceBindingInvalidInstance	Yes	migrated to TestServiceBindingRetryForNonExistingClass
TestCreateServiceBindingInvalidInstanceFailure	No
TestCreateServiceBindingNonBindable	No
TestCreateServiceBindingSuccess	Yes	TestBasicFlows
TestCreateServiceBindingWithParameters	No
TestCreateServiceBindingWithSecretTransform	No
TestDeleteServiceBindingFailureRetry	No
TestDeleteServiceBindingFailureRetryAsync	No
TestClusterServiceClassRemovedFromCatalogWithoutInstances	No
TestClusterServicePlanRemovedFromCatalogWithoutInstances	No
TestClusterServiceClassRemovedFromCatalogAfterFiltering	No

Which issue(s) this PR fixes

Resolves: kyma-project/kyma#2840

Please leave this checklist in the PR comment so that maintainers can ensure a good PR.

Merge Checklist:

New feature
- Tests
- Documentation
SVCat CLI flag
Server Flag for config
- Chart changes
- removing a flag by marking deprecated and hiding to avoid
  breaking the chart release and existing clients who provide a
  flag that will get an error when they try to update

piotrmiskiewicz · 2019-03-12T07:45:24Z

/hold

k8s-ci-robot · 2019-03-12T07:45:29Z

Hi @piotrmiskiewicz. Thanks for your PR.

I'm waiting for a kubernetes-incubator or kubernetes 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.

jberkhahn · 2019-03-12T17:22:41Z

/ok-to-test

mszostok · 2019-03-17T23:51:49Z

pkg/controller/flow_test.go

+	ct.AssertOSBBasicAuth(t, "user1", "p2sswd")
+	ct.AssertClusterServiceClassAndPlan(t)
+
+	// uncomment when the https://github.com/kubernetes-incubator/service-catalog/issues/2563 is fixed


comment still valid?

the comment removed

mszostok · 2019-03-18T00:03:02Z

pkg/controller/case_test.go

+		ObjectMeta: metav1.ObjectMeta{
+			Namespace:  testNamespace,
+			Name:       testBindingName,
+			Generation: 1,                                         // set by the Webhook


misleading comment. It's not set by webhook. The main k8s server is doing that.

you are right, removed

mszostok · 2019-03-18T00:07:35Z

pkg/controller/case_test.go

+	}
+}
+
+func (ct *ControllerTest) createClusterServiceBroker() *v1beta1.ClusterServiceBroker {


IMO is misleading:

you have function called createClusterServiceBroker which is returning fixture and also CreateSimpleClusterServiceBroker which is calling k8s Create function. Maybe the first one should be called just fixClusterServiceBroker? U already using that for other func e.g. fixCatalogResponse

mszostok · 2019-03-18T00:08:52Z

pkg/controller/case_test.go

+}
+
+func (ct *ControllerTest) CreateSimpleClusterServiceBroker() {
+	ct.scInterface.ClusterServiceBrokers().Create(ct.createClusterServiceBroker())


why you are not returning the error? it can be important in test to assert that. I know that is a fake client but still, it can produce an error (e.g. when u add ProxyReactor)

This comment applies for all Create* functions

ok, I've added error to methods Create*

mszostok · 2019-03-18T00:10:05Z

pkg/controller/case_test.go

@@ -0,0 +1,583 @@
+/*


IMO we should add some information/table which will show us what kind of test your PR covers.

Right now it's hard to me check what test was already replaced with the new approach.

added a table with such information to the description of the PR

mszostok · 2019-03-18T00:23:03Z

pkg/controller/flow_test.go

+	// THEN
+	assert.NoError(t, ct.WaitForReadyInstance())
+
+	// uncomment when the https://github.com/kubernetes-incubator/service-catalog/issues/2563 is fixed


what should be uncommented? Is that Comment still valid?

mszostok · 2019-03-18T00:23:29Z

pkg/controller/flow_test.go

@@ -0,0 +1,182 @@
+/*
+Copyright 2018 The Kubernetes Authors.


Suggested change

Copyright 2018 The Kubernetes Authors.

Copyright 2019 The Kubernetes Authors.

mszostok · 2019-03-18T00:23:52Z

pkg/controller/flow_test.go

@@ -0,0 +1,182 @@
+/*


IMO the filename is too generic. Could u precise the flow_test? it's scoped for binding only if yes, then what kind of flow u test? The binding_auth_flow_test.go will be ok name?

I've renamed it to controller_flow_test. The best name would be jsut controller_test but it is used by existing unit tests.

mszostok · 2019-03-18T00:25:37Z

pkg/controller/flow_test.go

+}
+
+func TestServiceBindingFailure(t *testing.T) {
+	ct := NewControllerTest(t)


if u want to have that consistent then u miss the // GIVEN comment

mszostok · 2019-03-18T00:25:41Z

pkg/controller/flow_test.go

+}
+
+func TestServiceBindingRetryForNonExistingClass(t *testing.T) {
+	ct := NewControllerTest(t)


if u want to have that consistent then u miss the // GIVEN comment

piotrmiskiewicz · 2019-03-21T13:06:48Z

/hold cancel

jboyd01 · 2019-03-25T19:32:24Z

some general comments:

public functions should have doc. IE all the TestBasicFlowWithBasicAuth(), TestBasicFlow(), TestServiceBindingOrphanMitigation()
I like the many advantages of these tests over the existing tests that require starting a server and can't be run in parallel, however this doesn't touch upon what we'll do to handle testing functionality that uses Web Hooks. If we are going to replace existing integration tests we should have a plan and ensure we have agreement.
should existing integration tests that are duplicated here be removed from the other integ tests? It would help to ensure we implement new tests for all existing tests if we carefully remove and document any issues. I'd rather see this then try to maintain a table. I don't envision a quick cut over to CRD - I think we will need to support Aggregated API & CRD simultaneously. This probably needs more discussion outside of this PR.

piotrmiskiewicz · 2019-03-26T11:54:43Z

I've added comments to test methods.

Current integration tests are testing all logic implemented in the controller and API Server. I didn't wanted to remove it until we decide (and do) what should be moved to e2e tests. New controller tests are not testing validations/defaults implemented in API Server or Webhooks (in CRD solution). If you think we can remove current integration tests which are migrated - we can do that. I didn't want to decrease the coverage.

In my opinion the e2e test must cover basic scenario(s) which touches all parts, for example: creates a resource with only necessary fields (all defaults are filled by webhooks), validations (then we are sure such webhooks are executed). On the other hand I don't think we need to check in e2e test like orphan mitigation scenario, retries etc.

The next step would be to add necessary cases to e2e test.

jberkhahn · 2019-03-26T23:00:29Z

pkg/controller/case_test.go

+	stopCh           chan struct{}
+}
+
+func NewControllerTest(t *testing.T) *ControllerTest {


I think the helper methods in here should be commented as well

we do not need to have it public, I've renamed it to newControllerTest

I mean, i would still want them commented so someone new coming in can know what they do and how to use them

ok, I put comments

jberkhahn · 2019-03-26T23:02:00Z

pkg/controller/case_test.go

+	}
+}
+
+func (ct *ControllerTest) fixClusterServiceBroker() *v1beta1.ClusterServiceBroker {


I don't understand why this is called "fix"? It looks to me like it's just making a basic example broker.

It is a fixture. It is not a best idea to call it createXXX because "CreateXXX" methods creates an object in k8s (fake clients).

ah that makes sense. I would change the name to fixtureXXX then.

jberkhahn · 2019-03-26T23:05:14Z

pkg/controller/case_test.go

+
+	"github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1"
+	fakesc "github.com/kubernetes-incubator/service-catalog/pkg/client/clientset_generated/clientset/fake"
+	scinterface "github.com/kubernetes-incubator/service-catalog/pkg/client/clientset_generated/clientset/typed/servicecatalog/v1beta1"


I guess 'scinterface' is a better name than 'v1beta1', but I was super confused until I realized you had renamed this import

there is another v1bet1 and I had to rename it

piotrmiskiewicz · 2019-03-28T14:12:51Z

/test pull-service-catalog-xbuild

piotrmiskiewicz · 2019-04-01T05:43:08Z

/test pull-service-catalog-unit

piotrmiskiewicz · 2019-04-01T11:12:06Z

/test pull-service-catalog-integration

jberkhahn · 2019-04-01T16:50:31Z

/lgtm

jberkhahn · 2019-04-01T23:35:13Z

could we move the chart over to an issue so we can keep track of which tests have been migrated moving foward?

MHBauer · 2019-04-01T23:38:00Z

Table needs to move to issue or be encoded in the source.

Best case is that this takes the integration tests down to the level without an apiserver.

MHBauer · 2019-04-01T23:38:08Z

/approve

k8s-ci-robot · 2019-04-01T23:38:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MHBauer

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:

~~OWNERS~~ [MHBauer]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* New controller tests (#2580) * Fix fmt issues * Add missing comments, add missing gitignore rule

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 12, 2019

k8s-ci-robot requested review from jberkhahn and jboyd01 March 12, 2019 07:45

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 12, 2019

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 12, 2019

piotrmiskiewicz force-pushed the controller-flow-test branch from 9619dfc to 26c3f56 Compare March 12, 2019 07:53

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 12, 2019

piotrmiskiewicz force-pushed the controller-flow-test branch from 22bc3fe to e1732b0 Compare March 12, 2019 10:49

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 Mar 12, 2019

mszostok reviewed Mar 18, 2019

View reviewed changes

piotrmiskiewicz force-pushed the controller-flow-test branch 2 times, most recently from b95ff0c to 1e934ed Compare March 21, 2019 12:53

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 21, 2019

piotrmiskiewicz force-pushed the controller-flow-test branch 2 times, most recently from 4ab57b8 to 632d019 Compare March 25, 2019 13:37

piotrmiskiewicz force-pushed the controller-flow-test branch from 0b9f6ce to 5b6f645 Compare March 26, 2019 09:18

jberkhahn reviewed Mar 26, 2019

View reviewed changes

piotrmiskiewicz force-pushed the controller-flow-test branch from a5403b9 to ea0556e Compare March 29, 2019 07:01

k8s-ci-robot assigned jberkhahn Apr 1, 2019

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 1, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 1, 2019

k8s-ci-robot merged commit 9f31b7d into kubernetes-retired:master Apr 1, 2019

piotrmiskiewicz added a commit to piotrmiskiewicz/service-catalog that referenced this pull request Apr 4, 2019

New controller tests (kubernetes-retired#2580)

d75a2d8

piotrmiskiewicz added a commit to piotrmiskiewicz/service-catalog that referenced this pull request Apr 4, 2019

New controller tests (kubernetes-retired#2580)

2eeca4c

piotrmiskiewicz added a commit to piotrmiskiewicz/service-catalog that referenced this pull request Apr 4, 2019

New controller tests (kubernetes-retired#2580)

a4547f2

piotrmiskiewicz mentioned this pull request Apr 4, 2019

Rewrite controller tests #2597

Closed

piotrmiskiewicz added a commit to piotrmiskiewicz/service-catalog that referenced this pull request Apr 4, 2019

New controller tests (kubernetes-retired#2580)

9210c5f

piotrmiskiewicz added a commit to piotrmiskiewicz/service-catalog that referenced this pull request Apr 4, 2019

New controller tests (kubernetes-retired#2580)

f4b8666

piotrmiskiewicz added a commit to piotrmiskiewicz/service-catalog that referenced this pull request Apr 4, 2019

New controller tests (kubernetes-retired#2580)

aa8015f

piotrmiskiewicz added a commit to piotrmiskiewicz/service-catalog that referenced this pull request Apr 4, 2019

New controller tests (kubernetes-retired#2580)

b29ad9e

piotrmiskiewicz added a commit to piotrmiskiewicz/service-catalog that referenced this pull request Apr 4, 2019

New controller tests (kubernetes-retired#2580)

3894618

viviyww pushed a commit to viviyww/service-catalog that referenced this pull request May 10, 2019

New controller tests (kubernetes-retired#2580)

d041767

piotrmiskiewicz added a commit to piotrmiskiewicz/service-catalog that referenced this pull request May 15, 2019

New controller tests (kubernetes-retired#2580)

c8990ad

piotrmiskiewicz added a commit to piotrmiskiewicz/service-catalog that referenced this pull request May 15, 2019

New controller tests (kubernetes-retired#2580)

4631811

mszostok mentioned this pull request May 19, 2019

REQUEST: New membership for @mszostok kubernetes/org#832

Closed

6 tasks

piotrmiskiewicz added a commit to piotrmiskiewicz/service-catalog that referenced this pull request Jun 12, 2019

New controller tests (kubernetes-retired#2580)

6c7df3e

piotrmiskiewicz added a commit to piotrmiskiewicz/service-catalog that referenced this pull request Jun 14, 2019

New controller tests (kubernetes-retired#2580)

f909efe

mszostok pushed a commit to piotrmiskiewicz/service-catalog that referenced this pull request Jun 24, 2019

New controller tests (kubernetes-retired#2580)

cd7d064

mszostok mentioned this pull request Jun 24, 2019

New controller tests - deletion resources #2596

Merged

10 tasks

k8s-ci-robot pushed a commit that referenced this pull request Jun 25, 2019

New controller tests - deletion resources (#2596)

6d7cea6

* New controller tests (#2580) * Fix fmt issues * Add missing comments, add missing gitignore rule

piotrmiskiewicz added a commit to piotrmiskiewicz/service-catalog that referenced this pull request Aug 29, 2019

New controller tests (kubernetes-retired#2580)

c01657a

	Copyright 2018 The Kubernetes Authors.
	Copyright 2019 The Kubernetes Authors.

New controller tests #2580

New controller tests #2580

Conversation

piotrmiskiewicz commented Mar 12, 2019 • edited Loading

piotrmiskiewicz commented Mar 12, 2019

k8s-ci-robot commented Mar 12, 2019

jberkhahn commented Mar 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotrmiskiewicz Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mszostok Mar 18, 2019 • edited Loading

Choose a reason for hiding this comment

piotrmiskiewicz Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotrmiskiewicz commented Mar 21, 2019

jboyd01 commented Mar 25, 2019

piotrmiskiewicz commented Mar 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotrmiskiewicz commented Mar 28, 2019

piotrmiskiewicz commented Apr 1, 2019

piotrmiskiewicz commented Apr 1, 2019

jberkhahn commented Apr 1, 2019

jberkhahn commented Apr 1, 2019

MHBauer commented Apr 1, 2019

MHBauer commented Apr 1, 2019

k8s-ci-robot commented Apr 1, 2019

piotrmiskiewicz commented Mar 12, 2019 •

edited

Loading

piotrmiskiewicz Mar 21, 2019 •

edited

Loading

mszostok Mar 18, 2019 •

edited

Loading

piotrmiskiewicz Mar 21, 2019 •

edited

Loading

piotrmiskiewicz commented Mar 26, 2019 •

edited

Loading