Something went wrong while fetching comments. Please try again.
Created by: camdencheek
What is this? This is a proposal PR -- it will not be merged as-is. The goal of this PR is to, in a concrete implementation, explore what better database mocking might look like. Our usage of global store handles has been deprecated, and has almost entirely been removed from the codebase at this point, which frees us up to use injected mocks instead of global replacement functions.
I chose to make a PR instead of an RFC because implementation was easy, and I wanted people to see what this would actually look like and be able to leave comments on sections of real code. This PR converts RepoStore to an interface, makes the implementing repoStore unexported, creates a database.DB interface, and converts a couple of tests to use the new mocks. The PR builds and all tests pass.
The current state of things Right now, our database mocks are global functions that are reset and overridden for each test that wishes to use them. This means any tests that depend on a mocked db call cannot be run in parallel.
These functions are created ad-hoc and manually, and only exist if some test needs to override them. There are also a number of ad-hoc methods on the mock store structs to facilitate checking whether the mock was called as well as custom logic that is closely tied to the tests that use them.
Checking call counts isn't often done across the codebase, which makes it difficult to catch stale mocks (the method being tested no longer calls the mocked function). I expect that this is partly because checking call counts is currently somewhat of a pain.
The proposal I propose the following changes to our database mocking infrastructure:
internal/database becomes an interfacego-mockgen
dbutil.DB with an interface representing all our stores so we mock things like db.Repos()
Details Item (1) allows us to swap a mocked store into any function that takes a store by making the public store object an interface. This keeps us from having to add hooks into all our store methods, and also keeps those hooks from getting stale. Additionally, it facilitates things like transparently caching store calls. As a bonus, it makes it really easy to see all the methods a store provides at a glance.
Item (2) forces callers to use the interfaces, and helps enforce a cleaner boundaries by disallowing accessing the fields of the stores directly. It also makes it more difficult to slide backwards if this proposal is implemented incrementally (as soon as a store is converted, making it private keeps old patterns from returning). Additionally, it makes repos only constructible via the exported constructor, which will require an instantiated db object -- this allows us to get rid of all the ensureStore() methods.
Item (3) makes mocking easy. The mocks are generated, and mocking out a method is as easy as repos.ListFunc.SetDefaultReturn(returnVal). In addition, it supports mocking multiple calls, hook functions, call history, and integration with testify. I'm not tied to this specific mock generator, but I've found it really nice to use compared to others, and the primary dev is in house, so that's cool.
Item (4) allows us to sanely mock stores even though we're passing in a generic db object. This way, we can mock db.Repos() to return our mocked repo store without having to resort to global hooks. Additionally, I think it's kinda nice to have our full database interface represented in a single place, but that might just be me. The new interface can embed the current dbutil.DB to facilitate migration and to allow an escape hatch for making direct queries to the db without using a store.
What happens next? I'll leave this up for a bit, let it accrue comments and answer questions as best as I can, then if the consensus is this is a direction we want to go, I make some tickets for the work and knock them out in my down time. I'm OOO until the 25th, so please review by then. I'll measure consensus by engagement with the PR, so if you approve/disapprove, please either leave a comment, a review, or a thumbs up/down.