Skip to content

HostDB mocking

Luke Champine requested to merge hostdb into master

This PR serves to showcase what extensive mocking would look like in a module. It's not perfect (comments/naming/organization could be improved), but it's more-or-less how I envisioned it. Nearly all of the hostdb's dependencies have been mocked. (The exceptions are: filepath.Join, sync.RWMutex, os.MkdirAll, crypto.Perm, rand.Int, time.Now, types.Currency, and possibly one or two others that I missed. These are pretty inconsequential, since most do not return errors.)

The New/newHostDB pattern in particular has made it very easy to unit test the hostdb's methods. Most of our module tests have fully initialize the modules, meaning they spawn goroutines, write to disk, mine blocks, etc., all just to test a fairly simple function. By testing with newHostDB (which takes all dependencies as parameters and has no side effects) instead of New, it becomes trivial to pass in only the dependencies that will be called and mock them accordingly. See the newly-added tests for examples.

Finally, @VoidingWarranties and I talked in person about modifying this code to reduce duplication of types. The basic premise is to define shared "stub" types for each module in the modules package instead defining "restricted interfaces" internal to each module. As an example, the hostdb now defines the following type:

hdbTransactionPool interface {
    AcceptTransactionSet([]types.Transaction) error
}

which is a parameter to newHostDB. Since modules.TransactionPool implements hdbTransactionPool, it can be passed to newHostDB. And during testing, tests which require AcceptTransactionSet must define a type that implements hdbTransactionPool. In Jordan's proposal, we would instead define a stub type for the transaction pool like so:

// in modules/transactionpool.go
type TransactionPoolStub struct{}
func (TransactionPoolStub) AcceptTransactionSet([]types.Transaction) error { return nil }
func (TransactionPoolStub) IsStandardTransaction(types.Transaction) error  { return nil }
func (TransactionPoolStub) PurgeTransactionPool()                          { }
// ... etc

then, when testing the hostdb, we would define our testing type like so:

type tpoolTest struct {
    modules.TransactionPoolStub
}
func (tpoolTest) AcceptTransactionSet([]types.Transaction) error {
    // ... test-specific logic ...
}

And finally, we would throw away the hdbTransactionPool type, and have newHostDB take a modules.TransactionPool instead. Since tpoolTest embeds modules.TransactionPoolStub, it inherits its methods and thus implements the full modules.TransactionPool interface. We can then override any methods as needed during testing. Part of the rationale here is that we will likely want to define shared stubs anyway, for dependencies like time.Sleep and net.Dial.

The code reduction is compelling, but something doesn't sit quite right with me about this approach. I prefer to pass modules the smallest interface they require. Also, embedding types all over the place may also be confusing or error-prone. I guess my primary objection is that I'm averse to writing generic code until at least two specific cases exist. I feel that we should adopt the pattern in this PR for now, and consider abstracting it only after we've mocked another module to a similar degree. It will be easier to see the parallel and evaluate what makes sense once the code is on the page instead of in our heads.

Merge request reports