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

[ink_e2e] spawn a separate contracts node instance per test #1642

Merged
merged 15 commits into from
Feb 7, 2023

Conversation

ascjones
Copy link
Collaborator

@ascjones ascjones commented Feb 3, 2023

Currently the E2E tests depend on a shared instance of substrate-contracts-node running in the background. This can cause issues where tests are not independent, and can be non deterministic when different tests share account ids. See #1615.

This PR spawns a unique instance of substrate-contracts-node per test, so the tests are completely isolated. I have copied the TestNodeProcess from subxt in order to manage the spawning of the node process and retrieving the unique ports which allow them to run in parallel. This also ensures that the processes are killed on drop.

The default is assuming substrate-contracts-node is installed on the PATH. The alternative is to specify the CONTRACTS_NODE env variable.

This is also a quality of life improvement, because it does not depend on the user or the CI environment starting up a contracts node in the background.

@@ -1,11 +0,0 @@
#[tokio::test]
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe worth replacing this with a test on launching the test node

@SkymanOne SkymanOne self-requested a review February 4, 2023 13:21
Copy link
Contributor

@SkymanOne SkymanOne left a comment

Choose a reason for hiding this comment

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

Can you please add CHANGELOG entry?

@codecov-commenter
Copy link

codecov-commenter commented Feb 6, 2023

Codecov Report

Merging #1642 (ba9c03b) into master (9b6f372) will decrease coverage by 24.62%.
The diff coverage is 68.18%.

@@             Coverage Diff             @@
##           master    #1642       +/-   ##
===========================================
- Coverage   70.49%   45.87%   -24.62%     
===========================================
  Files         207      206        -1     
  Lines        6388     6393        +5     
===========================================
- Hits         4503     2933     -1570     
- Misses       1885     3460     +1575     
Impacted Files Coverage Δ
crates/e2e/macro/src/codegen.rs 0.00% <0.00%> (ø)
crates/e2e/macro/src/lib.rs 0.00% <ø> (ø)
crates/e2e/src/client.rs 0.00% <ø> (-36.37%) ⬇️
crates/e2e/src/lib.rs 0.00% <ø> (-25.00%) ⬇️
crates/e2e/src/node_proc.rs 73.33% <73.33%> (ø)
crates/e2e/macro/src/config.rs 72.00% <100.00%> (+15.24%) ⬆️
crates/ink/codegen/src/traits.rs 0.00% <0.00%> (-100.00%) ⬇️
crates/ink/codegen/src/generator/env.rs 0.00% <0.00%> (-100.00%) ⬇️
crates/ink/codegen/src/generator/mod.rs 0.00% <0.00%> (-100.00%) ⬇️
crates/ink/codegen/src/enforced_error.rs 0.00% <0.00%> (-100.00%) ⬇️
... and 60 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ascjones ascjones mentioned this pull request Feb 6, 2023
Copy link
Contributor

@SkymanOne SkymanOne 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

@cmichi cmichi left a comment

Choose a reason for hiding this comment

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

Thanks, it's good!

@ascjones ascjones merged commit 8bc8593 into master Feb 7, 2023
@ascjones ascjones deleted the aj/auto-spawn-node branch February 7, 2023 11:43
@ascjones ascjones mentioned this pull request Feb 7, 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.

4 participants