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

Add Unbind tests #31

Merged
merged 1 commit into from
Sep 22, 2018
Merged

Add Unbind tests #31

merged 1 commit into from
Sep 22, 2018

Conversation

vsomayaji
Copy link
Contributor

Overview

Adds tests for Unbind, increasing coverage to 92.9%.

Notes

--- PASS: TestUnbind (0.00s)
    --- PASS: TestUnbind/error_getting_binding (0.00s)
    --- PASS: TestUnbind/binding_not_found (0.00s)
    --- PASS: TestUnbind/success (0.00s)
    --- PASS: TestUnbind/error_getting_instance (0.00s)
    --- PASS: TestUnbind/instance_not_found (0.00s)
    --- PASS: TestUnbind/error_detaching_role_policy (0.00s)
    --- PASS: TestUnbind/detach_role_policy (0.00s)
    --- PASS: TestUnbind/role_not_found (0.00s)

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@vsomayaji vsomayaji requested a review from jaymccon as a code owner September 22, 2018 01:42
@@ -108,11 +108,46 @@ func (db mockDataStoreProvision) GetServiceBinding(id string) (*serviceinstance.
switch id {
case "err":
return nil, errors.New("test failure")
case "err-instance":
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the simplest way to set up the different test cases, but I think it'd be cleaner long-term to make the changes required to use a dynamodbiface.DynamoDBAPI instead of the mockDataStoreProvision. Something for a future PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

The thought behind the extra layer of abstraction with the datastore interface was to make it easy to implement alternative persistent storage options like etcd. Not sure if it really does make it that much easier, or if it's something that we'll ever do, so I'm not opposed to getting rid of it.

Copy link
Contributor

@jaymccon jaymccon left a comment

Choose a reason for hiding this comment

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

LGTM

@jaymccon jaymccon merged commit e8f37a4 into awslabs:master Sep 22, 2018
knqyf263 pushed a commit to knqyf263/aws-servicebroker that referenced this pull request Oct 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants