2
\$\begingroup\$

I thought of doing the following to mock/test functions in Go and wanted some advice. Let's say you have an interface for a repository:

    type Creator interface {
        CreateUser(User) (*User, error)
    }
    
    type Updater interface {
        UpdateUser(User) (*User, error)
    }
    
    type Retreiver interface {
        GetUserByEmail(string) (*User, error)
        GetUserByID(uint) (*User, error)
    }
    
    type Repository interface {
        Creator
        Updater
        Retreiver
    }

The mock interface implements the Repository interface. It provides mock functions that can be swapped out depending on the test:

type MockUserRepo struct {
    CreateUserMock     func(user.User) (*user.User, error)
    UpdateUserMock     func(user.User) (*user.User, error)
    GetUserByEmailMock func(string) (*user.User, error)
    GetUserByIDMock    func(uint) (*user.User, error)
}

func (m *MockUserRepo) CreateUser(u user.User) (*user.User, error) {
    return m.CreateUserMock(u)
}

func (m *MockUserRepo) UpdateUser(u user.User) (*user.User, error) {
    return m.UpdateUserMock(u)
}

func (m *MockUserRepo) GetUserByEmail(s string) (*user.User, error) {
    return m.GetUserByEmailMock(s)
}

func (m *MockUserRepo) GetUserByID(i uint) (*user.User, error) {
    return m.GetUserByIDMock(i)
}

Then in the test function, you can create a mock for the single function that can be called. This can sometimes cause a nil pointer error if the function wasn't implemented/mocked by the testcase.

mockUserRepo := &MockUserRepo{
    GetUserByEmailMock: func(s string) (*user.User, error) {
        return nil, user.ErrUserDoesNotExist
   },
}

Any thoughts on if this is a bad/good practice?

\$\endgroup\$
1
  • 1
    \$\begingroup\$ This code is working and I've written it. I just copied the relevant parts for review. \$\endgroup\$ – Lfa Apr 10 at 20:10
2
\$\begingroup\$

When it comes to "best practices" to mock interfaces, there's a number of things to consider, not in the least: ease of use. Over the years, I've taken to use a mock generator tool. Rather than just implementing the interface in question, GoMock supports a lot of other useful things. You can read more about it here.

The way to use it in your case would be something akin to this:

//go:generate go run github.com/golang/mock/mockgen -destination mocks/creator_mock.go -package mocks your.project.com/package Creator
type Creator interface {
    CreateUser(User) (*User, error)
}

Then, in your project, just run go generate ./package/... to generate all mocks (ie all interfaces with a go:generate comment).

This will create a new package in your.project.com/package/mocks. In your unit tests, you just create mocks like this:

func TestCreateFail(t *testing.T) {
    ctrl := gomock.Controller(t)
    repo := mocks.NewMockCreator(ctrl)
    arg := User{}
    repo.EXPECT().CreateUser(arg).Times(1).Return(nil, SomeExpectedError)
    obj := NewWhateverYoureTesting(repo) // pass in mock dependency
    err := obj.CallYouWantToTest(arg)
    require.Error(t, err)
    ctrl.Finish() // checks if we received the expected number of calls
}

You can do quite a lot of more complex things with these generated mocks, like inject a custom function to control the behaviour of your mock in more detail, or add a callback to check the state of arguments, change the behaviour of the mocked function that is called based on how many times it is being called, or which arguments exactly are being passed:

func TestCreateSeveral(t *testing.T) {
    ctrl := gomock.Controller(t)
    repo := mocks.NewMockCreator(ctrl)
    arg1, arg2 := User{ID: 1}, User{ID: 2} // 2 different users
    // in case arg1 is passed, all goes well
    repo.EXPECT().CreateUser(arg1).Times(1).Return(&arg1, nil)
    // in case arg2 is passed, we return an error and check some fields
    repo.EXPECT().CreateUser(arg2).Times(1).Return(nil, SomeError).Do(func(check User) {
        // this callback will get the arguments that were passed to the repository mock
        require.Equal(t, check.SomeField, "is equal to this value, set in the function we're testing")
    })
    obj := NewWhateverYoureTesting(repo) // pass in mock dependency
    err := obj.CallYouWantToTest(arg1)
    require.NoError(t, err)
    require.Error(t, obj,CallYouWantToTest(arg2)) // now this should error
    ctrl.Finish() // checks if we received the expected number of calls
}

I'd suggest moving the boilerplate code to set up the mocks and all to a function, and wrap everything in a test-type, just to keep your tests clean:

type testObj struct {
    TestedObj // embed the type you're testing
    ctrl *gomock.Controller
    repo *mocks.MockCreator
}

func getTestedObj(t *testing.T) *testObj {
    ctrl := gomock.NewController(t)
    repo := mocks.NewMockCreator(ctrl)
    return &testObj{
        TestedObj: NewTestedObj(repo),
        ctrl:      ctrl,
        repo:      repo,
    }
}

Then your tests look quite clean, really:

func TestCreateSeveral(t *testing.T) {
    obj := getTestedObj(t)
    defer obj.ctrl.Finish() // defer this to the end

    // the test data
    arg1, arg2 := User{ID: 1}, User{ID: 2}

    // set up mock expectations & behaviour
    obj.repo.EXPECT().CreateUser(arg1).Times(1).Return(&arg1, nil)
    obj.repo.EXPECT().CreateUser(arg2).Times(1).Return(nil, SomeError).Do(func(check User) {
        require.Equal(t, check.SomeField, "is equal to this value, set in the function we're testing")
    })

    // the actual test
    err := obj.CallYouWantToTest(arg1)
    require.NoError(t, err)
    require.Error(t, obj.CallYouWantToTest(arg2))
}

All done.

What is quite useful to keep in mind that, when writing golang, it's considered good practice to define the interface alongside the type which depends on it, not next to the type(s) that end up implementing the interface. That way, everywhere something like a repository is used defines its own specific interface. The interfaces are minimal, and should only contain the methods the user will be using. An interface like you have:

type Repository interface {
    Creator
    Updater
    Retreiver
}

looks a bit suspicious to my eye. This interface is meant to handle everything from creating, updating, and fetching data. Those are distinct operations that I prefer to have handled by separate components, so I'm not very likely to end up with a catch-all interface like this. In addition to this, because interfaces are defined along side the user, not the implementation(s), it's improbable for me to end up composing an interface is quite the same way, too. Because the code to handle creations/updates (in other words writes) and reads would be in different packages, I might end up with something like this:

package foo // read package

//go:generate go run github.com/golang/mock/mockgen -destination mocks/repo_mock.go -package mocks your.project.com/foo Repo
type Repo interface {
    GetUserByEmail(string) (*User, error)
    GetUserByID(uint) (*User, error)
}

And:

package bar // writes

//go:generate go run github.com/golang/mock/mockgen -destination mocks/repo_mock.go -package mocks your.project.com/bar Creator
type Creator interface {
    CreateUser(User) (*User, error)
}

//go:generate go run github.com/golang/mock/mockgen -destination mocks/repo_mock.go -package mocks your.project.com/bar Updater
type Updater interface {
    UpdateUser(User) (*User, error)
}

// I'd probably only use this composed interface in my tests
//go:generate go run github.com/golang/mock/mockgen -destination mocks/repo_mock.go -package mocks your.project.com/bar Repo
type Repo interface {
    Creator
    Updater
}

Random comments:

Some other thoughts I had when I saw the code you posted

Context

I don't see your using the context package anywhere. When dealing with a repository, or a handle to a store of any kind for that matter, 99% of the time, the interface allows for a context argument to be passed. Because of the methods being CreateUser, and GetUserByEmail, I'm assuming you're handling requests of some kind. Requests all have a context associated with it. If the connection is closed, an expensive query ought to be cancelled, rather than it being allowed to continue. The request context will be cancelled in that case, and if you pass that context through to the repository (and eventually use it when hitting the store), that cancellation is propagated automatically. For REST API's, this usually doesn't make a huge difference, but when dealing with websockets (graphQL), or streaming (protobuf), it's important to do. Similarly, when your application boots, it's always a good idea to create an application context. In your main function:

ctx, cfunc := context.WithCancel(context.Background())
defer cfunc() // cancels the context when the application shuts down

You can call the cancellation function when you receive a TERM or KILL signal, or something unexpected happens. Cancelling the application context, again assuming you passed it through when establishing the connection to the store and any other external processes/services you're relying on), it will take care of closing the connections, and freeing up the resources in a clean and efficient way.

Your interface is a bit weird

When creating or updating, you're expecting the caller to pass in an object by value, and you're returning a pointer to the same type. I'm assuming your create function tries to insert the data, and returns nil, err or &userWithIDAfterInsert, nil. Why pass a copy of the data you're trying to insert? Why not simply do this:

type Creator interface {
    CreateUser(context.Context, *User) error // added context
}

Then in the implementation itself, do something like this:

id, err := r.db.Insert(ctx, user) // assuming insert returns the ID, it's a uint, so I'm assuming a SQL-style auto increment primary key
if err != nil {
    return err
}
user.ID = id
return nil

Same goes for your update function. The arguments that are being passed in represent the same thing as the thing you're returning. Why faff around with 2 copies of the same thing, if you can just do it all with the same object?

\$\endgroup\$
2
  • \$\begingroup\$ Thank you! I need to look into using Context. This is such a clear example for why it's important to use it! I like the idea of appending the data to the same object. There are other attributes that are updated after the insert (e.g. updated time, created time, etc..), but I should just be able to pass in the same object that was passed to me to the query. I appreciate the insight on where to put the interface as well. What if multiple packages want to use that same interface (e.g. multiple packages want to get user by ID)? Is it ok to repeat the "reader" interface in that instance? \$\endgroup\$ – Lfa Apr 12 at 23:29
  • 1
    \$\begingroup\$ @Lfa It's quite common to have a similar, or indeed exactly the same interface being defined multiple times in a project. The thing to keep in mind is how likely it is for a given interface to change (ie adding methods) in some places rather than others. If so, keep distinct interfaces. If it's just a one-method interface that is going to be used consistently across the entire project, then you can make a judgement call to keep a central definition instead \$\endgroup\$ – Elias Van Ootegem Apr 13 at 8:59
2
\$\begingroup\$

This seems fine to me: it's nice and simple, and I've seen this pattern used "in the wild" before. One thing you could do is, in the MockUserRepo proxying functions, if the function variable is nil, either execute a default implementation, or return a more specific error (or even panic). For example:

func (m *MockUserRepo) GetUserByEmail(s string) (*user.User, error) {
    if m.GetUserByEmailMock == nil {
        // default implementation: return a dummy user object
        return &user.User{ID: generateID(), Email: s}, nil
    }
    return m.GetUserByEmailMock(s)
}

// or:

func (m *MockUserRepo) GetUserByEmail(s string) (*user.User, error) {
    if m.GetUserByEmailMock == nil {
        // default implementation: panic with a more specific error
        panic("MockUserRepo: GetUserByEmail not set")
    }
    return m.GetUserByEmailMock(s)
}

The reason I'd be okay with a panic here is because not setting GetUserByEmail is almost certainly a coding bug in the test, not really a runtime error condition. This latter approach doesn't gain you much over just letting the nil pointer access panic, except that the error message is a bit more specific.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you! I appreciate the review! \$\endgroup\$ – Lfa Apr 12 at 23:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.