Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Fix try-runtime follow-chain, try-runtime upgrade tuple tests, cli test utils #13794

Merged
merged 31 commits into from
Apr 6, 2023

Conversation

liamaharon
Copy link
Contributor

@liamaharon liamaharon commented Mar 31, 2023

@liamaharon liamaharon added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. B1-note_worthy Changes should be noted in the release notes labels Mar 31, 2023
@liamaharon liamaharon added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label Mar 31, 2023
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New tests to prevent this happening again?

@liamaharon liamaharon added the T0-node This PR/Issue is related to the topic “node”. label Apr 1, 2023
@liamaharon liamaharon marked this pull request as draft April 1, 2023 08:30
@liamaharon liamaharon mentioned this pull request Apr 4, 2023
6 tasks
@liamaharon
Copy link
Contributor Author

liamaharon commented Apr 4, 2023

Hey @kianenigma, I've added a test for follow-chain.

The test

  1. Spins up a --dev node child process and waits for it to validate its first block
  2. Spins up a try-runtime follow-chain child process and waits for it to successfully follow at least 3 blocks

Misc extra notes/questions about the changes:

  • I moved cli testing utils from bin/node/cli/tests/common.rs to test-utils/cli/src/lib.rs so they could be shared between node CLI tests and try-runtime CLI tests, and added a new util in there.
  • The existing Command::new(cargo_bin("substrate")) pattern used to spawn a substrate child process seems to use target/debug/substrate. This means that a test could be broken by some changes, but the tests would pass if the project was not re-built. This feels a bit off, I'm wondering if there could be a better approach, to make sure the latest code is always used? edit: found a solution to this

@liamaharon liamaharon marked this pull request as ready for review April 4, 2023 16:34
@liamaharon liamaharon marked this pull request as draft April 5, 2023 06:40
@liamaharon liamaharon marked this pull request as ready for review April 5, 2023 10:59
@liamaharon liamaharon changed the title Fix try-runtime follow-chain, add pre/post runtime upgrade tuple test Fix try-runtime follow-chain, try-runtime upgrade tuple tests, cli test utils Apr 5, 2023
use tokio::process::{Child, Command};

#[tokio::test]
async fn follow_chain_works() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now that we have the infra, should we make tests for all commands? can be an issue that you either do yourself, or mentor.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think we should have tests for subcommands too; otherwise they'll just accidentally break sooner or later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely, it's tracked in this issue: #13796. I wanted to test just one command and get the approach reviewed before adding tests for the rest of them.

There're issues reported with some of the other try-runtime commands that I'm close to getting started on, I'll utilise this test infra while debugging those and leave the tests in the fix PR.

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but would be good if someone from @paritytech/sdk-node also takes a look 👍

test-utils/cli/src/lib.rs Outdated Show resolved Hide resolved
@liamaharon liamaharon merged commit a1d3d50 into master Apr 6, 2023
@liamaharon liamaharon deleted the liam-fix-try-runtime-follow-chain branch April 6, 2023 09:49
breathx pushed a commit to gear-tech/substrate that referenced this pull request Apr 22, 2023
…test utils (paritytech#13794)

* new test for try-runtime tuple stuff

* fix

* remove development comment

* formatting

* remove todo comment

* follow-chain working test

* refactor common cli testing utils

* fix comment

* revert Cargo.lock changes

* update Cargo.lock

* improve doc comment

* fix error typo

* update Cargo.lock

* feature gate try-runtime test

* build_substrate cli test util

* feature gate follow_chain tests

* move fn start_node to test-utils

* improve test pkg name

* use tokio Child and Command

* remove redundant import

* fix ci

* fix ci

* don't leave hanging processes

* improved child process cleanup

* use existing KillChildOnDrop

* remove redundant comment

* Update test-utils/cli/src/lib.rs

Co-authored-by: Koute <[email protected]>

---------

Co-authored-by: kianenigma <[email protected]>
Co-authored-by: Koute <[email protected]>
breathx pushed a commit to gear-tech/substrate that referenced this pull request Apr 22, 2023
…test utils (paritytech#13794)

* new test for try-runtime tuple stuff

* fix

* remove development comment

* formatting

* remove todo comment

* follow-chain working test

* refactor common cli testing utils

* fix comment

* revert Cargo.lock changes

* update Cargo.lock

* improve doc comment

* fix error typo

* update Cargo.lock

* feature gate try-runtime test

* build_substrate cli test util

* feature gate follow_chain tests

* move fn start_node to test-utils

* improve test pkg name

* use tokio Child and Command

* remove redundant import

* fix ci

* fix ci

* don't leave hanging processes

* improved child process cleanup

* use existing KillChildOnDrop

* remove redundant comment

* Update test-utils/cli/src/lib.rs

Co-authored-by: Koute <[email protected]>

---------

Co-authored-by: kianenigma <[email protected]>
Co-authored-by: Koute <[email protected]>
gpestana pushed a commit that referenced this pull request Apr 23, 2023
…test utils (#13794)

* new test for try-runtime tuple stuff

* fix

* remove development comment

* formatting

* remove todo comment

* follow-chain working test

* refactor common cli testing utils

* fix comment

* revert Cargo.lock changes

* update Cargo.lock

* improve doc comment

* fix error typo

* update Cargo.lock

* feature gate try-runtime test

* build_substrate cli test util

* feature gate follow_chain tests

* move fn start_node to test-utils

* improve test pkg name

* use tokio Child and Command

* remove redundant import

* fix ci

* fix ci

* don't leave hanging processes

* improved child process cleanup

* use existing KillChildOnDrop

* remove redundant comment

* Update test-utils/cli/src/lib.rs

Co-authored-by: Koute <[email protected]>

---------

Co-authored-by: kianenigma <[email protected]>
Co-authored-by: Koute <[email protected]>
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/april-updates-for-substrate-and-polkadot-devs/2764/1

nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
…test utils (paritytech#13794)

* new test for try-runtime tuple stuff

* fix

* remove development comment

* formatting

* remove todo comment

* follow-chain working test

* refactor common cli testing utils

* fix comment

* revert Cargo.lock changes

* update Cargo.lock

* improve doc comment

* fix error typo

* update Cargo.lock

* feature gate try-runtime test

* build_substrate cli test util

* feature gate follow_chain tests

* move fn start_node to test-utils

* improve test pkg name

* use tokio Child and Command

* remove redundant import

* fix ci

* fix ci

* don't leave hanging processes

* improved child process cleanup

* use existing KillChildOnDrop

* remove redundant comment

* Update test-utils/cli/src/lib.rs

Co-authored-by: Koute <[email protected]>

---------

Co-authored-by: kianenigma <[email protected]>
Co-authored-by: Koute <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T0-node This PR/Issue is related to the topic “node”.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

try-runtime follow-chain seems broken
4 participants