Add controller Service Instance test #2667
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Hi @jasiu001. Thanks for your PR. I'm waiting for a kubernetes-sigs or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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
added
the
size/XXL
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
/assign @jberkhahn |
|
/test pull-service-catalog-integration |
|
/retest |
|
|
||
| // WaitForInstanceCondition waits until ServiceInstance `status.conditions` field value is equal to condition in parameters | ||
| // returns error if the time limit has been reached | ||
| func (ct *controllerTest) WaitForInstanceCondition(condition v1beta1.ServiceInstanceCondition) error { |
There was a problem hiding this comment.
Is there a reason you duplicated this instead of using the one in test/util/util.go?
| // WaitForServiceInstanceProcessedGeneration waits until ServiceInstance parameter `Status.ObservedGeneration` is | ||
| // equal or higher than ServiceInstance `generation` value, ServiceInstance is in Ready/True status and | ||
| // ServiceInstance is not in Orphan Mitigation progress | ||
| func (ct *controllerTest) WaitForServiceInstanceProcessedGeneration(generation int64) error { |
| return err | ||
| } | ||
|
|
||
| func isServiceInstanceConditionTrue(instance *v1beta1.ServiceInstance) bool { |
| } | ||
|
|
||
| // AssertServiceInstanceHasNoCondition makes sure ServiceInstance is in not specific condition | ||
| func (ct *controllerTest) AssertServiceInstanceHasNoCondition(t *testing.T, cond v1beta1.ServiceInstanceCondition) { |
There was a problem hiding this comment.
would it make sense to move this to the util package?
| } | ||
|
|
||
| // AssertServiceInstanceOrphanMitigationStatus makes sure ServiceInstance is/or is not in Orphan Mitigation progress | ||
| func (ct *controllerTest) AssertServiceInstanceOrphanMitigationStatus(t *testing.T, state bool) { |
There was a problem hiding this comment.
seems like another thing to stick in the util package
| } | ||
|
|
||
| // CreateClusterServiceClass creates ClusterServiceClass with default parameters | ||
| func (ct *controllerTest) CreateClusterServiceClass() error { |
There was a problem hiding this comment.
Pretty much all of this might want to be moved into test/util. I could have sworn there was an equivalent for this in there already but I can't find it offhand.
|
|
||
| // TestCreateServiceInstanceFailsWithNonexistentPlan tests creating a ServiceInstance whose ServicePlan | ||
| // does not exist | ||
| func TestCreateServiceInstanceFailsWithNonexistentPlan(t *testing.T) { |
|
The tests themselves seem good, but I think there's quite a bit of refactoring of the setup stuff that could be done. A lot are dupes of functions we already have in the test/util package. |
|
I noticed that all comments are about why I don't use existing functions in As you can see in this commit which is already merged to master "utils" functions are moved to In the end, I would like to point out that in "our" |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jasiu001, jberkhahn 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 |
k8s-ci-robot
added
the
approved
|
@MHBauer In general here is the issue about that: To make a long story short, current integration tests are tightly coupled with api-server (starting server, etcd). We want to rewrite them into general integration tests with only one dependency to SC controllers. Right now, the rewritten tests are placed are under
the test name is exactly the same as defined in https://github.com/kubernetes-sigs/service-catalog/tree/master/test/integration, so it's easy to check them e.g. when you execute search: then you will see that test is defined in both places:
they are testing the same logic but the new way does not require the api-server right now we are in the transition phase and we run both tests (this is also the reason why |
There was a problem hiding this comment.
This is the last PR for #2597 issue
Why those 6 tests were not migrated?
- TestBasicFlowsWithOriginatingIdentity,
- TestCreateServiceBindingInstanceNotReady,
- TestCreateServiceBindingInvalidInstanceFailure,
- TestCreateServiceBindingNonBindable,
- TestCreateServiceBindingWithParameters,
- TestCreateServiceBindingWithSecretTransform
| // TestCreateServiceInstanceNonExistentClusterServiceClassOrPlan tests that a ServiceInstance gets | ||
| // a Failed condition when the service class or service plan it references does not exist. | ||
| func TestCreateServiceInstanceNonExistentClusterServiceClassOrPlan(t *testing.T) { | ||
| // TODO: cannot rewrite tests because fakeClient does not support `FieldSelector` during filtering ServiceInstance list |
There was a problem hiding this comment.
use t.Skip("message.."), thanks to that when this test will be executed we will see the information that it was skipped with given reason. Add also info that it should be added after merging the CRDs
|
@mszostok #2667 (comment) I didn't know about or forgot about the integration tag. Had a nil pointer dereference when running that way. Not 100% sure if those are exactly the tests responsible or not. |
|
What I presume is a flaky fail: I'll have to remember how to turn the logging up if you want me to try and repro for more info. Something doesn't seem quite ready if I'm hitting 2 issues within 3 runs. If not aware, my recommendation is to turn the run count up with Could be this is some cleanup not happening and exhausting the resources, but I haven't looked. |
|
Also figuring out how to run some tests or subtests in parallel would be good. |
|
@MHBauer considering your comment about parallel tests I paralleled some tests. The list of tests and their execution times before and after are presented in the table below:
When it comes to the I also tried to reproduce failing @mszostok missing tests have been added |
|
I forgot about feature gates. I remember those preventing nearly any use of parallel. Oh well. |
There was a problem hiding this comment.
parallel execution makes a difference 👍 I've executed those tests locally without any failures
just few minor comments :)
@MHBauer is everything ok from your side? if yes then we will merge it on Monday thanks to that we will be able to rebase the CRD branch :)
pkg/controller/case_test.go
Outdated
| } | ||
| } | ||
|
|
||
| // TODO: move to case_test.go |
There was a problem hiding this comment.
should be removed, right?
pkg/controller/case_test.go
Outdated
| return err | ||
| } | ||
|
|
||
| // TODO: move to case_test.go |
pkg/controller/case_test.go
Outdated
| } | ||
| } | ||
|
|
||
| // TODO: move to case_test.go |
| @@ -59,6 +59,39 @@ func TestBasicFlowWithBasicAuth(t *testing.T) { | |||
| ct.AssertOSBBasicAuth(t, "user1", "newp2sswd") | |||
| } | |||
|
|
|||
| // TestOriginatingIdentity tests whether the controller uses correct credentials when the secret changes | |||
| func TestOriginatingIdentity(t *testing.T) { | |||
There was a problem hiding this comment.
add info
// CAUTION: the test cannot be executed in parallel because it changes global flag which can affect the behavior of other tests
|
Is there a 1second sleep in here somewhere? A lot of tests take that suspiciously specific amount of time. |
|
here is exact invocation: $ go test --tags="integration" -v -run TestCreateServiceInstanceWithInvalidParameters -race -count 30 -failfast
=== RUN TestCreateServiceInstanceWithInvalidParameters
=== PAUSE TestCreateServiceInstanceWithInvalidParameters
=== CONT TestCreateServiceInstanceWithInvalidParameters
--- PASS: TestCreateServiceInstanceWithInvalidParameters (1.01s)
=== RUN TestCreateServiceInstanceWithInvalidParameters
=== PAUSE TestCreateServiceInstanceWithInvalidParameters
=== CONT TestCreateServiceInstanceWithInvalidParameters
--- PASS: TestCreateServiceInstanceWithInvalidParameters (1.01s)
=== RUN TestCreateServiceInstanceWithInvalidParameters
=== PAUSE TestCreateServiceInstanceWithInvalidParameters
=== CONT TestCreateServiceInstanceWithInvalidParameters
--- PASS: TestCreateServiceInstanceWithInvalidParameters (1.01s)
=== RUN TestCreateServiceInstanceWithInvalidParameters
=== PAUSE TestCreateServiceInstanceWithInvalidParameters
=== CONT TestCreateServiceInstanceWithInvalidParameters
--- FAIL: TestCreateServiceInstanceWithInvalidParameters (8.97s)
require.go:794:
Error Trace: controller_flow_instance_test.go:252
Error: Received unexpected error:
instance with proper conditions not found, the existing conditions: [{Type:Ready Status:True LastTransitionTime:2019-09-03 14:50:00.909996568 -0700 PDT m=+4.054543438 Reason:ProvisionedSuccessfully Message:The instance was provisioned successfully}]
Test: TestCreateServiceInstanceWithInvalidParameters
FAIL
exit status 1
FAIL github.com/kubernetes-sigs/service-catalog/pkg/controller 12.067sDo instances of the same test interfere with each other? Are we not cleaning up after ourselves? |
|
As a separate task, there's some very bizarre imports. Things imported multiple times with different names. |
jasiu001
force-pushed
the
controller-test
branch
from
c08aa6c
to
24acd3d
Compare
|
There is no As for the test You are right about there were imports with the same library, I found two in
I fixed it. |
|
/test pull-build-all-images-for-ppc64le |
|
Awesome. I'll give it another soak locally. |
|
There's definitely a goroutine lack-of-cleanup threadsplosion issue, but single runs appear to be okay. |
|
I think, this is I'll let someone else confirm. Remove the hold when it's ready. I've created #2698 as an issue to look into. |
k8s-ci-robot
added
do-not-merge/hold
|
ship it |
k8s-ci-robot
removed
the
do-not-merge/hold
This PR is a
What this PR does / why we need it:
The PR indroduces controller tests which will replace current integration tests, see #2597
Test refers to the ServiceInstance flow:
Please leave this checklist in the PR comment so that maintainers can ensure a good PR.
Merge Checklist:
breaking the chart release and existing clients who provide a
flag that will get an error when they try to update