chore: update e2e upgrade workflow and test configuration #5388
Conversation
damiannolan
requested review from
colin-axner,
AdityaSripal,
crodriguezvega,
charleenfei,
chatton and
DimitrisJim
as code owners
| @@ -28,7 +28,7 @@ import ( | |||
| ) | |||
|
|
|||
| const ( | |||
| haltHeight = uint64(350) | |||
| haltHeight = uint64(325) | |||
There was a problem hiding this comment.
whoops, forgot to push 'start a review'
did you figure out why this height change was needed? mainly curious here on why its done 😅
There was a problem hiding this comment.
I just dropped it lower because we were unnecessarily waiting for longer than we needed to. And the test runs long enough! :D
| var allNodes []testutil.ChainHeighter | ||
| for _, node := range chain.Nodes() { | ||
| allNodes = append(allNodes, node) | ||
| } | ||
|
|
||
| testutil.WaitForInSync(ctx, chain, allNodes...) |
There was a problem hiding this comment.
curious as to why other tests weren't running into issues because of this 🤔
There was a problem hiding this comment.
same! I can add this snippet to the other upgrade tests as well, I see no reason why we shouldn't have it 🤷🏻♂️
There was a problem hiding this comment.
In practise node operators will set their halt height or just wait for a node to panic in the upgrade module requesting the new chain binary - so anything that reduces any room for out of sync nodes in the test is good imo!
Description
Now passing in CI with multi validator/node setup: https://github.com/cosmos/ibc-go/actions/runs/7183852859/job/19563569360
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.