Upgrade nodes in CI tests #4029
Conversation
vicsn
left a comment
vicsn
left a comment
There was a problem hiding this comment.
LGTM, let's await Kai's review
| local node_index="$2" | ||
| local role="$3" # "validator" or "client" | ||
| local log_file="$4" | ||
|
|
||
| local flags=( "${common_flags[@]}" "--dev=$node_index" ) |
There was a problem hiding this comment.
We need to be able to support passing in custom flags when new required flags are added in new releases CC #4033
Also, it might be easier to read if we just pass a list of flags to start_node like we do in other tests. Currently it is abstraction on top of abstraction (common flags, role, etc.). CC @kaimast what do you think?
|
Looks good to me! I pushed one more commit with some minor cleanups (83355c6). I also rebased #3902 on it. That PR has some networking changes and the tests succeed. We might still want to test it with a branch that has breaking changes to see that those are caught, or did you do that already? Also, a side note, it would be great if we could keep the number of commits in PRs to a manageable amount. This one has 14 commits, for ~700 LOC. It's not a huge deal, but it can be a pain when reading the commit logs and or when using |
|
@kaimast good idea. You want to give it another go with a toy PR which changes the gateway handshake in a breaking way? |
@meddle0x53 can you try on a toy commit which makes a breaking change to both |
|
Yeah if I update Logs like this, @vicsn |
|
|
||
| upgrade-workflow: | ||
| jobs: | ||
| - upgrade-test |
There was a problem hiding this comment.
Given that it's a bit more expensive and unlikely to be an issue for most PRs, can we make sure to only run it on merges? See for example: ProvableHQ/snarkVM@b5ddc0c
You can also move the verify-windows job in there to only run on merges
3 participants
Motivation
To test upgrading snarkos versions in a network we run a special denvet test as described in #4026
Test Plan
Run the CI and the new test passes.
Documentation
The test works following the steps:
cargo install --locked --path . --features test_networkinto a separate prefix (SNARKOS_RELEASE_DIR).Added a new workflow for the new test, extracting it from the devnet tests, as it is time-taking with all the compilations and waiting on the consensus version.