Proposal For Use Of Interfaces

[manedurphy]

Proposal For Use Of Interfaces

Context

Greetings,

Our development team utilizes the gocb package for our platform. When we use external packages such as this, we tend to wrap them in an abstraction that makes our application code easier to understand. Take the snippet below, for example; we are creating our own wrapper package called store. The application only has to understand how the store package works, and doesn't need to know that behind the scenes it uses gocb. This makes it very easy to understand the application code since we are the ones defining those methods.

application

package main

func main() {
    s, err := store.NewStore()
    if err != nil {
        panic(err)
    }

    doesPlatformConfigurationExist, err := s.DoesPlatformConfigurationExist()
    if err != nil {
        panic(err)
    }

    // It is very easy to understand that this code is checking if the platform configuration document
    // exists in the store
    if !doesPlatformConfigurationExist {
        // Handle case for when the platform configuration document does not exist
    }
}

wrapper

package store

import "github.com/couchbase/gocb/v2"

type Store interface {
    DoesPlatformConfigurationExist() (bool, error)
}

type store struct {
    collection *gocb.Collection
}

func NewStore() (Store, error) {
    var store store

    // We can pretend that there is some boilerplate code here for initializing the *gocb.Collection object

    return &store, nil
}

func (s *store) DoesPlatformConfigurationExist() (bool, error) {
    var (
        res *gocb.ExistsResult
        err error
    )

    res, err = s.collection.Exists("platform_configuration", nil)
    if err != nil {
        return false, err
    }

    return res.Exists(), nil
}

Testing

We do not make wrappers solely for comprehensibility, but also for testing. Any code that uses the store package will be easy to write unit tests for because the Store type is an interface. In our unit tests, we can implement our own mockStore type that implements the Store interface.

The issue we are facing is not that we can't write tests at the application level, which consumes the store package, but rather that we cannot test the store package itself without creating a true connection to a Couchbase instance or cluster. We want to be able to run our unit tests in a CI pipeline where we are not able to connect to Couchbase.

The best way that we know how to address this is for the types in gocb to be interfaces instead of structs so that we can manipulate the functionality of the public methods directly in our unit tests. Let's look at the following examples:

Structs

We can see that with the structs approach, we are unable to test every code path. In order to test the cases where the document does and doesn't exist, we have to write additional code in the unit tests to write and remove the expected document from Couchbase. And we still are unable to test the case where an error has occurred because we cannot force Couchbase to return an error.

store.go

package store

import (
    "os"
    "time"

    "github.com/couchbase/gocb/v2"
)

type Store interface {
    DoesImportantDocumentExist() (bool, error)
}

type store struct {
    collection *gocb.Collection
}

func NewStore() (Store, error) {
    var (
        cluster *gocb.Cluster
        err     error
    )

    cluster, err = gocb.Connect("couchbase://127.0.0.1", gocb.ClusterOptions{
        Authenticator: gocb.PasswordAuthenticator{
            Username: os.Getenv("COUCHBASE_USERNAME"),
            Password: os.Getenv("COUCHBASE_PASSWORD"),
        },
        TimeoutsConfig: gocb.TimeoutsConfig{
            ConnectTimeout: 5 * time.Second,
        },
    })
    if err != nil {
        return nil, err
    }

    return &store{
        collection: cluster.Bucket("demo").DefaultCollection(),
    }, nil
}

func (s *store) DoesImportantDocumentExist() (bool, error) {
    var (
        res *gocb.ExistsResult
        err error
    )

    res, err = s.collection.Exists("important_document", nil)
    if err != nil {
        return false, err
    }

    return res.Exists(), nil
}

store_test.go

package store

import (
    "os"
    "testing"
    "time"

    "github.com/couchbase/gocb/v2"
    "github.com/stretchr/testify/assert"
)

var (
    collection *gocb.Collection
    s          Store
)

func init() {
    var (
        cluster *gocb.Cluster
        err     error
    )

    s, err = NewStore()
    if err != nil {
        panic(err)
    }

    cluster, err = gocb.Connect("couchbase://127.0.0.1", gocb.ClusterOptions{
        Authenticator: gocb.PasswordAuthenticator{
            Username: os.Getenv("COUCHBASE_USERNAME"),
            Password: os.Getenv("COUCHBASE_PASSWORD"),
        },
        TimeoutsConfig: gocb.TimeoutsConfig{
            ConnectTimeout: 5 * time.Second,
        },
    })
    if err != nil {
        panic(err)
    }

    collection = cluster.Bucket("demo").DefaultCollection()
    s = &store{
        collection: collection,
    }
}

func TestExists(t *testing.T) {
    tests := []struct {
        name        string
        errExpected bool
        doesExist   bool
    }{
        {
            name:        "Document exists",
            errExpected: false,
            doesExist:   true,
        },
        {
            name:        "Document does not exist",
            errExpected: false,
            doesExist:   false,
        },
    }

    for _, test := range tests {
        t.Run(test.name, func(t *testing.T) {
            var (
                documentExists bool
                err            error
            )

            if test.doesExist {
                _, err = collection.Upsert("important_document", "some value", nil)
                if err != nil {
                    t.Fatal(err)
                }

                documentExists, err = s.DoesImportantDocumentExist()
                if err != nil {
                    t.Fatal(err)
                }

                assert.True(t, documentExists, "document should exist")
                return
            }

            _, err = collection.Remove("important_document", nil)
            if err != nil {
                t.Fatal(err)
            }

            documentExists, err = s.DoesImportantDocumentExist()
            if err != nil {
                t.Fatal(err)
            }

            assert.False(t, documentExists, "document should not exist")
        })
    }
}

Interfaces

We can see that with the interfaces approach, it is easy to create our own mock version of a collection. Another change to note is that the *gocb.ExistsResult type has been changed from a struct pointer to an interface in the gocb package. By using an interface, gocb.ExistsResult, we can create our own types which implement that interface in our unit tests, allowing us to test every code path in our wrapper as well as run these unit tests in a pipeline without the need for a connection to a Couchbase instance.

store.go

package store

import (
    "os"
    "time"

    "github.com/couchbase/gocb/v2"
)

type Store interface {
    DoesImportantDocumentExist() (bool, error)
}

type collection interface {
    Exists(id string, opts *gocb.ExistsOptions) (docOut gocb.ExistsResult, errOut error)
}

type store struct {
    collection collection
}

func NewStore() (Store, error) {
    cluster, err := gocb.Connect("couchbase://127.0.0.1", gocb.ClusterOptions{
        Authenticator: gocb.PasswordAuthenticator{
            Username: os.Getenv("COUCHBASE_USERNAME"),
            Password: os.Getenv("COUCHBASE_PASSWORD"),
        },
        TimeoutsConfig: gocb.TimeoutsConfig{
            ConnectTimeout: 5 * time.Second,
        },
    })
    if err != nil {
        return nil, err
    }

    return &store{
        collection: cluster.Bucket("demo").DefaultCollection(),
    }, nil
}

func (s *store) DoesImportantDocumentExist() (bool, error) {
    var (
        res gocb.ExistsResult
        err error
    )

    res, err = s.collection.Exists("important_document", nil)
    if err != nil {
        return false, err
    }

    return res.Exists(), nil
}

store_test.go

package store

import (
    "errors"
    "testing"

    "github.com/couchbase/gocb/v2"
    "github.com/stretchr/testify/assert"
)

type mockCollection struct {
    existsFunc func(id string, opts *gocb.ExistsOptions) (docOut gocb.ExistsResult, errOut error)
}

func (m *mockCollection) Exists(id string, opts *gocb.ExistsOptions) (docOut gocb.ExistsResult, errOut error) {
    return m.existsFunc(id, opts)
}

type mockExistsResult struct {
    existsFunc func() bool
    resultFunc func() gocb.Result
}

func (m mockExistsResult) Exists() bool {
    return m.existsFunc()
}

func (m mockExistsResult) Result() gocb.Result {
    return m.resultFunc()
}

func TestExists(t *testing.T) {
    tests := []struct {
        name        string
        errExpected bool
        doesExist   bool
        collection  mockCollection
    }{
        {
            name:        "Document exists",
            errExpected: false,
            doesExist:   true,
            collection: mockCollection{
                existsFunc: func(id string, opts *gocb.ExistsOptions) (docOut gocb.ExistsResult, errOut error) {
                    return mockExistsResult{
                        existsFunc: func() bool {
                            return true
                        },
                    }, nil
                },
            },
        },
        {
            name:        "Document does not exist",
            errExpected: false,
            doesExist:   false,
            collection: mockCollection{
                existsFunc: func(id string, opts *gocb.ExistsOptions) (docOut gocb.ExistsResult, errOut error) {
                    return mockExistsResult{
                        existsFunc: func() bool {
                            return false
                        },
                    }, nil
                },
            },
        },
        {
            name:        "Error",
            errExpected: false,
            doesExist:   false,
            collection: mockCollection{
                existsFunc: func(id string, opts *gocb.ExistsOptions) (docOut gocb.ExistsResult, errOut error) {
                    return nil, errors.New("some kind of error occurred")
                },
            },
        },
    }

    for _, test := range tests {
        t.Run(test.name, func(t *testing.T) {
            s := &store{
                collection: &test.collection,
            }

            documentExists, err := s.DoesImportantDocumentExist()
            if test.errExpected {
                assert.NotNil(t, err)
            } else if test.doesExist {
                assert.True(t, documentExists, "document should exist")
            } else {
                assert.False(t, documentExists, "document should not exist")
            }
        })
    }
}

What We Are Asking

We are asking the maintainers for this package if they are open to changing the public structs in gocb to interfaces for the reasons listed above. The most obvious reason for why the answer would be no is that this would ultimately be a breaking change, and everyone else that uses this package would have to modify their code to work with the interfaces instead. Our goal here is really to test our wrapper in an automated fashion as mentioned before, without the need for a Couchbase instance. If the maintainers for this package have any solutions outside of the one that we have proposed, I am all ears. The alternative, if no other solution is provided and the maintainers are against the changes we have proposed, is for us to maintain our own fork of gocb and make the changes ourselves. We are not entirely against this, but it would be another layer of maintainence since we will need to periodically sync with upstream branch, which could include conflicts.

Additional Details

The changes that were raised in this Pull Request were the bare minimum amount of changes that were required to get the code snippets from above to work. This Pull Request is not a request to merge these changes, but rather to demonstrate the effort that it took to make our code sample work.

[chvck]

Hi @manedurphy apologies for the slow response. Unfortunately, as you say, this would be a breaking change so is not something that we can do at the moment. If we update the API to use interfaces then every time we add a new feature then it will also be a breaking change.

We typically subscribe to "don't mock what you don't own", partially for some of the reasons outlined in https://testing.googleblog.com/2020/07/testing-on-toilet-dont-mock-types-you.html.