IMO if we are to have useful tests (or even one), we should think more carefully about what we mock.

For example, I don't think mocking repostore is necessary - it can be easily created from test DB, and it'd be good to pre-seed it with actual data: https://github.com/sourcegraph/sourcegraph/blob/main/internal/codeintel/syntactic_indexing/internal/testutils/testutils.go#L15

This will aid debugging greatly, and reduce the amount of mocked code we run through.

Of the things you mock here I would only mock gitserver, as it relies on external state and network to clone repositories - the rest I believe can be constructed using available New* methods.

The process of filling out those constructors is very annoying but as this PR stands I think it will be hard to maintain and doesn't actually give much confidence in tests.

I looked at the testutils you linked, but that seems like a bad idea? By writing INSERT statements you're actually tying the test to the concrete database schema, when the whole point of the Repo pattern is to abstract from that, so that you can test a service against just the Repo interface and evolve the underlying schema if you have to.

FTR I don't think the mockRepoStore in here is the problem, because all we're mocking is a single GetByID call which is very unlikely to break in the future.
The mocked gitserver and lsifstore are more problematic because they're mocking things 4-5 calls deep from getSearchBasedUsages in service.go.

Well the actual way the database state is set up is not the important bit here. The important bit is that we use service implementations that interact with that state.

If it's easier to rewrite the InsertRepo in terms of just purely using RepoStore - that's great, as long as when the tests for this code run they actually use the real RepoStore talking to the real database.

when the whole point of the Repo pattern is to abstract from that, so that you can test a service against just the Repo interface and

Despite us using SQL to hydrate the state, the code under test still goes through the RepoStore interface. If the SQL scripts are broken and violate Repo's own internal logic, it will show up in the tests.

Using SQL helps circumvent the potentially much larger "full" state that RepoStore requires for each of its methods – but if we can get rid of it, that'd be even better.

I would suggest starting with setting up lsifstore – gitserver will be much more annoying to fake

note to our convo about testing libraries - in a lot of other code this block would become require.NoError(t, searchErr)

Builder warms my crusty JVM-infested heart