New controller tests - deletion resources #2596

piotrmiskiewicz · 2019-04-04T10:17:21Z

This PR is a

Feature Implementation
Bug Fix
Documentation

What this PR does / why we need it:
The PR indroduces controller tests which will replace current integration tests, see #2597

rewritten tests:

TestServiceBindingDeleteWithAsyncBindInProgress ✅
TestServiceInstanceDeleteWithAsyncProvisionInProgress ✅
TestServiceInstanceDeleteWithAsyncUpdateInProgress ✅
TestDeleteServiceInstance (part of the test is covered by TestBasicFlows) ✅
TestDeleteServiceBindingFailureRetry ✅
TestDeleteServiceBindingFailureRetryAsync ✅
TestRetryAsyncDeprovision ✅
TestClusterServiceClassRemovedFromCatalogWithoutInstances ✅
TestClusterServicePlanRemovedFromCatalogWithoutInstances ✅
TestClusterServiceClassRemovedFromCatalogAfterFiltering ✅

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

k8s-ci-robot · 2019-04-04T10:17:35Z

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.

piotrmiskiewicz · 2019-04-04T10:18:06Z

/hold

jberkhahn · 2019-04-05T22:43:03Z

/ok-to-test

piotrmiskiewicz · 2019-04-10T12:14:57Z

/hold cancel

jberkhahn · 2019-04-29T20:33:28Z

What's the status of this? Is it ready to go?

jberkhahn · 2019-05-06T23:42:43Z

I would rebase this on master, as we've upgraded the kube dependencies since this was written. I'm getting a buncha test failures when I rebase this on master and run the units.

piotrmiskiewicz · 2019-05-15T14:01:18Z

I'll do the rebase

piotrmiskiewicz · 2019-05-16T20:00:57Z

It is ready to go

mszostok

do not forget to execute make format (because of that travis build failed)

Please also fix the commit message "New controller tests - deletion resources" cause it does not illustrate well what you implemented in that PR.

pkg/controller/case_test.go

pkg/controller/controller_flow_binding_test.go

pkg/controller/controller_flow_instance_test.go

pkg/controller/controller_flow_test.go

pkg/controller/controller_flow_binding_test.go

mszostok · 2019-06-13T10:41:45Z

An additional thing that I found:
Right now the unit tests take around 21sec

$ go test ./pkg/controller/... -count 1
ok      github.com/kubernetes-sigs/service-catalog/pkg/controller       21.009s

when previously it was around 0.2sec

$ go test ./pkg/controller/... -count 1
ok      github.com/kubernetes-sigs/service-catalog/pkg/controller       0.231s

IMO we should introduce the // +build and run those rewritten integration test only when someone explicitly executes go test --tags="integration". We should also modify the make integration target and run those test there by default, so it will be always checked by pull-service-catalog-integration CI job.

After rewriting all tests then the old one can be removed and under make integration we will execute only those tests with // +build integration

piotrmiskiewicz · 2019-06-14T11:29:26Z

I've added //+build integration to test files so those test will be run as test-integration target (I've modified also makefile)

MHBauer · 2019-06-14T17:38:58Z

Makefile

@@ -284,13 +284,15 @@ test-update-goldenfiles: .init

 build-integration: .generate_files
 	$(DOCKER_CMD) go test -race github.com/kubernetes-sigs/service-catalog/test/integration/... -c
+	$(DOCKER_CMD) go test --tags="integration" -race github.com/kubernetes-sigs/service-catalog/pkg/controller/... -c


we probably don't need to do this anymore. go caching should have improved everything enough that this doesn't save any time.

Need to ensure DOCKER_CMD mounts the cache dir, and we can drop the build ahead of running the tests part.

I totally agree with you but maybe let's do that as the follow-up and remove the building from all the test stages (build-integration and build-e2e).

MHBauer · 2019-06-14T17:40:09Z

pkg/controller/case_test.go

@@ -1,3 +1,5 @@
+// +build integration
+


are these actually integration tests, or can they be moved with all of the others that already exist?

are these actually integration tests,

those tests have the same contracts as the current integration tests but just not starting the API Server.

Some time ago we discussed that current integration tests will not work for CRDs implementation and because of that, there was created such issue: #2597

basically, the goal is to replace the integration tests of the API Server with the logic which does not require to start the API Server at all. Thanks to that we still testing the contract described in integration tests but such approach allows us to do that both for API Server and CRD implementation.

The first part was already migrated: #2580 (comment)

This is the second PR, and after merging that we will submit the 3rd PR - the final one which will move all rest tests (we have that PR already prepared)

about adding // +build integration tag, please ready this comment

jberkhahn · 2019-06-18T22:31:00Z

I'm having difficulties running the tests due to import conflicts from the migration, do you think you could rebase this on master?

jberkhahn · 2019-06-18T22:52:53Z

pkg/controller/case_test.go

 func (ct *controllerTest) CreateSimpleClusterServiceBroker() error {
 	_, err := ct.scInterface.ClusterServiceBrokers().Create(ct.fixClusterServiceBroker())
 	return err
 }

+// CreateClusterServiceBrokerWithBasicAuth creates a ClusterServiceBroker with basic auth.


Thanks for putting in comments as you go, even if you weren't touching these

jberkhahn · 2019-06-18T23:08:49Z

pkg/controller/case_test.go

@@ -308,8 +484,34 @@ func (ct *controllerTest) CreateServiceInstance() error {
 	return err
 }

-func (ct *controllerTest) DeleteServiceInstance() error {
-	return ct.scInterface.ServiceInstances(testNamespace).Delete(testServiceInstanceName, &v1.DeleteOptions{})
+func (ct *controllerTest) UpdateServiceInstanceParameters() error {


Add a comment for this one?

jberkhahn · 2019-06-18T23:19:48Z

pkg/controller/case_test.go

@@ -395,6 +634,22 @@ func (ct *controllerTest) WaitForNotReadyBinding() error {
 	})
 }



comments for all these too, if you could

jberkhahn · 2019-06-18T23:32:05Z

Took a look, it looks pretty good.

mszostok · 2019-06-24T09:54:37Z

/test pull-service-catalog-xbuild

mszostok · 2019-06-24T10:57:21Z

/lgtm

jberkhahn · 2019-06-25T03:10:41Z

/approve

k8s-ci-robot · 2019-06-25T03:10:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jberkhahn, piotrmiskiewicz

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~~ [jberkhahn]

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 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 Apr 4, 2019

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 4, 2019

k8s-ci-robot requested review from jboyd01 and MHBauer April 4, 2019 10:17

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

piotrmiskiewicz force-pushed the controller-test-deletion branch from a4547f2 to 9210c5f Compare April 4, 2019 10:33

piotrmiskiewicz changed the title ~~New controller tests (#2580)~~ New controller tests - deletion resources Apr 4, 2019

piotrmiskiewicz force-pushed the controller-test-deletion branch from 9210c5f to f4b8666 Compare April 4, 2019 10:41

piotrmiskiewicz mentioned this pull request Apr 4, 2019

Rewrite controller tests #2597

Closed

piotrmiskiewicz force-pushed the controller-test-deletion branch 3 times, most recently from b29ad9e to 3894618 Compare April 4, 2019 12:57

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 Apr 5, 2019

jasiu001 approved these changes Apr 10, 2019

View reviewed changes

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

piotrmiskiewicz force-pushed the controller-test-deletion branch 2 times, most recently from c8990ad to 4631811 Compare May 15, 2019 14:11

mszostok mentioned this pull request May 19, 2019

Replaces the Aggregated API Server with the CustomResourceDefinitions (CRDs) solution #2630

Merged

3 tasks

piotrmiskiewicz force-pushed the controller-test-deletion branch from 4631811 to 6c7df3e Compare June 12, 2019 09:46

mszostok suggested changes Jun 12, 2019

View reviewed changes

piotrmiskiewicz force-pushed the controller-test-deletion branch from 6c7df3e to f909efe Compare June 14, 2019 11:28

MHBauer reviewed Jun 14, 2019

View reviewed changes

jberkhahn reviewed Jun 18, 2019

View reviewed changes

piotrmiskiewicz and others added 3 commits June 20, 2019 22:31

New controller tests (kubernetes-retired#2580)

cd7d064

Fix fmt issues

27810fd

Add missing comments, add missing gitignore rule

e275ae9

mszostok force-pushed the controller-test-deletion branch from f909efe to e275ae9 Compare June 24, 2019 09:42

mszostok approved these changes Jun 24, 2019

View reviewed changes

k8s-ci-robot assigned mszostok Jun 24, 2019

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

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

k8s-ci-robot merged commit 6d7cea6 into kubernetes-retired:master Jun 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New controller tests - deletion resources #2596

New controller tests - deletion resources #2596

piotrmiskiewicz commented Apr 4, 2019 •

edited

Loading

k8s-ci-robot commented Apr 4, 2019

piotrmiskiewicz commented Apr 4, 2019

jberkhahn commented Apr 5, 2019

piotrmiskiewicz commented Apr 10, 2019

jberkhahn commented Apr 29, 2019

jberkhahn commented May 6, 2019

piotrmiskiewicz commented May 15, 2019

piotrmiskiewicz commented May 16, 2019

mszostok left a comment

mszostok commented Jun 13, 2019

piotrmiskiewicz commented Jun 14, 2019

MHBauer Jun 14, 2019

mszostok Jun 24, 2019

MHBauer Jun 14, 2019

mszostok Jun 24, 2019

jberkhahn commented Jun 18, 2019

jberkhahn Jun 18, 2019

jberkhahn Jun 18, 2019

jberkhahn Jun 18, 2019

jberkhahn commented Jun 18, 2019

mszostok commented Jun 24, 2019

mszostok commented Jun 24, 2019

jberkhahn commented Jun 25, 2019

k8s-ci-robot commented Jun 25, 2019

		@@ -395,6 +634,22 @@ func (ct *controllerTest) WaitForNotReadyBinding() error {
		})
		}

New controller tests - deletion resources #2596

New controller tests - deletion resources #2596

Conversation

piotrmiskiewicz commented Apr 4, 2019 • edited Loading

k8s-ci-robot commented Apr 4, 2019

piotrmiskiewicz commented Apr 4, 2019

jberkhahn commented Apr 5, 2019

piotrmiskiewicz commented Apr 10, 2019

jberkhahn commented Apr 29, 2019

jberkhahn commented May 6, 2019

piotrmiskiewicz commented May 15, 2019

piotrmiskiewicz commented May 16, 2019

mszostok left a comment

Choose a reason for hiding this comment

mszostok commented Jun 13, 2019

piotrmiskiewicz commented Jun 14, 2019

MHBauer Jun 14, 2019

Choose a reason for hiding this comment

mszostok Jun 24, 2019

Choose a reason for hiding this comment

MHBauer Jun 14, 2019

Choose a reason for hiding this comment

mszostok Jun 24, 2019

Choose a reason for hiding this comment

jberkhahn commented Jun 18, 2019

jberkhahn Jun 18, 2019

Choose a reason for hiding this comment

jberkhahn Jun 18, 2019

Choose a reason for hiding this comment

jberkhahn Jun 18, 2019

Choose a reason for hiding this comment

jberkhahn commented Jun 18, 2019

mszostok commented Jun 24, 2019

mszostok commented Jun 24, 2019

jberkhahn commented Jun 25, 2019

k8s-ci-robot commented Jun 25, 2019

piotrmiskiewicz commented Apr 4, 2019 •

edited

Loading