refactor: move upgrade unit tests together #3302
Conversation
ninabarbakadze
force-pushed
the
nina/move-ica-tests
branch
from
68641f4 to
b1ce2ca
Compare
ninabarbakadze
changed the title
ninabarbakadze
requested review from
rootulp and
cmwaters
and removed request for
a team
WalkthroughWalkthroughThe recent updates focus on enhancing test configurations for application upgrades by introducing a utility function for setting up test environments. New tests have been added to assess the behavior of the application during version upgrades, specifically targeting minimum fees, Interchain Accounts (ICA), and packet forwarding. The restructuring also involves cleaning up obsolete code and imports, streamlining the setup process across various test modules. Changes
Recent Review DetailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
test/util/testapp_upgrade.go
Outdated
| @@ -0,0 +1,56 @@ | |||
| package util | |||
There was a problem hiding this comment.
[not blocking] FYI: https://dave.cheney.net/2019/01/08/avoid-package-names-like-base-util-or-common
test/util/testapp_upgrade.go
Outdated
| dbm "github.com/tendermint/tm-db" | ||
| ) | ||
|
|
||
| func SetupTestApp(t *testing.T, upgradeHeight int64) (*app.App, keyring.Keyring) { |
There was a problem hiding this comment.
nit:
| func SetupTestApp(t *testing.T, upgradeHeight int64) (*app.App, keyring.Keyring) { | |
| func SetupTestAppWithUpgradeHeight(t *testing.T, upgradeHeight int64) (*app.App, keyring.Keyring) { |
celestia-bot
requested review from
a team,
ramin and
rootulp
and removed request for
a team
ninabarbakadze
changed the title
There was a problem hiding this comment.
You're still repeating yourself a lot here. This kind of pattern lends itself to table based testing.
testCases := []struct {
module string
preUpgradeAssertion func (t *testing.T, app *app.App)
postUpgradeAssertion func(t *testing.T, app *app.App)
}{
{
module: "signal",
preUpgradeAssertion: func(t *testing.T, app *app.App, ) {
// add assertions about how this module doesn't yet exist
},
postUpgradeAssertion: func(t *testing.T, app *app.App) {
// add assertions about how this module now exists
},
},
}
// start up the testnet
// do the pre upgrade assertions for all testCases
// upgrade the application and check it succeeded
// do all the post upgrade assertions for all testCasesAlso don't forget that when a test errors that it tells the caller which module failed and at which stage
celestia-bot
requested review from
a team and
cmwaters
and removed request for
a team
celestia-bot
requested review from
a team and
rach-id
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
scripts/single-node.sh
Outdated
| # Persist ABCI responses | ||
| sed -i'.bak' 's#discard_abci_responses = true#discard_abci_responses = false#g' "${CELESTIA_APP_HOME}"/config/config.toml | ||
|
|
There was a problem hiding this comment.
Tip
Codebase Verification
The verification process did not find any tests specifically addressing the handling of ABCI responses in the shell scripts. This supports the initial review comment's emphasis on the need for thorough testing and documentation of the changes made to persist ABCI responses. It is crucial to ensure that these changes are well-tested and documented, particularly regarding their impact on node performance and storage requirements.
Analysis chain
Persisting ABCI responses by setting discard_abci_responses to false is a significant change. Ensure that this behavior is thoroughly tested and documented, especially regarding its impact on node performance and storage requirements.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for tests related to ABCI response handling.
rg --type sh 'ABCI response'
Length of output: 78
app/validate_txs.go
Outdated
| // FilterTxs applies the antehandler to all proposed transactions and removes | ||
| // transactions that return an error. | ||
| // |
There was a problem hiding this comment.
Tip
Codebase Verification
The investigation into the FilterTxs function and its associated tests reveals that while the function is used in the test scenario within out_of_order_prepare.go, there is no explicit evidence of tests specifically designed to verify the new functionalities of handling errors and arranging transactions as described in the review comment. It is recommended to create targeted tests to ensure these functionalities work as intended and do not introduce any unintended side effects.
Analysis chain
The updates to the FilterTxs function to apply an antehandler to all proposed transactions, remove transactions that return an error, and arrange transactions improve the robustness of transaction handling. Ensure that these changes are thoroughly tested to confirm that they work as expected and do not introduce any unintended side effects.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for tests covering the new transaction handling in FilterTxs.
rg --type go 'FilterTxs' | grep 'test'
Length of output: 167
Script:
#!/bin/bash
# Search for detailed test cases specifically covering the new functionalities in FilterTxs.
rg --type go -C 10 'FilterTxs' test/
Length of output: 1689
ninabarbakadze
force-pushed
the
nina/move-ica-tests
branch
from
c1a6cec to
915a5fb
Compare
Overview
Moves all upgrade unit tests in one place