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.

Sign up for GitHub

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

[DRAFT] Add unit level tests for App repository #887

Closed
wants to merge 2 commits into from
Closed

[DRAFT] Add unit level tests for App repository #887

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7965d11
Add unit level tests for App repository
matt-royal Mar 29, 2022
85a87b8
Integrate Kieron's feedback
matt-royal Mar 31, 2022
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: 2 additions & 2 deletions api/apis/integration/integration_suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ var (
rootNamespaceUserRole *rbacv1.ClusterRole
rootNamespace string
clientFactory repositories.UserK8sClientFactory
nsPermissions *authorization.NamespacePermissions
nsPermissions repositories.NamespacePermissions
)

var _ = BeforeSuite(func() {
Expand Down Expand Up @@ -105,7 +105,7 @@ var _ = BeforeSuite(func() {
dynamicClient, err := dynamic.NewForConfig(k8sConfig)
Expect(err).NotTo(HaveOccurred())
Expect(dynamicClient).NotTo(BeNil())
namespaceRetriever = repositories.NewNamespaceRetriver(dynamicClient)
namespaceRetriever = repositories.NewNamespaceRetriever(dynamicClient)
Expect(namespaceRetriever).NotTo(BeNil())

rand.Seed(time.Now().UnixNano())
Expand Down
14 changes: 7 additions & 7 deletions api/authorization/namespace_permissions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,29 +24,29 @@ type IdentityProvider interface {
GetIdentity(context.Context, Info) (Identity, error)
}

type NamespacePermissions struct {
type namespacePermissions struct {
privilegedClient client.Client
identityProvider IdentityProvider
rootNamespace string
}

func NewNamespacePermissions(privilegedClient client.Client, identityProvider IdentityProvider, rootNamespace string) *NamespacePermissions {
return &NamespacePermissions{
func NewNamespacePermissions(privilegedClient client.Client, identityProvider IdentityProvider, rootNamespace string) *namespacePermissions {
Copy link
Contributor

Choose a reason for hiding this comment

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

Returning a private struct is interesting. Maybe we should do more of this.

Copy link
Member

Choose a reason for hiding this comment

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

I've been wanting to do this in the handlers as well. Having the type be private means that you can't instantiate it incorrectly outside of the constructor.

Copy link
Member

@georgethebeatle georgethebeatle Mar 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I have always liked this approach, but have avoided it for years, because it used to yield a golint linter warning. I tried to find out one more time why exactly returning an unexported type is "annoying" and found this issue. However I am not convinced that it is evil if you return an interface implementation. So I think I agree with you, just putting these references here for additional context.

Copy link
Member

Choose a reason for hiding this comment

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

I think I found a valid use case for the annoyance argument. Imagine the following code

package foo

type thing struct {...}

func NewThing() *thing{....}

You need to testdrive it, so you create a test package (foo_test):

package foo_test
....

var myThing ???? // how do you var the thing to test so that you can use it in `BeforeEach`, etc?

Remember that the golang convention is that interfaces are defined wherever needed, not where they are implemented, therefore package foo might not have the Thing interface at all.

In this particular case, imagine you had to write unit tests for the namespacePermissions struct. How would you do that?

Copy link
Member

@matt-royal matt-royal Mar 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

In the extreme case, you can create the interface in the test file itself, since that's where it's used. However, in our codebase I think it would be reasonable to use the expected interface type in the test, since that's how the code will actually be used in practice. In this case that would mean var nsPerm repositories.NamespacePermission. If that feels awkward since the interface is declared in a different package, then we could move the interfaces into a shared package that contains common interfaces used throughout the codebase.

Personally, I prefer this approach to having to worry about objects being constructed incorrectly.

Copy link
Member

@danail-branekov danail-branekov Apr 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Personally, I prefer this approach to having to worry about objects being constructed incorrectly.

Absolutely agree! We should be using constructors to create objects and make all objects' field private to guarantee that, there is no argument about it.

What I doubt is whether making structs private (i.e. make their names start with a lowercase letter) and returning pointers to those structs be annoying for testing. For example

func NewThing() *Thing

vs

func NewThing() *thing

you can create the interface in the test file itself
Feels like a workaround for the sake of testing to me. What about cases when a struct implements a couple of interfaces? Would you need the "productive" interfaces defined wherever they are needed and a test interface that combines them all?

then we could move the interfaces into a shared package that contains common interfaces used throughout the codebase.
I do not think that this is quite golang idiomatic though.

return &namespacePermissions{
privilegedClient: privilegedClient,
identityProvider: identityProvider,
rootNamespace: rootNamespace,
}
}

func (o *NamespacePermissions) GetAuthorizedOrgNamespaces(ctx context.Context, info Info) (map[string]bool, error) {
func (o *namespacePermissions) GetAuthorizedOrgNamespaces(ctx context.Context, info Info) (map[string]bool, error) {
return o.getAuthorizedNamespaces(ctx, info, orgLevel, "Org")
}

func (o *NamespacePermissions) GetAuthorizedSpaceNamespaces(ctx context.Context, info Info) (map[string]bool, error) {
func (o *namespacePermissions) GetAuthorizedSpaceNamespaces(ctx context.Context, info Info) (map[string]bool, error) {
return o.getAuthorizedNamespaces(ctx, info, spaceLevel, "Space")
}

func (o *NamespacePermissions) getAuthorizedNamespaces(ctx context.Context, info Info, orgSpaceLevel, resourceType string) (map[string]bool, error) {
func (o *namespacePermissions) getAuthorizedNamespaces(ctx context.Context, info Info, orgSpaceLevel, resourceType string) (map[string]bool, error) {
identity, err := o.identityProvider.GetIdentity(ctx, info)
if err != nil {
return nil, fmt.Errorf("failed to get identity: %w", err)
Expand Down Expand Up @@ -84,7 +84,7 @@ func (o *NamespacePermissions) getAuthorizedNamespaces(ctx context.Context, info
return authorizedNamespaces, nil
}

func (o *NamespacePermissions) AuthorizedIn(ctx context.Context, identity Identity, namespace string) (bool, error) {
func (o *namespacePermissions) AuthorizedIn(ctx context.Context, identity Identity, namespace string) (bool, error) {
var rolebindings rbacv1.RoleBindingList
err := o.privilegedClient.List(ctx, &rolebindings, client.InNamespace(namespace))
if err != nil {
Expand Down
3 changes: 2 additions & 1 deletion api/authorization/namespace_permissions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package authorization_test

import (
"code.cloudfoundry.org/cf-k8s-controllers/api/repositories"
"context"
"errors"
"fmt"
Expand All @@ -20,7 +21,7 @@ import (
var _ = Describe("Namespace Permissions", func() {
var (
ctx context.Context
nsPerms *authorization.NamespacePermissions
nsPerms repositories.NamespacePermissions
namespaces map[string]bool
getErr error
authInfo authorization.Info
Expand Down
2 changes: 1 addition & 1 deletion api/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ func main() {
if err != nil {
panic(fmt.Sprintf("could not create dynamic k8s client: %v", err))
}
namespaceRetriever := repositories.NewNamespaceRetriver(dynamicClient)
namespaceRetriever := repositories.NewNamespaceRetriever(dynamicClient)

mapper, err := apiutil.NewDynamicRESTMapper(k8sClientConfig)
if err != nil {
Expand Down
8 changes: 4 additions & 4 deletions api/repositories/app_repository.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,10 @@ const (
type AppRepo struct {
namespaceRetriever NamespaceRetriever
userClientFactory UserK8sClientFactory
namespacePermissions *authorization.NamespacePermissions
namespacePermissions NamespacePermissions
}

func NewAppRepo(namespaceRetriever NamespaceRetriever, userClientFactory UserK8sClientFactory, authPerms *authorization.NamespacePermissions) *AppRepo {
func NewAppRepo(namespaceRetriever NamespaceRetriever, userClientFactory UserK8sClientFactory, authPerms NamespacePermissions) *AppRepo {
return &AppRepo{
namespaceRetriever: namespaceRetriever,
userClientFactory: userClientFactory,
Expand Down Expand Up @@ -160,7 +160,7 @@ func (f *AppRepo) GetApp(ctx context.Context, authInfo authorization.Info, appGU

userClient, err := f.userClientFactory.BuildClient(authInfo)
if err != nil {
return AppRecord{}, fmt.Errorf("get-app failed to build user client: %w", err)
return AppRecord{}, fmt.Errorf("failed to build user client: %w", err)
}

app := workloadsv1alpha1.CFApp{}
Expand Down Expand Up @@ -395,7 +395,7 @@ func (f *AppRepo) CreateOrPatchAppEnvVars(ctx context.Context, authInfo authoriz
func (f *AppRepo) SetCurrentDroplet(ctx context.Context, authInfo authorization.Info, message SetCurrentDropletMessage) (CurrentDropletRecord, error) {
userClient, err := f.userClientFactory.BuildClient(authInfo)
if err != nil {
return CurrentDropletRecord{}, fmt.Errorf("set-current-droplet: failed to create k8s user client: %w", err)
return CurrentDropletRecord{}, fmt.Errorf("failed to build user client: %w", err)
}

baseCFApp := &workloadsv1alpha1.CFApp{
Expand Down
Loading