Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

speed up ui tests. #944

Merged
merged 27 commits into from
May 12, 2023
Merged

speed up ui tests. #944

merged 27 commits into from
May 12, 2023

Conversation

tadeohepperle
Copy link
Contributor

@tadeohepperle tadeohepperle commented May 2, 2023

Partly fixes #877
Three changes were added here to speed up the tests:

  • run UI tests only for a few pallets not for all
  • add 2 smaller versions of the polkadot metadata and use them where appropriate
  • use nextest that parallelizes testing across and within crates

Here are some runtime results from 4 runs on the master and the PR branch.
"All Other Tests" encompasses all tests except the wasm tests.
"UI Tests" is only the UI tests alone for the master branch. It is the job that takes most time and is a lower limit for the runtime of all non-wasm tests, if they run in parallel.
We can see that the speedup is at least about 30% (potentially much more, because all tests are compared to only the UI test job here)

The rows Test Wasm and Check Docs are added as control variables to see that nothing really changed there. Nextest does not do doc tests, so that stays the same.

With Speed Up Changes

  Run 1 Run 2 Run 3 Run 4 Mean Std-Deviation
Check Docs 0:08:39 0:05:18 0:07:15 0:11:23 0:08:09 0:02:34
All Other Tests 0:08:06 0:10:32 0:08:06 0:07:46 0:08:38 0:01:17
Test Wasm 0:03:05 0:02:42 0:03:50 0:03:06 0:03:11 0:00:28

Master

  Run 1 Run 2 Run 3 Run 4 Mean Std-Deviation
Check Docs 0:06:41 0:05:15 0:06:21 0:13:07 0:07:51 0:03:34
UI Tests 0:12:43 0:13:11 0:09:56 0:10:55 0:11:41 0:01:31
Test Wasm 0:03:36 0:03:04 0:03:35 0:04:42 0:03:44 0:00:41

@tadeohepperle tadeohepperle force-pushed the tadeo-hepperle-speed-up-tests branch from 7a91f00 to a466d4d Compare May 4, 2023 11:09
tadeohepperle and others added 12 commits May 4, 2023 13:35
* WIP updating to syn 2.0.0

* WIP darling compat

* Update darling and syn workspace deps

* NestedMeta::parse_meta_list

* Rename attribute keyword type property to path

* Fmt

* Update more type to path

* Unused darling

* Cargo.lock

* Add missing syn features
* Update frame-metadata to v15.1.0

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* Enable V15 unstable metadata in frame-metadata

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* metadata: Move validation hashing to dedicated file

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* Use sp-metadata-ir from substrate to work with metadata

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* Revert using sp-metadata-ir in favor of conversion to v15

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* metadata: Convert v14 to v15

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* metadata: Use v15 for validation

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* codegen: Use v15 for codegen

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* metadata/bench: Use v15

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* Adjust to v15 metadata

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* Adjust testing

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* Improve documentation

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* force CI

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* rpc: Fetch metadata at version

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* artifacts: Update polkadot.scale from commit 6dc9e84dde2

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* codegen: Fetch V15 using the new API

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* codegen: Add runtime API interface

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* metadata: Hash runtime API metadata for validation

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* metadata: Extract runtime API metadata wrapper from subxt::Metadata

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* subxt: Adjust hashing cache to reflect root+item keys

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* rpc: Add raw state_call API method

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* runtime_api: Add payload with static and dynamic variants

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* subxt: Allow payloads to call into the runtime

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* examples: Add example to make a runtime API call both static and dynamic

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* Update polkadot.rs

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* codegen: Simplify client fetching

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* Address feedback and fallback to old API if needed

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* runtime_api: Make mutability conditional on input params

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* Regenerate polkadot.rs

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* metadata: Retain only pallets without runtime API info

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* codegen: Retry via `Metadata_metadata` without conversion

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* payload: Remove `Decode` and change validation fn

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* metadata: Retain runtime API types

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* codegen: Runtime APIs documentation based on flag

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* Update examples/examples/custom_metadata_url.rs

Co-authored-by: James Wilson <james@jsdw.me>

* Update artifacts from polkadot-a6cfdb16e9

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* Update polkadot.rs with polkadot-a6cfdb16e9

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* codegen: Generate input structures for runtime API

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* runtime_api: Remove the static paylaod and use single impl

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* examples: Fetch account nonce

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* testing: Adjust build script to fetch latest metadata

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* testing: Check account nonce from runtime API

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* Update cargo.lock

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* codegen: Fix doc generation for runtime types

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* codegen: Rename `inputs` runtime calls module to `types`

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* codegen: Generate Calls structs inside the types module

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* testing: Check Alice account nonce before submitting the tx

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* cli: Add metadata version option flag supporting v14 and unstable

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* cli: Specify version to fetch

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* subxt: Fallback to fetching latest stable metadata

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* subxt: Add unstable-metadata feature to fetch the latest

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* RuntimeVersion with Latest and Version(u32)

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* Update polkadot.rs

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* codegen: Adjust fetch_metadata to inspect version list

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* testing: Adjust metadata to metadata_legacy

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* events: Adjust docs to use metadata_legacy

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* have a pass over fetch_metadata

* cargo fmt

* Option<String> when fetch metadata via latest API

* clippy

* fmt

* cli: Use the MetadataVersion from codegen

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* cli: Specify latest as default for MetadataVersion

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* cli: Remove version from metadata and use the one from file_or_url

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* Fix clippy

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* codegen: Decode metadata independently for different RPC calls

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: James Wilson <james@jsdw.me>
#947)

* remove validation hash where not wanted

* .into()

* fix example

* cargo fmt
* WIP Starting to write book; extrinsics first pass done

* cargo fmt

* Ongoing work; events, constants, wip blocks

* at_latest() and wip blocks

* remove need to import parity-scale-codec crate with Subxt for macro to work

* More docs; expanding on setup guide and finish pass of main sections

* Tidy and remove example section for now

* format book lines to 100chars

* Fix example code

* cargo fmt

* cargo fmt

* fix example

* Fix typos

* fix broken doc links, pub mods

* Update Subxt macro docs

* can't link to Subxt here

* move macro docs to Subxt to make linking better and fix example code

* note on macro about docs

* cargo fmt

* document the no_default_derives macro feature

* Address feedback and remove redundant text

* address review comments; minor tweaks

* WIP add Runtime calls to book

* Improve Runtime API docs

* expose thing we forgot to expose and doc link fixes
@tadeohepperle tadeohepperle marked this pull request as ready for review May 5, 2023 15:29
@tadeohepperle tadeohepperle requested review from a team as code owners May 5, 2023 15:29
@niklasad1
Copy link
Member

niklasad1 commented May 8, 2023

@tadeohepperle AFAIU the numbers you got on this branch was done with cargo nextest?

You didn't run the tests on this branch cargo test as well?

@tadeohepperle
Copy link
Contributor Author

@tadeohepperle AFAIU the numbers you got on this branch was done with cargo nextest?

You didn't run the tests on this branch cargo test as well?

No, for this branch it is cargo nextest for the normal (non wasm, non doc) tests. Cargo test is only used for the doc tests.

@niklasad1
Copy link
Member

niklasad1 commented May 9, 2023

The reason I ask is because it would be good to know where perf improvements comes from i.e, whether it's cargo nextest or your other changes.

Because cargo nextest has some restrictions and it has been slower when we tried in jsonrpsee IIRC.

@jsdw
Copy link
Collaborator

jsdw commented May 9, 2023

I'm generally inclined to approve this because:

  • Running fewer per-pallet UI tests is an easy win and locally that saved a bunch of time anyway.
  • Fewer CI steps (simpler is better!)
  • The "slowest" time taken seems to be reduced, and that's really the one we care about!

A couple of small outstanding questions to add to what Niklas said:

  1. I wonder how much the added metadata.scale files help; is there must difference either way? If not that different then I'd be inclined to just have the one file still to keep things simpler (I was hoping it'd be more dramatic :D)
  2. As Niklas says I wonder what would happen if no nextest. I'd geuss here that it does make a difference since the total time you have is pretty good, and the jobs are merged which allow it to do its thing better.

@tadeohepperle tadeohepperle force-pushed the tadeo-hepperle-speed-up-tests branch from 8d59910 to 82a5a21 Compare May 11, 2023 14:21
@@ -18,7 +18,7 @@
//! ```rust,no_run
//! use sp_keyring::AccountKeyring;
//!
//! #[subxt::subxt(runtime_metadata_path = "../artifacts/polkadot_metadata.scale")]
//! #[subxt::subxt(runtime_metadata_path = "../artifacts/polkadot_metadata_full.scale")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: could use the small metadata here?

@@ -14,7 +14,7 @@
//! Using this macro looks something like:
//!
//! ```rust,no_run
//! #[subxt::subxt(runtime_metadata_path = "../artifacts/polkadot_metadata.scale")]
//! #[subxt::subxt(runtime_metadata_path = "../artifacts/polkadot_metadata_full.scale")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: could use the tiny metadata here?

@@ -18,7 +18,7 @@
//! ```rust,no_run
//! use sp_keyring::AccountKeyring;
//!
//! #[subxt::subxt(runtime_metadata_path = "../artifacts/polkadot_metadata.scale")]
//! #[subxt::subxt(runtime_metadata_path = "../artifacts/polkadot_metadata_full.scale")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: could use the small metadata here?

@@ -24,7 +24,7 @@
//! ```rust,no_run
//! use sp_keyring::AccountKeyring;
//!
//! #[subxt::subxt(runtime_metadata_path = "../artifacts/polkadot_metadata.scale")]
//! #[subxt::subxt(runtime_metadata_path = "../artifacts/polkadot_metadata_full.scale")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: could use the tiny metadata here (I think)?

@@ -45,7 +45,7 @@
//! ```rust,no_run
//! use sp_keyring::AccountKeyring;
//!
//! #[subxt::subxt(runtime_metadata_path = "../artifacts/polkadot_metadata.scale")]
//! #[subxt::subxt(runtime_metadata_path = "../artifacts/polkadot_metadata_full.scale")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: could use the small metadata here?

Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

Looks great! I think we could use a small/tiny metadata in a load of places where full is used at the mo but we can tackle that in a follow up PR if you prefer (I commented on some but looks like many others could also be small or tiny)

@tadeohepperle
Copy link
Contributor Author

tadeohepperle commented May 11, 2023

@jsdw
I changed the metadata to the small version in a couple of places, when the pipeline is green again, we can merge it hopefully.

# to merge to master, too:
branches:
- master
push:
Copy link
Member

Choose a reason for hiding this comment

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

hrm, you replaced tabs with spaces?

it makes this PR it a little noisy, would be better to make that change in another PR if needed.

scripts/artifacts.sh Outdated Show resolved Hide resolved
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
#
# This script is to be run from the root of the repository: `scripts/artifacts.sh`
#
# It expects a local polkadot node to be running a JSON-RPC HTTP server at 127.0.0.1:9933
Copy link
Member

Choose a reason for hiding this comment

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

The server running on port 9933 is removed

Suggested change
# It expects a local polkadot node to be running a JSON-RPC HTTP server at 127.0.0.1:9933
# It expects a local polkadot node to be running a JSON-RPC server at 127.0.0.1:9944

@tadeohepperle tadeohepperle merged commit 6087dc0 into master May 12, 2023
@tadeohepperle tadeohepperle deleted the tadeo-hepperle-speed-up-tests branch May 12, 2023 09:27
@jsdw jsdw mentioned this pull request Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Speed up Tests
6 participants