Skip to content
This repository has been archived by the owner on May 6, 2022. It is now read-only.

Add ginkgo tests for svcat get classes #2230

Merged
merged 3 commits into from
Jul 30, 2018
Merged

Add ginkgo tests for svcat get classes #2230

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a1760d6
Add ginkgo tests for svcat get classes
carolynvs-msft Jul 24, 2018
aca8386
Update generated code
carolynvs-msft Jul 24, 2018
3831bd4
Test the Run method instead of getAll
carolynvs-msft Jul 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion cmd/svcat/class/get_cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,11 @@ func NewGetCmd(cxt *command.Context) *cobra.Command {
cmd := &cobra.Command{
Use: "classes [NAME]",
Aliases: []string{"class", "cl"},
Short: "List classes, optionally filtered by name",
Short: "List classes, optionally filtered by name, scope or namespace",
Example: command.NormalizeExamples(`
svcat get classes
svcat get classes --scope cluster
svcat get classes --scope namespace --namespace dev
svcat get class mysqldb
svcat get class --uuid 997b8372-8dac-40ac-ae65-758b4a5075a5
`),
Expand Down
126 changes: 126 additions & 0 deletions cmd/svcat/class/get_cmd_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ import (
svcatfake "github.com/kubernetes-incubator/service-catalog/pkg/client/clientset_generated/clientset/fake"
"github.com/kubernetes-incubator/service-catalog/pkg/svcat"
"github.com/kubernetes-incubator/service-catalog/pkg/svcat/service-catalog"
"github.com/kubernetes-incubator/service-catalog/pkg/svcat/service-catalog/service-catalogfakes"
. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
k8sfake "k8s.io/client-go/kubernetes/fake"
Expand Down Expand Up @@ -174,3 +177,126 @@ func TestListClasses(t *testing.T) {
})
}
}

var _ = Describe("Get Classes Command", func() {
Describe("NewGetClassesCmd", func() {
It("Builds and returns a cobra command", func() {
cxt := &command.Context{}
cmd := NewGetCmd(cxt)
Expect(*cmd).NotTo(BeNil())
Expect(cmd.Use).To(Equal("classes [NAME]"))
Expect(cmd.Short).To(ContainSubstring("List classes, optionally filtered by name, scope or namespace"))
Expect(cmd.Example).To(ContainSubstring("svcat get classes"))
Expect(cmd.Example).To(ContainSubstring("svcat get classes --scope cluster"))
Expect(cmd.Example).To(ContainSubstring("svcat get classes --scope namespace --namespace dev"))
Expect(len(cmd.Aliases)).To(Equal(2))
})
})
Describe("Validate", func() {
It("allows class name arg to be empty", func() {
cmd := &getCmd{}
err := cmd.Validate([]string{})
Expect(err).To(BeNil())
})
It("optionally parses the class name argument", func() {
cmd := &getCmd{}
err := cmd.Validate([]string{"mysqldb"})
Expect(err).To(BeNil())
Expect(cmd.name).To(Equal("mysqldb"))
})
})
Describe("getAll", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

generally you don't have to test private methods, as they should be subsumed in whatever public methods use them. I would change this to test Run(), and control the different cases by modifying whether you pass in a uuid or name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's wrong with a unit test testing a single function?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's not the testing a single function, it's that it's a private function - if the content of getAll was just inlined in Run, it wouldn't make sense to try to test the functionality of that block specifically, and because it's private, it kinda is just inlined.

This wouldn't be a big change, just switch the describe to "Run", and change the calls to to getAll with calls to Run with the appropriate fields filled in.

Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment to the test about not worrying about it if it breaks. I'm not hung up on idealogical test purity that says you only test public methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why would we add a test that we don't care if it breaks? And you do test private methods, but implicitly. A test for Run(), which I think we should definitely have, would be 99% the same lines of code as the tests she's written here for getAll() and a hypothetical test for get()

It("Calls the pkg/svcat libs RetrieveClasses with namespace scope and current namespace", func() {
outputBuffer := &bytes.Buffer{}

fakeApp, _ := svcat.NewApp(nil, nil, "default")
fakeSDK := new(servicecatalogfakes.FakeSvcatClient)
fakeSDK.RetrieveClassesReturns(
[]servicecatalog.Class{&v1beta1.ServiceClass{ObjectMeta: metav1.ObjectMeta{Name: "mysqldb", Namespace: "default"}}},
nil)
fakeApp.SvcatClient = fakeSDK
cmd := getCmd{
Namespaced: &command.Namespaced{Context: svcattest.NewContext(outputBuffer, fakeApp)},
Scoped: command.NewScoped(),
}
cmd.Namespace = "default"
cmd.Scope = servicecatalog.NamespaceScope

err := cmd.getAll()

Expect(err).NotTo(HaveOccurred())
scopeArg := fakeSDK.RetrieveClassesArgsForCall(0)
Expect(scopeArg).To(Equal(servicecatalog.ScopeOptions{
Namespace: "default",
Scope: servicecatalog.NamespaceScope,
}))

output := outputBuffer.String()
Expect(output).To(ContainSubstring("mysqldb"))
})
It("Calls the pkg/svcat libs RetrieveClasses with namespace scope and all namespaces", func() {
outputBuffer := &bytes.Buffer{}

fakeApp, _ := svcat.NewApp(nil, nil, "default")
fakeSDK := new(servicecatalogfakes.FakeSvcatClient)
fakeSDK.RetrieveClassesReturns(
[]servicecatalog.Class{
&v1beta1.ServiceClass{ObjectMeta: metav1.ObjectMeta{Name: "mysqldb", Namespace: "default"}},
&v1beta1.ServiceClass{ObjectMeta: metav1.ObjectMeta{Name: "postgresdb", Namespace: "test-ns"}},
},
nil)
fakeApp.SvcatClient = fakeSDK
cmd := getCmd{
Namespaced: &command.Namespaced{Context: svcattest.NewContext(outputBuffer, fakeApp)},
Scoped: command.NewScoped(),
}
cmd.Namespace = ""
cmd.Scope = servicecatalog.NamespaceScope

err := cmd.getAll()

Expect(err).NotTo(HaveOccurred())
scopeArg := fakeSDK.RetrieveClassesArgsForCall(0)
Expect(scopeArg).To(Equal(servicecatalog.ScopeOptions{
Namespace: "",
Scope: servicecatalog.NamespaceScope,
}))

output := outputBuffer.String()
Expect(output).To(ContainSubstring("mysqldb"))
Expect(output).To(ContainSubstring("postgresdb"))
})
It("Calls the pkg/svcat libs RetrieveClasses with all scope and current namespaces", func() {
outputBuffer := &bytes.Buffer{}

fakeApp, _ := svcat.NewApp(nil, nil, "default")
fakeSDK := new(servicecatalogfakes.FakeSvcatClient)
fakeSDK.RetrieveClassesReturns(
[]servicecatalog.Class{
&v1beta1.ClusterServiceClass{ObjectMeta: metav1.ObjectMeta{Name: "mysqldb"}},
&v1beta1.ServiceClass{ObjectMeta: metav1.ObjectMeta{Name: "postgresdb", Namespace: "default"}},
},
nil)
fakeApp.SvcatClient = fakeSDK
cmd := getCmd{
Namespaced: &command.Namespaced{Context: svcattest.NewContext(outputBuffer, fakeApp)},
Scoped: command.NewScoped(),
}
cmd.Namespace = "default"
cmd.Scope = servicecatalog.AllScope

err := cmd.getAll()

Expect(err).NotTo(HaveOccurred())
scopeArg := fakeSDK.RetrieveClassesArgsForCall(0)
Expect(scopeArg).To(Equal(servicecatalog.ScopeOptions{
Namespace: "default",
Scope: servicecatalog.AllScope,
}))

output := outputBuffer.String()
Expect(output).To(ContainSubstring("mysqldb"))
Expect(output).To(ContainSubstring("postgresdb"))
})
})
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Would also be nice to have a set of cases for when you're just getting a single class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is the next PR. This change was only for getting a list of classes. 😀

Copy link
Contributor

Choose a reason for hiding this comment

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

👍