Stop Fighting Your Go Tests: Simplify and Clarify
Last time, we did some spring cleaning. We kicked out unnecessary third-party dependencies that were lounging around in our tests and contributing nothing. Along the way, we discovered yet another bad coding habit: making our projects more complicated by overusing external packages when the standard library does the job.
This time, we’re rolling up our sleeves to simplify the test setup, pick Go types that actually make sense, and stop confusing readers with constantly changing names.
All of this we will illustrate with few examples taken from OSS code - the cache storages project used by the Souin HTTP cache.
The Souin HTTP cache is a new HTTP cache system suitable to use by every reverse-proxy. It can be either placed on top of reverse-proxies like Apache, Nginx or as plugin in projects like Træfik, Caddy or Tyk.
So, let’s jump straight to the code and read tests for the simplefs storage plugin.
Checking errors an unusual way
Right at the beginning of the file, we can see the first test. What’s going on here?
func TestCustomSimplefsConnectionFactory(t *testing.T) {
instance, err := getSimplefsInstance()
if nil != err {
t.Error("Shouldn't have panic")
}
if nil == instance {
t.Error("Simplefs should be instanciated")
}
}
Yes, checking errors this way seems unusual. The function getSimplefsInstance returns two values: instance and an err. As always, we verify if the returned err is not nil. The test does this, but the positions of err and nil are reversed. The fix is straightforward.
if err != nil {
t.Error("Shouldn't have panic")
}
Now, the error message. It tells us Shouldn't have panic. Does it mean that the function getSimplefsInstance can panic? We don’t know it. But even if the getSimplefsInstance panics, there would be no way we could reach the line that checks the error. So, the error message must be an unfortunate choice of words.
What does the function actually do? It creates a default store. The fs in the name suggests we deal with a file system store.
func getSimplefsInstance() (core.Storer, error) {
return simplefs.Factory(core.CacheProvider{}, zap.NewNop().Sugar(), 0)
}
The function doesn’t accept any parameters. It returns two values: some store and an error. A non-nil error indicates that the factory function failed to create the store, and we can’t use it in tests. If that happens, each test should fail immediately. But it’s not the case, unfortunately. We must make a small change and replace t.Error() with t.Fatal(). We can skip creating store value, as we are only interested in detecting potential errors.
func TestCustomSimplefsConnectionFactory(t *testing.T) {
_, err := getSimplefsInstance()
if err != nil {
t.Fatal(err)
}
}
At this stage, we have a test that calls getSimplefsInstance and checks if the function returns an error. That’s all. We also replaced the string literal Shouldn't have panic with the actual err value to be able to see error messages if something goes wrong.
It’s time to ask our usual question: What are we really testing here? Does the test name accurately describe the tested behaviour? The name says TestCustomSimplefsConnectionFactory. Honestly, it’s hard to imagine what behaviour we’re aiming to verify here.
Employing the Black-Box tactic
Remember, we should be able to understand the tested behaviour by glancing at the terminal. Since we deal with the package public API, it’s even more important to document behaviour and use descriptive names.
package simplefs_test
import (
...
"github.com/darkweak/storages/core"
"github.com/darkweak/storages/simplefs"
)
The first line of the snippet above tells us that the tests are in a separate package. We have tests not in the simplefs package but in the simplefs_test. It means we must import the simplefs package and can interact with it only through its interface - exported identifiers. It’s a good example of black-box testing techniques applied on the package level.
We can simplify the test names to reflect what behaviour we test. How about changing the name from
func TestCustomSimplefsConnectionFactory(t *testing.T)
to
func TestCreateDefaultStore(t *testing.T)
In the end, that’s all we do. We create an instance of a store, a default store in the storefs package. After a minor refactoring, we end up with the following test.
func TestCreateDefaultStore(t *testing.T) {
_, err := getSimplefsInstance()
if err != nil {
t.Fatal(err)
}
}
Let’s run the tests to make sure we didn’t break anything.
go test
PASS
ok github.com/darkweak/storages/simplefs 10.300s
Everything is OK. We can move on.
Assumptions are dangerous
Although tests are green, we should ask again our fundamental question: what do we really test here and why?
Let’s take a look at the rest of the tests. Can you spot a dangerous pattern?
func TestIShouldBeAbleToReadAndWriteDataInSimplefs(t *testing.T) {
client, _ := getSimplefsInstance()
_ = client.Set("Test", []byte(baseValue), time.Duration(20)*time.Second)
...
}
That’s right. All tests ignore possible errors. First, when tests create the store (here, the variable client), and when operating the store—setting and retrieving values. That’s questionable at best.
The fact that we run only one test to verify the creation of the default store doesn’t mean each test can create the store! What if something happens during test execution and, for whatever reason, the test doesn’t create the store? We won’t be able to catch it, and will continue running tests and executing business logic. Horrible perspective!
Second, all tests ignore potential errors that can occur during store operations. For example, what happens if client.Set returns an error? If yes, how are we going to interpret results from client.Get? You see where we are going. One omission affects another. Errors compound and make the test useless. This, on the other hand, rots confidence in the quality of the software we build.
Let’s fix the problems one at a time.
t.Helper to the rescue
First, we focus on changing how we build the store. We will start by changing the function name to newDefaultStore. It’s shorter and more descriptive than the original one.
Second, we change the function signature. The function accepts the *testing.T struct and registers itself as a test helper by calling t.Helper() right at the beginning. The advantage? Any potential error originating from calling simplefs.Factory is handled in the test helper.
func newDefaultStore(t *testing.T) core.Storer {
t.Helper()
s, err := simplefs.Factory(core.CacheProvider{}, zap.NewNop().Sugar(), 0)
if err != nil {
t.Fatal(err)
}
return s
}
It means that the function creates only a valid store. If an error occurs, the function calls t.Fatal, and the entire test fails before any business logic is executed. Let’s see how to test our new machinery.
func TestSetAndRetrieveValueFromStore(t *testing.T) {
store := newDefaultStore(t)
err := store.Set("Test", []byte(baseValue), time.Duration(20)*time.Second)
if err != nil {
t.Fatal(err)
}
...
}
The test always creates a valid store. We don’t need to handle errors in the test. Instead, we focus only on business logic and improving readability. Big win. For you, your team and whoever will be working on the project in the future.
Are we done? Not yet. In the second part, we will fix other common mistakes:
- Relying on global values in tests
- Duplicating tests and using misleading names
- Changing types in tests