HostDB mocking
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.