Ensure metadata is in sync with running node during tests #333

jsdw · 2021-11-26T11:33:10Z

Adds a test-runtime crate with the sole purpose of acquiring metadata from the current available substrate binary, and generating/exposing the subxt node runtime types from it, to use during tests.

This ensures that the metadata we're using is always in sync with the substrate runtime the integration tests are running against.

A next step would be to pick a pinned version of substrate to run during CI that we know we're happy with, and to run a nightly pipeline using latest substrate to spot compatibility breakages.

Closes #329

…r tests

jsdw · 2021-11-26T15:10:06Z

.github/workflows/rust.yml

@@ -70,6 +77,13 @@ jobs:
      - name: Checkout sources
        uses: actions/checkout@v2

+      - name: Download Substrate
+        run: |
+            curl "https://releases.parity.io/substrate/x86_64-debian:stretch/latest/substrate/substrate" --output substrate --location


I don't much like the repetition of this curl thing, but the alternate way of sharing artifacts between jobs works out to be about as verbose...

ascjones

Main thing is, it would be good to remove reqwest since we already have ureq and jsonrpsee http client dependencies in the workspace.

Also, I remember someone in my presentation (@athei or @dvdplm?) suggesting we could even enable the option for the macro to fetch the metadata from the running substrate node, which may be something we can do in the future for this use case.

ascjones · 2021-11-26T15:26:59Z

test-runtime/Cargo.toml

+
+[build-dependencies]
+hex = "0.4.3"
+reqwest = { version = "0.11.6", features = ["blocking", "json"] }


Since we already depend on subxt above we could avoid this extra (another!) http client dependency. Should be able to use subxt::RpcClient

Good point! I'll make use of that or one of the other http deps instead!

Also, I remember someone in my presentation (@athei or @dvdplm?) suggesting we could even enable the option for the macro to fetch the metadata from the running substrate node, which may be something we can do in the future for this use case.

Yeah this was my suggestion. To download the metadata inside the proc macro.

Interesting! I reckon that's best left for a separate PR if we want it. It does strike me that adding the feature to the macro would only really be beneficial for this sort of testing though, so I actually quite like the idea that ithe test specific logic is kept separate and that the macro doesn't become a little more complex to handle it :)

test-runtime/build.rs

ascjones · 2021-11-26T15:42:55Z

test-runtime/build.rs

+}
+
+/// Returns the next open port, or None if no port found in range.
+fn next_open_port() -> Option<u16> {


I wonder whether we could extract TestNodeProcess and other test utils to their own crate. Not only could we reuse the node spawning logic, but we could publish that crate for use by other projects for testing their nodes. I would certainly use it (this code is originally copied from cargo-contract).

Just an idea, don't need to do it here.

I had a very similar thought doing this! I didn't really fancy the amount of churn in this PR, but I can imagine it would be useful!

Will be worth doing, separate PR probably better - must remember to remove the duplication here.

why aren't we just binding to 0.0.0.0:0 instead and letting the OS find the next free port?

That is copied from code I wrote in haste, so most likely not the best way to do it. If that works then let's do it.

Edit, we can probably use that or similar since this code was originally designed for all the tests which spin up in parallel so are all attempting to claim ports

I do like the 0.0.0.0:0 approach normally; the complexity is that you then need to read the actual port used back to connect to the service, so I think I'd need to monitor the stdout (unless you have a better idea; that's what I did in telemetry anyway?).

In the other tests, we need to grab 3 ports by the looks of things, so it is a little trickier!

But yes, in this build script, it's probably a little more robust to scan for a port than rely on any specific stdout from substrate on the matter (but if there's a better way to see which port was used, that'd be great!).

tests/integration/main.rs

ascjones · 2021-11-26T17:47:46Z

test-runtime/build.rs

+            let res =
+                subxt::RpcClient::try_from_url(&format!("http://localhost:{}", port))
+                    .await
+                    .unwrap()


This might panic if it can't connect

Ah yes, I think I took it to be if the url was malformed, and was surprised to then have to move it after the await..

Actually, I did have a perusal through the code, and it's all OK as is (if the url is WS, it tries to establish a connection (hence the await), but if it's HTTP it won't try to connect and will only error if not an http(s) URL).

I'll change to an expect to document this!

yeah, I think we should kill http anyway because it won't work for subscriptions.

ascjones

Other than the unwrap() LGTM

dvdplm · 2021-11-29T08:59:04Z

.github/workflows/rust.yml

@@ -70,6 +77,13 @@ jobs:
      - name: Checkout sources
        uses: actions/checkout@v2

+      - name: Download Substrate
+        run: |
+            curl "https://releases.parity.io/substrate/x86_64-debian:stretch/latest/substrate/substrate" --output substrate --location


dvdplm · 2021-11-29T08:59:43Z

test-runtime/Cargo.toml

+name = "test-runtime"
+version = "0.1.0"
+edition = "2021"
+


A description would be good here.

dvdplm · 2021-11-29T09:00:13Z

test-runtime/README.md

+The logic for this crate exists mainly in the `build.rs` file.
+
+At compile time, this crate will:
+- Spin up a local `substrate` binary (set the `SUBSTRATE_NODE_PATH` env var to point to a custom binary, otehrwise it'll look for `substrate` on your PATH).


Suggested change

- Spin up a local `substrate` binary (set the `SUBSTRATE_NODE_PATH` env var to point to a custom binary, otehrwise it'll look for `substrate` on your PATH).

- Spin up a local `substrate` binary (set the `SUBSTRATE_NODE_PATH` env var to point to a custom binary, otherwise it'll look for `substrate` on your PATH).

dvdplm · 2021-11-29T09:02:20Z

test-runtime/README.md

+
+At compile time, this crate will:
+- Spin up a local `substrate` binary (set the `SUBSTRATE_NODE_PATH` env var to point to a custom binary, otehrwise it'll look for `substrate` on your PATH).
+- Obtain metadata from this node.


Just a thought: perhaps the substrate node should have a sub-command metadata dump /path/to/metadata-dumpfile.scale that does this work? Feels a bit awkward to start a full node and run queries against it.

That's a good idea

I assume that the metadata dump command would itself start the substrate node and such. wrapping this logic into a separate command that's useful on its own does sound like a good idea to me too!

paritytech/substrate#10418

…#333) * add test-runtime crate to dynamically obtain metadata/node_runtime for tests * cargo fmt * Download substrate prior to cargo calls (hopefully) * add README explaining test-runtime * Fix CI, fmt and clippy * more clippy * tweak test-node readme * fmt the clippied * A little tidy up in build.rs * use ureq and raw string * Don't export unnecessary metadata * async_std/RpcClient/bytes instead of ureq/Value/hex * newline * document try_from_url error unwrap

jsdw added 5 commits November 26, 2021 11:29

add test-runtime crate to dynamically obtain metadata/node_runtime fo…

7b2b25c

…r tests

Merge branch 'master' into jsdw-metadata-from-node

fd278c3

cargo fmt

b35d4f0

Download substrate prior to cargo calls (hopefully)

8322f75

add README explaining test-runtime

389fb8a

jsdw marked this pull request as draft November 26, 2021 12:06

Fix CI, fmt and clippy

9fd95f2

jsdw force-pushed the jsdw-metadata-from-node branch from b2f75c7 to 9fd95f2 Compare November 26, 2021 14:27

jsdw added 3 commits November 26, 2021 14:35

more clippy

dd2c9a3

tweak test-node readme

5851a7a

fmt the clippied

69856f0

jsdw requested a review from ascjones November 26, 2021 15:05

jsdw marked this pull request as ready for review November 26, 2021 15:05

jsdw commented Nov 26, 2021

View reviewed changes

A little tidy up in build.rs

b912bb7

ascjones reviewed Nov 26, 2021

View reviewed changes

jsdw added 4 commits November 26, 2021 16:13

use ureq and raw string

128c40e

Don't export unnecessary metadata

f1cf775

async_std/RpcClient/bytes instead of ureq/Value/hex

07af1ce

newline

30b5902

jsdw requested a review from ascjones November 26, 2021 17:25

ascjones reviewed Nov 26, 2021

View reviewed changes

ascjones approved these changes Nov 26, 2021

View reviewed changes

document try_from_url error unwrap

61d2044

niklasad1 approved these changes Nov 29, 2021

View reviewed changes

ascjones merged commit 4b9ee13 into master Nov 29, 2021

ascjones deleted the jsdw-metadata-from-node branch November 29, 2021 09:39

dvdplm reviewed Nov 29, 2021

View reviewed changes

dvdplm mentioned this pull request Dec 3, 2021

[feature request] Add metadata dump subcommand paritytech/substrate#10418

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure metadata is in sync with running node during tests #333

Ensure metadata is in sync with running node during tests #333

jsdw commented Nov 26, 2021 •

edited

Loading

jsdw Nov 26, 2021

dvdplm Nov 29, 2021

ascjones left a comment •

edited

Loading

ascjones Nov 26, 2021

jsdw Nov 26, 2021

athei Nov 26, 2021

jsdw Nov 26, 2021 •

edited

Loading

ascjones Nov 26, 2021

jsdw Nov 26, 2021

ascjones Nov 26, 2021 •

edited

Loading

niklasad1 Nov 26, 2021

ascjones Nov 26, 2021 •

edited

Loading

jsdw Nov 26, 2021 •

edited

Loading

jsdw Nov 26, 2021

ascjones Nov 26, 2021

jsdw Nov 26, 2021

jsdw Nov 26, 2021

niklasad1 Nov 27, 2021

ascjones left a comment

dvdplm Nov 29, 2021

dvdplm Nov 29, 2021

dvdplm Nov 29, 2021

dvdplm Nov 29, 2021

ascjones Nov 30, 2021

jsdw Nov 30, 2021

dvdplm Dec 3, 2021

	- Spin up a local `substrate` binary (set the `SUBSTRATE_NODE_PATH` env var to point to a custom binary, otehrwise it'll look for `substrate` on your PATH).
	- Spin up a local `substrate` binary (set the `SUBSTRATE_NODE_PATH` env var to point to a custom binary, otherwise it'll look for `substrate` on your PATH).

Ensure metadata is in sync with running node during tests #333

Ensure metadata is in sync with running node during tests #333

Conversation

jsdw commented Nov 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ascjones left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw Nov 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ascjones Nov 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ascjones Nov 26, 2021 • edited Loading

Choose a reason for hiding this comment

jsdw Nov 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ascjones left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw commented Nov 26, 2021 •

edited

Loading

ascjones left a comment •

edited

Loading

jsdw Nov 26, 2021 •

edited

Loading

ascjones Nov 26, 2021 •

edited

Loading

ascjones Nov 26, 2021 •

edited

Loading

jsdw Nov 26, 2021 •

edited

Loading