Chain Upgrade Test #163
Chain Upgrade Test #163
Conversation
hannydevelop
requested review from
jackzampolin,
kkennis and
mvid
as code owners
hannydevelop
requested review from
agouin,
EricBolten and
philipjames44
and removed request for
kkennis
| e2e_build_upgrade_images: e2e_clean_slate | ||
| @docker pull $(ORCHESTRATOR_IMAGE) | ||
| @docker tag $(ORCHESTRATOR_IMAGE) orchestrator:3.1.0 | ||
| @docker build -t sommelier:4.0.1 -f Dockerfile . |
There was a problem hiding this comment.
Nit: I think you meant to use the SOMM_IMAGE (line 410) var here? Something like "SOMM4_IMAGE" may help disambiguate it from SOMMELIER_IMAGE
There was a problem hiding this comment.
Thanks for catching that, line 410 isn't necessary, I'm removing that in the next commit. As you can see, it's not used here, or anywhere.
@docker pull $(ORCHESTRATOR_IMAGE)
@docker tag $(ORCHESTRATOR_IMAGE) orchestrator:3.1.0
@docker build -t sommelier:4.0.1 -f Dockerfile .
@docker pull $(ETHEREUM_IMAGE)
@docker tag $(ETHEREUM_IMAGE) ethereum:3.1.0
@docker pull $(SOMMELIER_IMAGE)
@docker tag $(SOMMELIER_IMAGE) sommelier:3.1.0
I was using it to debug
| hdwallet "github.com/miguelmota/go-ethereum-hdwallet" | ||
| ) | ||
|
|
||
| const DerivationPath = "m/44'/60'/0'/0/0" |
There was a problem hiding this comment.
Nit: maybe name it EthereumDerivationPath so it doesn't get mistakenly used for Cosmos keys in the future?
same for the one in intregation_tests/keys.go
There was a problem hiding this comment.
Hmm, I think for better consistency in somm's integration file as well as gravity-bridge, I'll leave this out for now. Thanks for reviewing though, I appreciate
| if err != nil { | ||
| return true | ||
| } |
There was a problem hiding this comment.
Seems like any code after this until the end of the Eventually block is unreachable since line 168 ensures that err != nil
| }, | ||
| }) | ||
| res, err := s.chain.sendMsgs(*clientCtxs, sendCoin) | ||
| s.T().Logf("Send coin response:%s", res) |
There was a problem hiding this comment.
Perhaps we should only log the response if there is an error? It's a very large object and makes the logs kind of noisy.
There was a problem hiding this comment.
Sure, was thinking about that too. Will do that
| // Submit proposal | ||
| s.T().Log("submit proposal upgrading chain") | ||
| res, err := s.chain.sendMsgs(*clientCtx, proposalMsg) | ||
| s.T().Logf("submit proposal response:%s", res) |
There was a problem hiding this comment.
Same thought here about only logging res on error
| e2e_upgrade: e2e_clean_upgrade_slate | ||
| @E2E_SKIP_CLEANUP=true upgrade_tests/upgrade_tests.test -test.failfast -test.v -test.run UpgradeTestSuite -testify.m TestSommChainUpgrade || make -s fail | ||
|
|
||
| fail: |
There was a problem hiding this comment.
This overwrites another task named fail in the integration tests section, resulting in these logs when running the new e2e commands:
Makefile:445: warning: overriding commands for target `fail'
Makefile:390: warning: ignoring old commands for target `fail'
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Closes: #173
Description
Support testing chain upgrades, potentially by integrating with or using ideas from ibctest. Here's a link to more info on what this test entails.