Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Testing improvements for working with multiple go mods #4527

Open
3 tasks
chatton opened this issue Aug 31, 2023 · 3 comments
Open
3 tasks

Testing improvements for working with multiple go mods #4527

chatton opened this issue Aug 31, 2023 · 3 comments
Assignees
Labels
testing Testing package and unit/integration tests type: refactor Architecture, code or CI improvements that may or may not tackle technical debt.

Comments

@chatton
Copy link
Contributor

chatton commented Aug 31, 2023

The Problem

When we wanted to update our simapp to use callbacks/wasm functionality, this ended up resulting in circular imports between ibc-go and callbacks/wasm. In order to get our tests working, a temporary workaround was to duplicate the SimApp and add the callbacks/wasm specific logic to the SimApps in the separate go.mods.

This issue outlines some thoughts and suggestions for possible improvements we could make to our testing set up that could make it more flexible to allow for testing without fully duplicating the SimApp in each go module.

Tests importing the simapp package

importing simapp.Setup

Currently, a lot of tests use simapp.Setup(t, false). Example here

In a lot of these cases, we don't actually need this full functionality, this PR has some examples of how we can strip this out.

The GetSimApp() function

This is one of the biggest issues we need to deal with. this function is used in almost every one of our tests. We assume we have a concrete instance of simapp.SimApp that we are dealing with. This is true without tests in ibc-go but as sub moduiles are introduced, this becomes a a problem as we want to be able to write tests without using the same SimApp instances.

Possible solution:

Change the TestChain type to look like

type TestChain[T TestingApp] struct {
  // ...
    App           T
  // ...
}

// returns concrete type specified at TestChain creation
func (chain *TestChain[T]) GetSimApp() T {
    return chain.App
}

// chain := TestChain[*simapp.SimApp]{...}
// chain := TestChain[*callbacks.SimApp]{...}
// chain := TestChain[*wasm.SimApp]{...}

// simapp := chain.GetSimApp()

This would allow other modules to use the TestChain type, but during instantiation, we would specify the concrete App type, that must still implement the TestingApp interface.

Tools for building a simapp, living in the testing library (within the ibc-go go.mod)

Instead of having a separate SimApp per module, we would have a modified version of one SimApp per module. A function could exist within ibc-go/testing, which makes it possible to dynamically build SimApps. Note: this approach would mean we don't actually need to do the generic approach mentioned above, however it could still be benefital for code that imports the testing library and does not wish to use our test simapps.

The following code is just an example of what usages might look look.

simapp := ibctesting.NewSimApp().
    ModifyIBCRouter(func(router *Router){
        // wire up new modules
    }).
    ModifyModuleManager(func(mm *Manager){
      // change the module manager
    }).
    ModifyApp(func(app *SimApp){
      // change anything
    }).
    Build()

ibctesting.NewSimApp().Build() would return a functional simapp, that has all core ibc functionality wired up, which is usable in our E2E tests. (this could start with everything that currently lives in the same go.mod, so transfer, fee and ICA).

Other go mods would import this from ibc-go/testing, and then apply any additional changes needed to wire up additional features relevant to the module. Each of these other modules would have a separate dockerfile/main.go and have a corresponding docker image for the relevant E2E tests.

This would have the added benefit of being able to run fewer E2Es per PR. Having seperate smaller E2E test suites per simapp should result in fewer tests per PR, instead of having one mega SimApp that has all features.

Note: a brief attempt at this was done, but we ran into some issues, specifically when the transfer keeper was a value instead of reference type. This meant any modifications made after the fact were not applied. If we want to take this approach, we would need to ensure that it is possible to modify any existing fields and the updates will take effect. Making these keepers reference types seems to be the most straightforward way of doing this, but will be api breaking.

SimApp in separate go.mod?

If we like the approaches outlined in this issue, I don't think we actually need to put the ibc-go/SimApp in a separate go.mod. We would instead be splitting out functionality and features across several go.mods, with core functionality remaining the ibc-go/SimApp, and it being extended in additional modules.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@chatton chatton added needs discussion Issues that need discussion before they can be worked on testing Testing package and unit/integration tests type: refactor Architecture, code or CI improvements that may or may not tackle technical debt. labels Aug 31, 2023
@srdtrk
Copy link
Member

srdtrk commented Sep 5, 2023

What would be the workflow for say testing callbacks middleware with wasmd (after they integrate)? Callbacks couldn't import wasmd as this would be a circular dependency.

@chatton
Copy link
Contributor Author

chatton commented Sep 5, 2023

@srdtrk, you're talking about needing a simapp that is importing both callbacks and wasmd. Yeah that's a good point, we would need to have a separate go mod containing to pull in everything (wasmd, callbacks, ibc-go) into a single simapp to make that work.

The separate simapp for a gomod might end up being the most practical solution.

If we opted for one simapp with everything configured, there may be some difficulties testing things with in-development features.

@colin-axner colin-axner removed the needs discussion Issues that need discussion before they can be worked on label Jan 15, 2024
@chatton
Copy link
Contributor Author

chatton commented Jan 16, 2024

Next step:

Try and create a Simapp Builder and refactor the existing Wasm Simapp to use it and get current tests passing. After this we can see if the same approach will be useful across the board.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Testing package and unit/integration tests type: refactor Architecture, code or CI improvements that may or may not tackle technical debt.
Projects
Status: Backlog 🕐
Development

No branches or pull requests

4 participants