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

Put integration tests behind feature flag #515

Merged
merged 27 commits into from Apr 30, 2022
Merged

Put integration tests behind feature flag #515

merged 27 commits into from Apr 30, 2022

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Apr 22, 2022

The substrate binary is used internally for integration testing purposes.

This PR ensures that building/running tests do not rely on the existence of the
SUBSTRATE_NODE_PATH in the environment, which is guarded under the
integration-tests crate.

The subxt tests that rely on the substrate binary are moved to a dedicated crate.
At the same time, any dependency on the test-runtime is removed to avoid any
cyclic dependencies. The only crate that requires the substrate binary to exist in the
environment is the integration-tests crate.

Testing Done

cd subxt
SUBSTRATE_NODE_PATH="" cargo build --release
SUBSTRATE_NODE_PATH="" cargo test

Closes #351.

Next Steps

  • Move integration tests to their own crate to avoid substrate binary dependency on other crates
  • Construct manual metadata for subxt caching tests
  • Add metadata.scale blob for examples and metadata crate testing
  • Ensure #[cfg] is valid with feature flag

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

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

An alternative to the test-runtime feature-flag approach would be having the
dependency declared as optional = true and included for the feature flag only.
Unfortunately, this option does not work for dev-dependencies. Having the
test-runtime as simply dependency would cause cyclic reference.

Since tests/integration is really it's own crate, this could be moved to the top level and made into an explicit crate which would allow using a normal dependency and make it optional?

@lexnv lexnv mentioned this pull request Apr 28, 2022
12 tasks
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
lexnv added 15 commits April 28, 2022 16:28
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
integration-tests/src/lib.rs Outdated Show resolved Hide resolved
subxt/Cargo.toml Outdated Show resolved Hide resolved

Metadata::try_from(meta)
Metadata::try_from(prefixed)
.expect("Cannot translate runtime metadata to internal Metadata")
}

Copy link
Collaborator

@jsdw jsdw Apr 29, 2022

Choose a reason for hiding this comment

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

// Currently the caching does not take into account different pallets
// as the intended behavior is to use this method only once.
// Enforce this behavior into testing.
let hash_old = metadata.metadata_hash(&["Balances"]);
assert_eq!(hash_old, hash);

I don't really understand what this line does? (also doesn't look like there is a balances pallet in the fake metadata?)

(alas I couldn't comment on the line itself; sorry!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have updated the comment. If we call metadata_hash with an argument of pallets: ["System"] it would produce a hash that is cached internally. Therefore, if we call the same method again, but with different arguments (ie pallets: ["Balances"]), it would fetch the previously cached value 😄

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, that is clearer! :)

@jsdw
Copy link
Collaborator

jsdw commented Apr 29, 2022

Looks good to me offhand, and I think the separate crate is a nice organisational improvement, and gives people the opportunity to avoid the integration tests if they don't have a suitable substrate binary to hand while still making it the default, which I think is right.

Just a couple of small comments and it's all good from me :)

@jsdw jsdw requested a review from ascjones April 29, 2022 11:24
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Copy link
Member

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

How about moving the contents of integration-tests/tests/ (which at the moment is an implicit sub-crate) into integration-tests/src (renaming main.rs to lib.rs?

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
Copy link
Contributor Author

lexnv commented Apr 29, 2022

@ascjones Moving the/tests directory directly under /src makes the crate more ergonomic for me. Thanks for the feedback!

Copy link
Member

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Looking good to me, nice one!

@lexnv lexnv merged commit 5c17afb into master Apr 30, 2022
@lexnv lexnv deleted the 351_test_feature branch April 30, 2022 09:26
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.

Put integration tests behind feature flag to avoid need for substrate on PATH
3 participants