-
Notifications
You must be signed in to change notification settings - Fork 2.6k
try-runtime-cli: execute-block
& create-snapshot
tests
#14343
try-runtime-cli: execute-block
& create-snapshot
tests
#14343
Conversation
execute-block
& create-snapshot
testsexecute-block
& create-snapshot
tests
@liamaharon Could you review this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, thanks for this!
Co-authored-by: Liam Aharon <liam.aharon@hotmail.com>
Co-authored-by: Liam Aharon <liam.aharon@hotmail.com>
Co-authored-by: Liam Aharon <liam.aharon@hotmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-authored-by: Liam Aharon <liam.aharon@hotmail.com>
Co-authored-by: Liam Aharon <liam.aharon@hotmail.com>
Command::new(cargo_bin("substrate")) | ||
.stdout(process::Stdio::piped()) | ||
.stderr(process::Stdio::piped()) | ||
.args(&["try-runtime", "--runtime=existing"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, the need for --runtime
for this subcommand was a fundamental mistake that I added at some point @liamaharon 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wdyt about existing
always being a default?
.stderr(process::Stdio::piped()) | ||
.args(&["try-runtime", "--runtime=existing"]) | ||
.args(&["execute-block"]) | ||
.args(&["live", format!("--uri={}", ws_url).as_str()]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will only execute the latest block. Would be interesting if you also think about incorporating --at
, if reasonably possible.
Same applies to the other one as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I will do that.
Please let us know once ready for final review. |
@liamaharon I realised that the problem I encountered was not in the In the substrate/utils/frame/try-runtime/cli/src/commands/execute_block.rs Lines 99 to 107 in 6e0059a
|
Hmm.. Intuitively I would also expect |
#[tokio::test] | ||
async fn create_snapshot_works() { | ||
// Build substrate so binaries used in the test use the latest code. | ||
common::build_substrate(&["--features=try-runtime"]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now understood why we have to build again. Just very annoying and slow…
Maybe we can somehow inherit the build args in this function, so that it re-uses the cargo cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be great. It does take quite some time to run all of these tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea that would be a follow up as well; debug that build_substrate
function and check if we can make it re-use the already existing build artifacts by using the same compile args that were used for the test.
@@ -332,6 +332,12 @@ where | |||
); | |||
} | |||
|
|||
frame_support::log::info!( | |||
target: LOG_TARGET, | |||
"try-runtime: Block #{:?} successfully executed", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some ideas, we could do stuff here like:
- State trace (output of all modified storage keys)
- Print all executed calls + Events
- Consumed weight
- Block size etc
Probably most of it in JSON, so we can render it eventually (like chopsticks already does for state changes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be done in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Just some ideas for the future 😄
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Needs to merge master to make the CI happy. |
Going to merge this. We can investigate afterwards if this |
bot merge |
Waiting for commit status. |
…h#14343) * execute-block test * test create-snapshot * oops * Update utils/frame/try-runtime/cli/tests/create_snapshot.rs Co-authored-by: Liam Aharon <liam.aharon@hotmail.com> * Update utils/frame/try-runtime/cli/tests/create_snapshot.rs Co-authored-by: Liam Aharon <liam.aharon@hotmail.com> * Update utils/frame/try-runtime/cli/tests/create_snapshot.rs Co-authored-by: Liam Aharon <liam.aharon@hotmail.com> * remove snapshot * execute block: new log * use prefix & make tempfile a dev dependencie * Update utils/frame/try-runtime/cli/tests/execute_block.rs Co-authored-by: Liam Aharon <liam.aharon@hotmail.com> * Update utils/frame/try-runtime/cli/tests/create_snapshot.rs Co-authored-by: Liam Aharon <liam.aharon@hotmail.com> * ".git/.scripts/commands/fmt/fmt.sh" * --at option in execute-block test * fixes & use --at option in create-snapshot test * hmm * fmt * remove nonsense * Update utils/frame/try-runtime/cli/tests/create_snapshot.rs Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * Update utils/frame/try-runtime/cli/tests/execute_block.rs Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * remove unnecessary test modules * try to load snapshot file * fix --------- Co-authored-by: Liam Aharon <liam.aharon@hotmail.com> Co-authored-by: command-bot <> Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
/tip small |
@kianenigma A small (2 KSM) tip was successfully submitted for @Szegoo (DfqY6XQUSETTszBQ1juocTcG9iiDoXhvq1CoVadBSUqTGJS on kusama). https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Fkusama-rpc.polkadot.io#/referenda |
This PR adds tests for the following
try-runtime
commands:execute-block
create-snapshot
Part of: #13796
Kusama address: DfqY6XQUSETTszBQ1juocTcG9iiDoXhvq1CoVadBSUqTGJS