Testing expected / unreachable errors?

I'm trying familiarise myself more with testing in Go, and have come up against only one problem so far: testing for errors.

There are plenty of times it seems, when using stdlib functions for things like IO where you know an error will occur, but you can't realistically trigger it in tests without some kind of mock that forces an error to happen.

My thoughts so far on this are basically that coverage is not important, testing things that are important to test is important. But where do you draw the line? Is a permissions error that would be very difficult to test, worth testing by mocking?

To add more context to this, I'm using the os, archive/tar, and compress/gzip packages to create .tar.gz files programmatically. Nothing more specific than that just yet, but I am also trying to test the whole thing. There are certain parts (like the aforementioned potential permissions issue) that I can't trigger in reality, but using an interface that wraps the calls to os that I use I can intercept the call, and force an error to occur under certain circumstances with a particular implementation of that interface.

So, I suppose I have a couple of questions:

• What is important to test here in your opinions?
• If there is an error that I can only trigger by using some kind of mock, wrapped interface, should I be doing that? Or is that overkill?

---

评论：

jerf:

"Mock" is the wrong conceptualization. You really can't "mock" things in Go the way you do in other languages. What you do is declare an interface that does the things you want to do, then in your test code provide degenerate implementations of that interface. For instance, one I've used multiple times is:

type BadWriter struct {
    err error
}
func (bw BadWriter) Write(p []byte) (n int, err error) {
    return 0, bw.err
}

Variants can be written on that basic idea.
The other thing that you sometimes have to look out for is that you don't end up excessively specific in your own code. For instance, you mention that you're using compress/gzip; despite the fact that it hands you back a *gzip.Reader, you often want to interact with it just as an io.Reader, precisely so you can dependency-inject your way into using something else.
Another trick I often use is to split functions into two pieces, one that deals with all the messy setup code and one that does the "real work" strictly through interfaces (or through things I can easily inject, like integers or strings), for example:
func DoStuffWithSockets(addr string) error {
    conn, err := net.Dial("tcp", addr)
    if err {
        return err
    }
    // set timeout on the socket here
    r, err := gzip.NewReader(conn)
    if err {
        return err
    }
    return actuallyDoStuff(r)
}
// taking io.Reader is critical here
func actuallyDoStuff(r io.Reader) error {
    // now actually read from the socket and do all your parsing, etc.
}

Your test code can then test actuallyDoStuff using a byte.Buffer, and test all the tricky parsing code, and if you want to leave DoStuffWithSockets untested in unit testing code and just check it during later testing, you're justified. That's not really the stuff that screws up, it's usually the parsing of what comes in and doing the right thing that screws up. If you make the actuallyDoStuff private, as I did, it's virtually guaranteed to be inlined in production code, so it doesn't even cost you a function call.
Elaborating on this particular case just to continue the example, if you're dealing with sockets you probably ought to be using the timeout function. But if you try to take the full net.Conn interface, you're in for a bad time. So you just declare your own interface:
type ReadableSocket interface {
    io.Reader
    SetDeadline(t time.Time) error
}
func actuallyDoStuff(r ReadableSocket) error { ... }

and then provide yourself a wrapper
type MakeReadable struct {
    io.Reader
}
func (mr MakeReadable) SetDeadline(time.Time) error {
    return nil
}

And now, once again, you can test actuallyDoStuff with a byte.Buffer. If you want, you can also test that the deadlines are set correctly by adding more stuff, though you may choose to leave that undone. You can also inject the previous BadWriter I implemented, wrapped with MakeReadable, to simulate timeout deadline errors.
Eventually you just get fluent with all this stuff, I think. How far you want to go is up to you. There's only a bare handful of things that truly can't be mocked. Though you may find for a couple of things like filesystem manipulation or time manipulation you want to pick up a library that somebody has already written. (Time in particular I recommend that for.)

SeerUD:

This is an excellent write-up. Thank you for taking the time to do so!
The compress/gzip usage is only for writing at present (though I may expand to reading in the future).
Did you get a chance to look at the files I linked in another comment? They show how I'm handling the interfacing and abstraction at the moment - it might be close to what you mean here. I feel like I've been unable so far to use more generic interfaces like io.Reader / io.Writer / io.Closer (and the combinations) effectively, because of methods like Writer.WriteHeader, but perhaps I'm just trying to abstract this at the wrong point...

jerf:

In your own code, with your own objects, you can always replace a use of some object with an interface. Even statically-accessed values can be routed through a "getter" instead.[1]
Where you do end up in some trouble is if a foreign library absolutely insists on taking its own type, and you're stuck in your test code either setting up whatever that expects, or skipping the test. For file system stuff, I tend to go with the former, grabbing a temporary directory and mangling it with whatever I need and deleting it with a "defer" to ensure I don't miss it. For most other things, I do the latter. In one case I cracked open the target library and made a local modification to make it take an interface, though. Because, per my first paragraph, a library can always be made to take an interface instead of a local concrete instance.
I like external libraries that come via source, and while I try not to modify them at the drop of a hat because that causes its own problems, it is OK to sometimes reach in and hack them to do something else you need. With just a bit of care and git knowhow, it's not even necessarily that hard to stay up-to-date with minor release changes.
[1]: "Getter" can get a bit of a bad rap. One pattern I've had a few times is that something will have a "Name() string" method of some sort, and in many implementation that will be a "pure getter" that just "return obj.Name"s. But then I always seem to end up with an implementation that does something more complicated. So, anyhow, don't be afraid to put the odd "getter" into your interfaces.

jeffrallen:

+1 for hacking dependencies until they are testable. But please get the upstream to take your changes so that we call win.

stone_henge:

You can use interfaces to build very testable code. In your case, archive/tar and compress/gzip use io.Writer which means that you can get very far by just passing your code a simple Writer implementation in your test cases, for example to generate an error immediately or after n writes.
You shouldn't be unit testing the standard library code, though, so in your case I would only use it to verify that errors are propagated and ultimately handled cleanly. If you have a particular strategy for dealing with the permissions error, you should of course make sure that you test that, but in some cases it is OK or even preferable to deal with error cases more generally. For some applications, just logging and exiting is a viable general error handling strategy.

SeerUD:

Alright, that sounds sensible.
For tarring, at the moment I've abstracted that away behind another interface and a "factory" to build that interface. I'm not sure how horrible that sounds, but it has greatly simplified the code where it is being used.
This is that file as it stands: https://gist.github.com/SeerUK/97c438b778a3e39dcd07333c65b5a926
This is an example of where it's used: https://gist.github.com/SeerUK/58c1d5d5545b104495e5e859b0ec451f
So, you can see I've already created that wrapper for the filesystem, I suppose this is a similar thing I'm doing now with archiving. Though in this case, the abstraction does feel a bit more useful, because I could make it possible to choose an archive format easier this way and share the interface.
That function in the second file is totally covered by tests now, and I believe it should be easier now to test the tarring in isolation, as you'd mentioned, potentially by using a different writer. I believe the tar.Writer type does have some special methods on for writing the header however...
I just wanted to make sure I'm not going insanely over-the-top by making these interfaces and abstractions for what feels a little bit like the sake of coverage.

stone_henge:

It seems like you could test much of that code by having Build take an io. Writer instead of an os.File and defining an interface for the minimal surface of xos.FileSystem you use, making test implementations for those. Then, what is now a File and a FileSystem can easily be implementations that are specific to your test suite.

SeerUD:

I may do that with os.File, but that is indeed what I'm already doing with xos.FileSystem, the interface is already only what is used for these things, and there is an implementation that I made that I can attach functions to that intercept the calls to mess with the data or return errors specifically at points - otherwise just transparently calling the real os-backed implementation.
I'll definitely take a look at replacing os.File with an io.WriteCloser (and maybe use io.ReadCloser for files being archived).

stone_henge:

It doesn't look like you actually need a WriteCloser on that level of abstraction; you only ever use the Writer functionality on the os.File in Build, immediately passing it into the tarball writer. You only Close the actual file in the caller. I think it's correct to only let the caller close the stream, since it is also responsible for opening it.
As for comments on the level of abstraction, I think it's easy to understand the intent and follow the code, but instead of using a factory interface I'd prefer a slice of builder+extension pairs indexed by an identifying enumerator. That obviates creating empty struct types like TarGzArtifactFactory for each artifact type and implementing the Extension method, while clearly listing the Artifact backends and their respective extensions in one place.

SeerUD:

Aah yes, fair point about closing! :)
I see what you mean mostly by the slice of builder+extension pairs, just not what that pairing part would look like.
I guess the rest would look something like:
type ArtifactFormat int
const (
    TarGz ArtifactFormat = iota
    Zip
    Stub
)
var formats = [...]FormatPair{ // ???
    // ???
}

This looks sort of like how months and weekdays are handled in the time package. Is that the sort of thing you mean?
I do like where this is going, because it would mean the resulting method call on the Compressor would look something like this from outside of the package:
compressor := archive.NewDefaultCompressor()
compressor.Dirf(path, "backup-%s-%d", archive.TarGz)
// Or maybe...
compressor := archive.NewDefaultCompressor(archive.TarGz)
compressor.Dirf(path, "backup-%s-%d")

Edit: Do you mean something like this? I'm not too sure about it:
type ArtifactBuilderFunc func(xos.FileSystem, *os.File) Artifact
type ArtifactBuilder int
const (
    TarGz ArtifactBuilder = iota
    Stub
)
var builders = [...]struct {
    ext string
    fn ArtifactBuilderFunc
}{
    {
        "tar.gz",
        func(fs xos.FileSystem, file *os.File) Artifact {
            gw := gzip.NewWriter(file)
            tw := tar.NewWriter(gw)
            return &TarGzArtifact{
                fs: fs,
                gw: gw,
                tw: tw,
            }
        },
    },
}
var tarGzBuilder = builders[TarGz].fn(/* ... */)

Though, this is a little trickier still if you want to access the builders outside of the package. Maybe that is just something I don't need to be concerned about though...

stone_henge:

Something like that indeed, though I would still define the builders outside the array initializer. I'm not sure that I understand your access concern, but I would just make builders public.
One nasty effect of this approach is that you need to make sure that the indices match the enum values manually. There's nothing like designated initializers in C99, where you can name indices during initialization and present them in any order. This can be solved by making the array a slice and initialize it in a function, or you can use a map instead.

davebrophy:

My theory goes that you should only enforce test coverage where the error actually originates from. When you're just returning an error from another function, you should assume that function is properly tested. That's why I created courtney, which:

Runs tests in multiple packages with coverage
Merges the coverage files
Excludes blocks with panics
Excludes blocks that return an error that has been tested to be non-nil (see the readme for more details about how this works)
Excludes blocks annotated with a "notest" comment

What you're left with is the really critical code-paths, and once you reach 100% code coverage you can add the -e (enforce) flag to the courtney command - this will throw an error if there's any code without coverage.
Let me know what you think: https://github.com/dave/courtney

SeerUD:

This does look very useful! I must admit, I still wouldn't be able to shake the feeling that I'd be sort of cheating by doing this. It's strange though because when you do think about how errors are handled in other languages, with things like exceptions, you don't have to "cover" those in your tests when they're being propagated up...
I'll have to give this some thought, but I tried it out and it works great. It's very nice having all of the packages' coverage reports in one place!

davebrophy:

when you do think about how errors are handled in other languages, with things like exceptions, you don't have to "cover" those in your tests when they're being propagated up

Exactly!

davebrophy:

Here's an example of a bunch of error returns being excluded by courtney: https://codecov.io/gh/dave/jennifer/src/master/jen/comments.go#L77

SeerUD:

I did have a quick poke around on the coverage report I'd seen there, it looks good! I've made an issue for one UX thing I've found, it'd be great to have your input on it.

Tikiatua:

Hi there,
We have a similar problem with lots of error handlers using the package github.com/neelance/graphql-go to provide a graphql endpoint. 
Most of the error conditions are pretty hard to simulate. Writing those tests will take up a lot of time and not provide too much value in the end. However, it would be nice, if the code coverage would not stay around 70% only because this errors are not tested. 
Therefore we are thinking about writing a tool that can work in conjunction with the coverage file output by go test. The idea would be to be able to markup specific blocks that should be exempt from coverage testing, similar to the nolint statements for go meta linter.
A "nocover" statement would exclude the following line respective block from the coverage report or indicate the code separately as explicitly not covered.
func doSometing() {
   result, err := someFnThatAlmostNeverReturnsAnError()
   // nocover
   if err != nil {
      fmt.Println("there was an error")
   }
}