Explain or improve how to test collections

[julienrbrt]

Multiple users complained that with the (partial) migration to collections in v0.50 (Hub, Injective), testing modules that use collections became harder.

The solution we provided was to create keeper wrappers and use that in the modules. That is acceptable for v0.50, as not all modules have migrated to collections. However, with v0.52 coming, the deleted API surface has increased significantly.
However, it appears that we ourselves didn't encounter the problem as we kept the API we needed directly in the modules (check bank API for instance vs staking).

We should possibly go through each module that migrated to collections and bring back so API for compatibility, or provide an example on how to write tests and use ourselves that method (otherwise it's unfair to break people but not ourselves).

cc @MSalopek

[testinginprod]

I think with the current approach there are a lot of problems same with the previous one.

Previously we exposed a lot of methods in the keepers, which in turn exposed state api details and a promise of compatibility and maintainability on that API.

Now we expose the struct with the state directly, which on one side it is better, on the other side it still has the problem that we cannot change the state layout of a module without breaking APIs.

I think the direction should be that the module API surface that we expose should be exposed through queries and messages and the router should be used to communicate with modules.

[facundomedica]

I think the direction should be that the module API surface that we expose should be exposed through queries and messages and the router should be used to communicate with modules.

But we'll still have to show users how to wrap a module if it doesn't expose what they need, right? I guess this is an issue with any software, there are some cases in which a library doesn't expose variables that are important to the developer. In Go we end up using reflection to get those fields, but I think we can have something better here with wrapping

[aaronc]

Could someone give some concrete examples of what specifically is harder to test? i.e. what was someone doing before that they can't do now with a specific module example.

[facundomedica]

Let's say you want to get Proposals, so before we had "GetProposals" and you were able to have an interface with that function. Now we don't have that function anymore, so the only way to get proposals is by having the concrete type. This also makes it very hard or even impossible to mock.

[aaronc]

If GetProposals is part of the public API then it doesn't matter what implementation we use. If we removed it from the public API just because we thought it wasn't needed because that was how we managed state before collections, well that's just poor API design on our part - either we were leaking implementation details into public API (which I think is quite common in the SDK) or we just didn't have a good idea of what the public API should be.

[julienrbrt]

If we removed it from the public API just because we thought it wasn't needed because that was how we managed state before collections, well that's just poor API design on our part - either we were leaking implementation details into public API (which I think is quite common in the SDK) or we just didn't have a good idea of what the public API should be.

That is entirely true, but because those APIs were public, users were using them.
GetProposals is in a way still public, as the collection field is still public, so we actually haven't reduced the API surface, we kept it the same but made it harder to mock.
I think we deleted only what we didn't use but kept what we did (for instance, in bank we have quite much api that is simply a collection wrapper, and we use that in testing in other modules). That broke multiple expected interfaces and test of people.

[MSalopek]

@aaronc
As already mentioned above, the GetProposals method leaked and was exposed to users.

We were using it when testing governance hooks behaviour and it was included in our generated mock helpers.

With the ExpectedKeepers interface pattern, mocking was straightforward as we didn't need to rely on a concrete governance keeper.

With the introduction of collections we now need to iterate the active/inactive proposal queues which are available as struct fields - this means we need to have the concrete struct type available in our tests and cannot use mocks as we used to. This turned most of our hooks unit tests into integration tests.

Changing how tests work is not a huge issue but it does cause issues when upgrading library code as there's usually a testing suite re-write involved.

Tests after the collections introduction (refactored to cosmos-sdk v0.50.x):

• https://github.com/cosmos/interchain-security/blob/main/tests/integration/provider_gov_hooks.go

Tests before the collections introduction (cosmos-sdk v0.45.x):

• https://github.com/cosmos/interchain-security/blob/v3.3.3-lsm/x/ccv/provider/keeper/hooks_test.go#L93
• note the use of MockGovKeeper.

There's nothing wrong with changing the testing approach - integration tests are sometimes a better fit for the situation.

The key point for us is consistency accross modules so testing and usage is predictable and upgrades don't break testing suites.

[kakysha]

Just two cents from me, when faced this exact problem, initially we thought that we will be more tightly coupled between modules, since previously expected keepers was the way to go and separate the interface from implementation, but now we need to have direct dependency on concrete keeper to access it's collection field.

But, in reality, most of the expected keeper method signatures include module's concrete types in either of function arguments or return values anyway, so the impact is not that big.

And yes, I completely agree, relying on a keeper method just because it was public is faulty assumption and should've never been used. Waiting for proto-based inter-module communication.