Unit testing a REST API in Go

Question

I am learning Go by writing a simple CRUD REST API using gotilla/mux and Gorm.

I would like to get some feedback on the application code and on unit tests in particular. Please suggest me what to improve, what to do better, what not to do and generally tell me what comes to your mind.

func TestGetAllStudents(t *testing.T) {
    app := initApp()
    st := student{ID: "id-1", Age: 20, Name: "John Doe"}
    app.db.Save(st)
    req, _ := http.NewRequest("GET", "/students", nil)
    r := httptest.NewRecorder()
    handler := http.HandlerFunc(app.getAllStudents)

    handler.ServeHTTP(r, req)

    checkStatusCode(r.Code, http.StatusOK, t)
    checkContentType(r, t)
    checkBody(r.Body, st, t)
}

func TestAddStudent(t *testing.T) {
    app := initApp()
    var rqBody = toReader(`{"name":"John Doe", "age":20}`)
    req, _ := http.NewRequest("POST", "/students", rqBody)
    r := httptest.NewRecorder()
    handler := http.HandlerFunc(app.addStudent)

    handler.ServeHTTP(r, req)

    checkStatusCode(r.Code, http.StatusCreated, t)
    checkContentType(r, t)
    checkProperties(firstStudent(app), t)
}

func TestUpdateStudent(t *testing.T) {
    app := initApp()
    app.db.Save(student{ID: "id-1", Age: 25, Name: "Peter Doe"})
    var rqBody = toReader(`{"name":"John Doe", "age":20}`)
    req, _ := http.NewRequest("PUT", "/students/id", rqBody)
    req = mux.SetURLVars(req, map[string]string{"id": "id-1"})
    r := httptest.NewRecorder()
    handler := http.HandlerFunc(app.updateStudent)

    handler.ServeHTTP(r, req)

    checkStatusCode(r.Code, http.StatusOK, t)
    checkContentType(r, t)
    checkProperties(firstStudent(app), t)
}

func TestDeleteStudent(t *testing.T) {
    app := initApp()
    app.db.Save(student{ID: "id-1", Age: 20, Name: "John Doe"})
    req, _ := http.NewRequest("DELETE", "/students/id", nil)
    req = mux.SetURLVars(req, map[string]string{"id": "id-1"})
    r := httptest.NewRecorder()
    handler := http.HandlerFunc(app.deleteStudent)

    handler.ServeHTTP(r, req)

    checkStatusCode(r.Code, http.StatusOK, t)
    checkContentType(r, t)
    checkDbIsEmpty(app.db, t)
}

func initApp() App {
    db, _ := gorm.Open("sqlite3", ":memory:")
    db.AutoMigrate(&student{})
    return App{db: db}
}

func firstStudent(app App) student {
    var all []student
    app.db.Find(&all)
    return all[0]
}

func toReader(content string) io.Reader {
    return bytes.NewBuffer([]byte(content))
}

func checkStatusCode(code int, want int, t *testing.T) {
    if code != want {
        t.Errorf("Wrong status code: got %v want %v", code, want)
    }
}

func checkContentType(r *httptest.ResponseRecorder, t *testing.T) {
    ct := r.Header().Get("Content-Type")
    if ct != "application/json" {
        t.Errorf("Wrong Content Type: got %v want application/json", ct)
    }
}

func checkProperties(st student, t *testing.T) {
    if st.Name != "John Doe" {
        t.Errorf("Name should match: got %v want %v", st.Name, "Peter Doe")
    }
    if st.Age != 20 {
        t.Errorf("Age should match: got %v want %v", st.Age, 20)
    }
}

func checkBody(body *bytes.Buffer, st student, t *testing.T) {
    var students []student
    _ = json.Unmarshal(body.Bytes(), &students)
    if len(students) != 1 {
        t.Errorf("Wrong lenght: got %v want 1", len(students))
    }
    if students[0] != st {
        t.Errorf("Wrong body: got %v want %v", students[0], st)
    }
}

func checkDbIsEmpty(db *gorm.DB, t *testing.T) {
    var students []student
    db.Find(&students)
    if len(students) != 0 {
        t.Errorf("Student has not been deleted")
    }
}

Here are the concerns I already have (not sure, how relevant they are):

• I didn't manage to mock DB, that's why I am using an in-memory DB
• The verification code was too verbose, I extracted it into separate functions. Is it something you would do?
• I didn't manage to test negative scenarios (as a result of the first concern).
• I don't like having to call mux.SetURLVars to inject a path variable. How to do this better?
• I didn't manage to test the code that initializes Mux routers

Answer 1

Here are some notes I made while reading your code.

For a real-world code review, code should be correct, maintainable, reasonably efficient, and, most importantly, readable.

Writing code is a process of stepwise refinement.

Start by looking at two similar methods.

go-angular/server/app.go:

func (a *App) addStudent(w http.ResponseWriter, r *http.Request) {
    w.Header().Set("Content-Type", "application/json")
    s := student{}
    err := json.NewDecoder(r.Body).Decode(&s)
    s.ID = uuid.New().String()
    if err != nil {
        sendErr(w, http.StatusBadRequest, err.Error())
    } else {
        err = a.db.Save(&s).Error
        if err != nil {
            sendErr(w, http.StatusInternalServerError, err.Error())
        } else {
            w.WriteHeader(http.StatusCreated)
        }
    }
}

func (a *App) updateStudent(w http.ResponseWriter, r *http.Request) {
    w.Header().Set("Content-Type", "application/json")
    s := student{}
    err := json.NewDecoder(r.Body).Decode(&s)
    if err != nil {
        sendErr(w, http.StatusBadRequest, err.Error())
    } else {
        s.ID = mux.Vars(r)["id"]
        err = a.db.Save(&s).Error
        if err != nil {
            sendErr(w, http.StatusInternalServerError, err.Error())
        }
    }
}

Write them in more readable form.

func (a *App) addStudent(w http.ResponseWriter, r *http.Request) {
    w.Header().Set("Content-Type", "application/json")
    var s student
    err := json.NewDecoder(r.Body).Decode(&s)
    if err != nil {
        sendErr(w, http.StatusBadRequest, err.Error())
        return
    }
    s.ID = uuid.New().String()
    err = a.db.Save(&s).Error
    if err != nil {
        sendErr(w, http.StatusInternalServerError, err.Error())
        return
    }
    w.WriteHeader(http.StatusCreated)
}

func (a *App) updateStudent(w http.ResponseWriter, r *http.Request) {
    w.Header().Set("Content-Type", "application/json")
    var s student
    err := json.NewDecoder(r.Body).Decode(&s)
    if err != nil {
        sendErr(w, http.StatusBadRequest, err.Error())
        return
    }
    s.ID = mux.Vars(r)["id"]
    err = a.db.Save(&s).Error
    if err != nil {
        sendErr(w, http.StatusInternalServerError, err.Error())
        return
    }
    w.WriteHeader(http.StatusOK)
}

Write more readable code.

Go Code Review Comments: Indent Error Flow.

Try to keep the normal code path at a minimal indentation, and indent the error handling, dealing with it first. This improves the readability of the code by permitting visually scanning the normal path quickly.

Use var to declare variables that are decoding targets. They have zero value(s).

var s student

Don't do things before they are necessary.

s.ID = uuid.New().String()

The s.ID value is discarded in addStudent if there is an error in decoding. Wait until it is needed, as you did in updateStudent.

In addStudent you returned a status.

w.WriteHeader(http.StatusCreated)

Return a status in updateStudent.

w.WriteHeader(http.StatusOK)

And so on.

Make sure that your app.go code is good before worrying about testing.

Answer 2

OK, so a bit later than I expected, but here's some comments I have. I'll try to address the concerns you list to a reasonable degree:

• I didn't manage to mock DB, that's why I am using an in-memory DB

This should be covered by the big section titled "This is not a unit test"

• The verification code was too verbose, I extracted it into separate functions. Is it something you would do?

I've also hinted at creating a package for this, so yes, this is most certainly something I'd separate out. Having said that, you might want to do some googling to find packages that generate validation code. A quick search came up with this govalidator package. I've not used it myself, so I can't vouch for it, but there's plenty of packages out there that will allow you to write something like this:

type CreateUserRequest struct {
    Name string `validate:type="string";min_len=5;regex="[A-Za-Z ]+"`
    Age  int    `validate:min="18";max="120"`
}

The syntax and example validation is completely made-up, so you'll have to see for yourself what you want and what makes sense.

• I didn't manage to test negative scenarios (as a result of the first concern).

Like you said, this ties in with your first concern, and my breakdown of that problem addresses this, too.

• I don't like having to call mux.SetURLVars to inject a path variable. How to do this better?

The golang standard library provides you with the net/http/httptest package, which probably can help you out with this. This is something I've not discussed yet, but I'm likely to revisit this review with some more details on this.

• I didn't manage to test the code that initializes Mux routers

As mentioned in the comment I left: you shouldn't unit-test external packages. You should choose a dependency based on functionality, maturity, community support, performance, etc... depending on your needs, the weight you give each to of these factors can vary. Generally speaking, gorilla.Muxis a widely used, mature, and battle tested package. You can rely on the router component being well tested. It's not the user's job of a package to unit-test a dependency. That's a bit like trying to write unit-tests covering golang's standard library functions. That's the job of the people maintaining those packages. If you can test that requests to a given route land you in the correct handler function (which you can), then you know the routes have been configured correctly.

Anyway: here's the actual code-review:

This is not a unit test

What you have is not a unit test. Plain and simple. A unit test is a type of test where you test the logic inside a given package. You want to control the input entirely so you can see if you're getting the desired output. Your tests rely on stuff like the database to function. In a real unit test, all dependencies that aren't essential to the business logic inside the code you're testing (which should be all dependencies) should be mocked.

Looking at the code itself, there's fairly little in the way of business logic to test, but assuming you'll build on what you have, you might want to change the structure of things somewhat.

Separate into packages

It's good practice to move all the direct DB interactions into its own package. That will allow you to test the more complex logic without needing to setup an in-memory database all the time, and test basic stuff with arbitrary data. Let's assume you want'a CAS (Check And Set) or Upsert type functionality at some point. Let's create a type that has the logic to validate/check student data, and can call on the DB to store/update the records:

package persistence

// DB interface we need, the package USING the dependency declares the interface it needs
type DB interface {
    Save(*Student) error
    GetByID(id string) (*Student, error)
    Update(*Student) error // this could be handled by Save,but you'll see why I'm making the destinction
}

// Logger interface, this should be an application wide thing, and needn't be an interface per package, but I'm including it here for completeness
type Logger interface {
    Debug(... interface{})
    Info(... interface{})
    Warn(... interface{})
    Err(... interface{})
    Fatal(... interface{})
}

// CAS I'm bad at naming things, but you get the gist
type CAS struct {
    db DB
    log Logger
}

var (
    // NameEmptyErr one of many predefined errors validation can return
    NameEmptyErr    = errors.New("name for student missing")
    IDAlreadySetErr = errors.New("new student record already has an ID, call Update/Upsert")
    PersitenceErr   = errors.New("failed to persist data")
)

// New returns a new CAS validator, pass in dependencies here (DI)
func New(log Logger, db DB) *CAS {
    return &CAS{
        db: db,
        log: log,
    }
}

// CASStudent validates the student record, and if everything checks out, the record is stored
func (c *CAS) CASStudent(student *Student) error {
    if student.Name == "" {
        return NameEmptyErr
    }
    // add all validation here
    if student.ID != "" {
        return IDAlreadySetErr
    }
    // validation complete, store. Let the db layer handle ID's
    // we're passing a pointer to the data, so the ID can be set on the object
    if err := c.db.Save(student); err != nil {
        c.log.Err(err)
        return PersistenceErr
    }
    return nil
}

Great, now how does this help you get closer to mocking dependencies? Does this mean you have to manually create test types for all the interfaces you need to mock? Well actually, no. Thankfully, there's tools that can auto-generate all the mocks you need. If you update the interfaces, just running a single command will update the mocks to conform to the new interfaces. The best tool for this in my experience is gomock and mockgen. It's fantastically easy to use, just add a single line of comments above the interfaces you need mocking:

package persistence

//go:generate go run github.com/golang/mock/mockgen -destination mocks/db_mock.go -package mocks github.com/your/repo/persistence DB
type DB interface{}

//go:generate go run github.com/golang/mock/mockgen -destination mocks/logger_mock.go -package mocks github.com/your/repo/persistence Logger
type Logger interface{}

to generate the code, in your repo, simply run go generate ./..., and all mocks for all packages will be generated and you'll find the mocks for a given package under path/to/package/mocks. The mock package name will be package mocks.

Cool, let's write a unit test

So we have a package that takes care of validation, and uses mockable dependencies, let's set about writing a unit test for our new persistence package:

package persistence_test // yes, we're changing the package name

import (
    "testing"

    "github.com/your/repo/persistence" // import the package to test
    "github.com/your/repo/persistence/mocks" // import the mocks

    "github.com/golang/mocks/gomock" // you'll see why I'm using these
    "github.com/stretchr/testify/assert"
)

type testCAS struct {
    *persistence.CAS // the type we're testing
    ctrl *gomock.Controller
    db   *mocks.MockDB
    log  *mocks.MockLogger
}

// this func sets up everything we need to run a test with mocks
func getCAS(t *testing.T) testCas {
    ctrl := gomock.NewController(t)
    db := mocks.NewDBMock(ctrl)
    log := mocks.NewLoggerMock(ctrl)
    return testCas{
        CAS: persistence.New(log,db),
        ctrl: ctrl,
        db: db,
        log: log,
    }
}

func TestCASStudentSuccessSimple(t *testing.T) {
    cas := getCAS(t)
    defer cas.Finish()
    id := "id set by mock"
    student := &persistence.Student{
        Name: "this needs to be set",
    }
    // if our code is correct, we're expecting exactly 1 call to db.Save
    // for this test we don't want it to return an error, so job done
    // the DB package is going to set the ID, so we'll set up the mock to do the same
    cas.db.EXPECT().Save(student).Times(1).Return(nil).Do(func(s *persitence.Student) {
        s.ID = id // set the id to the var above
    })
    // Now let's call the code we want to test:
    err := cas.CASStudent(student)
    assert.NoError(t, err) // ensure no errors were returned
    assert.Equal(t, id, student.ID) // the id should now be set
}

// Finish isn't required, but I add it for convenience, instead of writing foo.ctrl.Finish()
func (c *testCas) Finish() {
    c.ctrl.Finish()
}

Great! Now That's a unit test. The only actual code we're executing is found in the persistence package, and it's only testing the unit of code that validates the data, and passes it on for storage. The added benefit of this is that you don't have to rewrite a bunch of test should you decide to change the underlying storage solution to, say, a key-value store, or some other DB. The interfaces can remain unchanged, so you can swap out a package, safe in the knowledge that all code will still behave as expected as long as the interface they're expecting is implemented. If there is a bug, you'll just know the bug is found in the package you're replaced the old one with, and nowhere else.

Just for fun, let's try a more complex test:

func TestCASComplex(t *testing.T) {
    cas := getCas(t)
    defer cas.Finish()
    // data contains the test-cases, expect contains the errors we may expect
    data := map[strign]*persistence.Student{
        "noName": {}, // no name set, we expect validation to fail
        "idSet":  {
            Name: "name is set",
            ID: "but so is ID",
        },
        "dbFail": {
            Name: "this is fine",
        },
    }
    expect := map[string]error{
        "noName": persistence.NameEmptyErr,
        "idSet":  persistence.IDAlreadySetErr,
        "dbFail": persistence.PersistenceErr,
    }
    // the first 2 calls won't call the DB/logger, only the last one will:
    dbErr := errors.New("db error")
    // you can change gomock.Any() with the data["dbFail"] value
    cas.db.EXPECT().Save(gomock.Any()).Times(1).Return(dbErr)
    cas.log.EXPECT().Err(dbErr).Times(1) // the error from the db should be logged
    for k, student := range data {
        expErr := expect[k] // get the error we're expecting
        optionalID := student.ID // get ID before the call
        err := cas.Save(student)
        assert.Error(t, err) // make sure an error is returned
        assert.Equal(t, expErr, err) // and check if it's the error we're expecting
        assert.Equal(t, optionalID, student.ID) // make sure the ID wasn't changed
    }
}

So with these 2 example test functions, we're covering all possible code-paths in the CASStudent function:

• The student name is missing
• the student already has an ID
• the student data is valid and stored successfully
• The student data is valid, but the DB failed

Neat!

Important note/rant on interfaces

A common thing to see people do, especially when they're coming from another language that is more, shall we say, "traditionally OO", is to declare the interface for any given component/type in the package where the interface is implemented. In golang, the opposite approach is the way to go. A packackage handling DB interactions returns a "raw" type that can be passed to one or more types that depend on it. Each of these users might need different parts of the API this DB type exposes. If all of them use the same, big, interface, chances of tightly coupled code increase, and it becomes a lot harder to swap out dependencies on the fly (or even refactor on a package-by-package basis). Go's ducktype interfaces are a very powerful tool in your armory here. I've purposefully declared the Logger and DB interfaces in the persistence package above. Basic SOLID principles apply here (IoC and Liskov substitution principle specifically).

A simple example:

package "contacts"

type addressBook struct {
    data map[string]Address
}

func NewAddressbook(addresses ...Address) *addressBook {
    a := addressBook{
        data: map[string]Address{}, // or make(map[string]Address, len(addresses)
    }
    for _, addr := range addresses {
        a[addr.Person.Name] = addr
    }
    return &a
}

func (a *addressBook) Add(addr Address) {
    a.data[addr.Person.Name] = addr
}

func (a addressBook) Find(name string) (*Address, error) {
    if addr, ok := a.data[name]; ok {
        return &addr, nil
    }
    return nil, AddressNotFoundErr
}

Now this is all pretty clear, but what if I were to change the NewAddressbook func to return an interface that I defined in this package. The interface would, naturally, have to contain all exported functions, so it'd look like this:

type Addressbook interface {
    Add(Address)
    Find(string) (*Address, error)
}

The package that's writing to the addressbook, then, will require its dependency to implement a Find function, too, but it simply never uses it. That's code-smell at best. In practice, it's a matter of time before some developer gets lazy, and implements something in that package real quick (e.g. a Upsert type call) using the Find function. This function, however, is using a value receiver, not a pointer receiver, and thus you've introduced a potential data-race.

The same problem presents itself when you use this interface in the package responsible for displaying the addressbook data: it now has write access to the data. It shouldn't. Even back in the 70s, K&R knew this wasn't right, and used the const qualifier to protect people against themselves.

For these reasons (and separation of concern, modularity, testability, sanity, unicorn happiness, ...) the package writing should declare an interface only specifying the Add() func, and the display package should only specify the Find() function. If you change the addressbook from an in-memory map to a DB backed store, and want to write custom filters for the representation layer, you're likely to end up with the read and write dependencies being provided by different packages anyway.

Sorry for the tangent, but it's a giant pet-peeve of mine. I've seen countless people implement a single, giant interface rather than being sensible. Trust me, it's a lot easier to find your way in a codebase when you open a file, and just see an explicit definition of all the interfaces that package/type depends on, instead of seeing a ton of imports, pointing you towards 1001 interfaces, and then having to through the code to work out which parts of the interfaces the code you're working on actually relies on. (rant over)

Cosmetics

Now lists like the ones above are nice to keep track of what you're testing. Wouldn't it be nice to have a way to have lists like that in our unit test? Thankfully, you can. Instead of having a data and expect map, or a TestX function per case, I tend to group tests per function like this:

func TestCASStudent(t *testing.T) {
    t.Run("Student name is missing", studentNameMissing)
    t.Run("Student ID already set", studentIDSet)
    t.Run("Student CAS success", studentCASSuccess)
    t.Run("Student valid, DB error", studentDBError)
}

Then it's a simple matter of implementing the functions one by one.

Further reading/later update

I'm going to leave you with this to digest for now. I'll probably revisit this answer at a later date, because there's still stuff I haven't gotten to. For now, though, I'll point you to the standard httptest package, which could be useful when testing HTTP requests :)

Answer 3

You can simplify your build and allow for easier integration tests* with a docker-compose.yml file.

version: "3.7"
services:
  webapp:
    build: .
    ports: 
      - 8080:8080
    depends_on:
      - db
    environment:
      - DB_PASS='your-strong-pass'
  db:
    image: postgres:11.5
    environment:
      - POSTGRES_USER=go
      - POSTGRES_PASSWORD=your-strong-pass
      - POSTGRES_DB=go

Just add it to the root of your application and run the command docker-compose up --build.
To use it for tests, you can have a different docker-compose file with a command to run integration tests.

• Note that integration tests are not unit tests.
  With unit testing, you are testing functions in the purest form you can achieve.
  With integration tests you are running your program while mocking the outside world (databases, file systems, ...) and check if the output of your program (or the exposed APIs) is as you expect.
  Both kinds of tests are important as they test different things.