Add v7 to v8 e2e upgrade test #4591
Conversation
e2e/testsuite/tx.go
Outdated
| @@ -137,10 +137,13 @@ If this is a compatibility test, ensure that the fields are being sanitized in t | |||
|
|
|||
| // ExecuteGovProposalV1 submits a governance proposal using the provided user and message and uses all validators | |||
| // to vote yes on the proposal. It ensures the proposal successfully passes. | |||
| func (s *E2ETestSuite) ExecuteGovProposalV1(ctx context.Context, msg sdk.Msg, chain *cosmos.CosmosChain, user ibc.Wallet, proposalID uint64) { | |||
| func (s *E2ETestSuite) ExecuteGovProposalV1(ctx context.Context, msg sdk.Msg, chain *cosmos.CosmosChain, user ibc.Wallet) { | |||
There was a problem hiding this comment.
small unrelated improvement to not need to manually track proposal count.
chatton
requested review from
colin-axner,
AdityaSripal,
crodriguezvega,
charleenfei,
damiannolan and
DimitrisJim
as code owners
…://github.com/cosmos/ibc-go into cian/issue#4216-add-v7-to-v8-e2e-upgrade-test
| github.com/cometbft/cometbft v0.38.0-rc3 | ||
| github.com/cosmos/cosmos-sdk v0.50.0-rc.0.0.20230911190209-e4033faa38ef | ||
| github.com/cometbft/cometbft v0.38.0 | ||
| github.com/cosmos/cosmos-sdk v0.50.0-rc.0.0.20230913040121-1c9c5ae64ea8 |
There was a problem hiding this comment.
updated to a newer commit which includes the migration fix.
|
TestV6ToV7ChainUpgrade seems to be failing for an unknown reason, I'll create a follow up issue to investigate. |
There was a problem hiding this comment.
Thank you @chatton!!! Leaving my approval. Happy to comment out the v6 -> v7 test if we want to tackle in a followup?
e2e/testsuite/tx.go
Outdated
| proposalID := s.proposalIds[chain.Config().ChainID] | ||
| s.proposalIds[chain.Config().ChainID]++ |
There was a problem hiding this comment.
Should we increment after the tx succeeds? It will be incremented even if submission fails, but then I guess the test should fail?
There was a problem hiding this comment.
good point, yes makes sense to do increment afterwards. In practice I think it will make no difference as the test will fail, but this would be a nice improvement!
There was a problem hiding this comment.
defer func() ?
I reckon its probably fine either as you say, test will likely be failing
There was a problem hiding this comment.
I thought I had already approved when it was draft!
Follow up review looks great! Excellent work @chatton, thank you! 🚀
e2e/testsuite/testsuite.go
Outdated
| @@ -34,6 +34,8 @@ const ( | |||
| type E2ETestSuite struct { | |||
| testifysuite.Suite | |||
|
|
|||
| // proposalIds keeps track of the active proposal ID for each chain. | |||
| proposalIds map[string]uint64 | |||
There was a problem hiding this comment.
maybe the linter cries that this is not proposalIDs ? maybe I cry?
There was a problem hiding this comment.
oh I think you're right 😄
e2e/testsuite/tx.go
Outdated
| proposalID := s.proposalIds[chain.Config().ChainID] | ||
| s.proposalIds[chain.Config().ChainID]++ |
There was a problem hiding this comment.
defer func() ?
I reckon its probably fine either as you say, test will likely be failing
|
merged with v7 still failing, this will be fixed in a followup |
Description
ref: #4216
This PR made also makes a few small adjustments/improvements to the upgrade tests.
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/).godoccomments.Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.