Testing a template function in GoLang using mocking - making it DRY & easy to follow

Question

TL;DR

I'm wondering how to organise test functions using mocks for GoLang drier & easier to follow.

Function

I have a template function that is very similar to ACF's getField:

• It take's in a string (field name) & the post ID.
• It checks if the ID exists and the obtains the post by ID (the f.store.Posts belongs to the fields struct)
• If the post doesn't exist or if the fields could not be unmarshalled it will return an empty string.

func (f *fields) getField(field string, id ...int) interface{} {

    fields := f.fields
    if len(id) > 0 {
        post, err := f.store.Posts.GetById(id[0])
        if err != nil {
            return ""
        }
        var m map[string]interface{}
        if err := json.Unmarshal(*post.Fields, &m); err != nil {
            return ""
        }
        fields = m
    }

    val, found := fields[field]
    if !found {
        return ""
    }

    return val
}

Testing

From the function, presumably I need to test if, The post exists, the json could be unmarshalle or if the field exists. I have started to use github.com/golang/mock/gomock but am finding it confusing! In each of the test functions below I create a new controller and mockPost, add assigning the fields Post to it.

func TestGetField(t *testing.T) {
    f, err := helper(`{"text": "content"}`)
    if err != nil {
        t.Error(err)
    }

    if field := f.getField("text"); field == "" {
        t.Errorf(test.Format("content", nil))
    }

    if field := f.getField("wrongval"); field != "" {
        t.Errorf(test.Format("", field))
    }
}

func TestGetField_Post(t *testing.T) {
    f, err := helper("{}")
    if err != nil {
        t.Error(err)
    }

    controller := gomock.NewController(t)
    defer controller.Finish()

    data := []byte(`{"posttext": "postcontent"}`)
    mockPost := domain.Post{
        Id:     2,
        Fields: (*json.RawMessage)(&data),
    }

    posts := mocks.NewMockPostsRepository(controller)
    f.store.Posts = posts
    posts.EXPECT().GetById(2).Return(mockPost, nil)

    field := f.getField("posttext", 2)
    if field != "postcontent" {
        t.Errorf(test.Format("postcontent", field))
    }
}

func TestGetField_No_Post(t *testing.T) {
    f, err := helper("{}")
    if err != nil {
        t.Error(err)
    }

    controller := gomock.NewController(t)
    defer controller.Finish()

    posts := mocks.NewMockPostsRepository(controller)
    var mockErr = fmt.Errorf("No post")
    posts.EXPECT().GetById(gomock.Any()).Return(domain.Post{}, mockErr)
    f.store.Posts = posts

    field := f.getField("text", 1)

    if field != "" {
        t.Errorf(test.Format("", field))
    }
}

func TestGetField_Invalid_JSON(t *testing.T) {
    f, err := helper("{}")
    if err != nil {
        t.Error(err)
    }

    controller := gomock.NewController(t)
    defer controller.Finish()

    data := []byte(`"text "content"`)
    mockPost := domain.Post{
        Id:     1,
        Fields: (*json.RawMessage)(&data),
    }

    posts := mocks.NewMockPostsRepository(controller)
    posts.EXPECT().GetById(1).Return(mockPost, nil)
    f.store.Posts = posts

    field := f.getField("text", 1)
    if field != "" {
        t.Errorf(test.Format("", field))
    }
}

Feedback

• Without using the mock package, how would you approach this?
• Are there any areas for improvement with the current codebase?
• Is there away to consolidate any of the testing methods? It seems quite long winded for such a short function

Answer 1

I don't know exactly what your fields type is, but from the code you've shared, there's a couple of strong recommendations I'd like to make about the code itself, before getting in to the testing aspect.

post, err := f.store.Posts.GetById(id[0])

From this line, and the fact that you're mocking store, I'm deducing your fields constructor looks something like this:

func New(store SomeStoreT) *fields {
    return &fields{
        store: store,
    }
}

Because you're accessing store.Posts directly, the SomeStoreT is going to be the actual type, complete with exposed fields. This means you can't really mock the store type as a whole, you have to ensure that whatever the exported fields are, is safe for concurrent use, and that no other code either on purpose or accidentally assigns anything to said fields. An extreme example would be:

func main () {
    store := storage.New()
    f := fields.New(store)
    store.Posts = nil
    f.GetPosts() // panic
}

It's therefore better to either replace the store dependency with an interface that exposes what you need through getters, so the fields needn't be exported. It'd be as simple as changing the line

post, err := f.store.Posts.GetById(id[0])

to:

post, err := f.store.Posts().GetById(id[0])

Still, there's something that I find a lot more disturbing than this: the use of interface{} and the json.Unmarshal() calls. Your store field has a Posts field. That tells me you know what data you're fetching, or saving. Why would you let code other than the storage layer deal with the marshalling and unmarshalling of that data? Your code should pass around a type that represents a post, and it's the job of the storage layer to handle the marshalling. If you decide JSON isn't the right format, and want to opt for something faster (like a binary format), or you want to switch to a completely different type of DB like Postgres, then you don't want to be forced to change the code throughout your application. You just should be able to swap out you storage adapter, optionally change the tags on the data types, and update the package to handle the new marshalling and be done with it:

package domain

type Post struct {
    ID      int    `json:"id"` // preferred ID in all-caps
    Content string `json:"content"`
}

Then, in the storage package:

package storage

type Posts struct {
    // whatever fields to access the storage used
}

func (p *Posts) GetByID(id int) (*domain.Post, error) {
   record, err := p.db.Get(id) // whatever you need to do here
   if err != nil {
      return nil, err
   }
   ret := domain.Post{}
   if err := json.Unmarshal(record, &ret); err != nil {
       return nil, err
   }
   return &ret, nil
}

So with this, you can change your function in fields to not have to do any unmarshalling at all. It's all handled in the storage. Now, getting a field by string isn't possible with this code, of course. I'm not sure of that's actually a good idea, but that's more of an X-Y problem. You can quite easily implement something that makes getting fields by string ridiculously easy, in fact. All you need to do is create a wrapper type and custom unmarshalling:

package domain

type Post struct {
    PostRecord // embedded struct
    postMap map[string]interface{}
}

type PostRecord struct {
    ID      int    `json:"id"`
    Content string `json:"content"`
}

func (p *Post) UnmarshalJSON(data []byte) error {
    // unmarshal into embedded type
    if err := json.Unmarshal(data, &p.PostRecord); err != nil {
        return err
    }
    if err := json.Unmarshal(data, &p.postMap); err != nil {
        return err
    }
    return nil
}

func (p Post) MarshalJSON() ([]byte, error) {
    return json.Marshal(p.PostRecord) // marshal the embedded type
}

func (p Post) GetField(f string) (interface{}, bool) {
    v, ok := p.postMap[f]
    return v, ok
}

Now your fields function would look something like this:

// you're only ever using `id[0]`, so I don't understand the variadic argument here
// it's also important to note that there's a lot of calls that can error happening here
// and you need to return those errors. It'll make debugging a lot easier
func (f *fields) getField(field string, id int) (interface{}, error) {
    post, err := f.store.Posts().GetByID(id)
    if err != nil {
        return nil, err
    }
    v, ok := post.GetField(field)
    if !ok {
        return nil, errors.New("unknown field for post type")
    }
    return v, nil
}

You'll note that I'm returning nil in case of an error. That's because an empty string is the nil value for a string, but it could just as well be a valid value for an existing record in some cases. It's best to indicate an error by returning said error, and nil for values that are just not defined. If a field doesn't exist, it has no value, and so nil is the only correct thing to return. Of course, the nil value for an int is 0, and an empty string for a string, but your function looks to me like it's written to be non-type-specific. If the Post type had a timestamp field, stored either as time.Time or int64, and you don't return an error, just an empty string, then the caller might do something like this:

tval := fields.getField("timestamp", 123)
ts, ok := tval.(int64)
if !ok {
    return errors.New("invalid timestamp value")
}

Now why is the timestamp value invalid? Becuase the field didn't exist (a typo in the string I passed)? Because the ID doesn't exist? Or because the timestamp field has since been updated to another type? I don't know...

This also assumes that the caller has the sense to check whether the type assertion worked. There's no reason why the caller might just have:

ts := tval.(int64)

and continue to use ts assuming it was the correct value, while in reality it'll default to 0. Write your functions to account for sloppy use. Errors are more likely to be checked than boolean indicators. Return values should be unequivocal when it comes to distinguishing between an error and success. An emtpy string is a perfectly valid return value on success, so don't rely on it to communicate failure.

---

I'll update this review gradually. I think this is a reasonable amount of information to digest, and get to work with. I've written all of the code snippets above off the cuff, untested, etc... there may be typo's and omissions, so don't assume they're copy-paste ready.

---

TL;DR answers to your questions

• You have to mock dependencies in unit tests. that's what unit tests are/do
• I've started this review with some suggestions/recommendations to improve on your code - so yes, there are things that you can do there
• with the changes I've proposed, the getFields function is basically a call to the storage package (which is tested separately), and a call on the domain object's GetField function, which is something you unit-test separately. you can still write tests for the function here, but it's just having a mock return either an error or a Post instantce, which you can hard-code in your test. It's far less cumbersome to set up, and test as a result.