Create binary for cumulus testing node #1128 #1158

skunert · 2022-04-08T14:37:21Z

In this PR I create a simple binary for a cumulus testing node. This is a draft until we have checked that this is a good fit for testing with zombienet.

Supported cli args:

--parachain-id
--relay-chain-bootnodes
--alice, bob, ...
--reserved-nodes
--reserved-only
--node-key
--bootnodes
--relay-chain-bootnodes
--collator
--relay-chain-spec
--port

export-genesis-wasm
export-genesis-state

// following arguments are used to build specific scenarios for the tests
--use-null-consensus
--disabled-block-announcements

skunert · 2022-04-20T14:53:54Z

I changed the PR to not use the builder that is mentioned in the issue.
Reasoning:

We now have the same CLI as the collator
Configuration is handled in a way that is close to production

The CLI is copy pasted from the original collator in polkadot-parachains.

test/service/src/lib.rs

* add build-spec subcommand * reorder args

koute · 2022-05-10T10:22:45Z

The cargo fmt job is failing. (:

So I have a few questions:

Do we already have any tests (perhaps on a separate branch somewhere) which will use this new binary?
Are you looking to merge this in before the test are written, or after?
Since the CLI closely matches the production binary anyway can you explain why this new binary is necessary? Can't we run the zombienet tests with normal cumulus binary? I see that you've added e.g. --use-null-consensus so I suppose those tests that require that functionality can't be run with the normal binary, but what about the rest? Is the intent to use this binary for all of zombienet tests, or for only a few of them?

skunert · 2022-05-10T12:07:02Z

I should have probably given a bit more context.

We are planning to migrate the cumulus integration tests to zombienet. Zombienet requires the binary. @pepoviola is working on zombienet and agreed to help us migrate the integration tests once I implemented the binary for him. My original plan was to merge this and have a separate PR for the tests themselves. For a short period, the binary would be unused. We could also leave this PR open and have the test migration here, but this comes at the cost of maintaining this PR.

The extra binary is to have a clear separation between the testing infrastructure and the prod binary. They are similar, but the testing binary offers access to the test runtime and the extra arguments you mentioned (we are free to add more specific scenarios easily to the test-collator). We intend to use this test binary for all zombienet tests in cumulus. At some point, we could probably even remove the builder pattern for the old integration tests and have this test-collator only.

pepoviola · 2022-05-10T17:08:54Z

Hi, I already have a couple of integration test ready to add in this pr if you want. We will need also to build an image with the test-collator to use in gitlab ci. I can also add that step to the current setup.

Thanks!

* add migrated tests * rename test files * uncomment args and remove extra collator node

koute · 2022-05-12T13:06:26Z

I should have probably given a bit more context.

We are planning to migrate the cumulus integration tests to zombienet. Zombienet requires the binary. @pepoviola is working on zombienet and agreed to help us migrate the integration tests once I implemented the binary for him. My original plan was to merge this and have a separate PR for the tests themselves. For a short period, the binary would be unused. We could also leave this PR open and have the test migration here, but this comes at the cost of maintaining this PR.

The extra binary is to have a clear separation between the testing infrastructure and the prod binary. They are similar, but the testing binary offers access to the test runtime and the extra arguments you mentioned (we are free to add more specific scenarios easily to the test-collator). We intend to use this test binary for all zombienet tests in cumulus. At some point, we could probably even remove the builder pattern for the old integration tests and have this test-collator only.

I see!

I'd personally would probably prefer if we'd use the production binary for those tests that actually can just use the production binary since, you know, over time the behavior between the production binary and the test binary might go out of sync and then we might not be testing what is actually relevant. We could also consider (assuming the changes are unintrusive enough, and I don't know if they'd be) adding an extra cargo opt-in feature for those special test-only args that the tests need and just use the production binary for all of them? But I know practical logistics of the whole thing might make this infeasible and/or a bad idea (and I'm not the one implementing this) so I'll leave this decision to you. (:

koute

It mostly looks fine to me. Obviously it's still incomplete, but since this is purely for tests I think it's fine to just merge it and iterate on it instead of just waiting until everything's ready and do one huge PR.

test/service/src/cli.rs

zombienet_tests/0002-pov_recovery.toml

test/service/src/main.rs

* add migrated tests * rename test files * uncomment args and remove extra collator node * fix identation

davxy · 2022-05-13T15:38:39Z

test/service/src/cli.rs

+
+#[derive(Debug, Parser)]
+#[clap(
+	propagate_version = true,


Suggested change

propagate_version = true,

version,

propagate_version = true,

Without this the application doesn't have the version option, thus can't use the propagate_version.
Indeed clap panics if the binary is built without --release flag

Good catch, I think this was also missing for our other binaries, added it there too

From what I experienced, other binaries (e.g. polkadot-collator) doesn't have the same problem.

I think this is because they are using a custom build script to manually define a custom version string augmented with the commit hash:

cumulus/polkadot-parachains/build.rs

Line 20 in a5bfd11

generate_cargo_keys();

And

https://github.com/paritytech/substrate/blob/d45e3837bc89338c6eae1ff5dc250d01e1cae0e6/utils/build-script-utils/src/version.rs#L21

Ah I see, was not aware of that. I left it only for the test binary, should be fine now

davxy · 2022-05-13T15:52:11Z

I haven't got enough experience with zombienet testing to give a strong opinion WRT the final application of this PR... but once the missing "version" thing is fixed, from a formal pow it LGTM

bkchr · 2022-05-16T20:54:54Z

I'm fine to merge this first and then have the test enabled later

test/service/src/main.rs

Sebastian Kunert added 2 commits April 8, 2022 16:26

Create binary for cumulus testing node

84b2895

Fix call to wrong method

2bd3d06

skunert mentioned this pull request Apr 8, 2022

Provide binary for test service #1128

Closed

skunert added 13 commits April 11, 2022 14:49

Fix iterator bug

14b8148

Add real networking to polkadot node

bfa9f8f

Merge branch 'master' into skunert-cumulus-testing-node

2eff8bc

Allow using of file chain specs

14ebbc1

Implement export of wasm blob and genesis state

c5c64ae

Add parachain id to test service export

fba51d3

Remove debug logs

c5cbf76

Fix main.rs

319e96a

Do not use cli from polkadot-collator

ec851db

Add tracing dependency

014f021

Improve cli structure

4c009f1

Use struct instead of enum for export commands

1ee5e93

Add missing license header

b89d636

skunert added B0-silent Changes should not be mentioned in any release notes C1-low 📌 labels Apr 20, 2022

Fix benchmark

b6e1d27

skunert marked this pull request as ready for review April 21, 2022 07:14

skunert requested a review from a team April 21, 2022 07:15

wigy-opensource-developer reviewed Apr 21, 2022

View reviewed changes

test/service/src/lib.rs Show resolved Hide resolved

add build-spec subcommand (#19)

7262bb2

* add build-spec subcommand * reorder args

skunert requested a review from a team May 9, 2022 10:53

skunert added 2 commits May 10, 2022 13:38

Fix formatting

0190cd9

Merge branch 'master' into skunert-cumulus-testing-node

f09c063

Add zombienet test (#26)

686a444

* add migrated tests * rename test files * uncomment args and remove extra collator node

koute approved these changes May 12, 2022

View reviewed changes

test/service/src/cli.rs Outdated Show resolved Hide resolved

zombienet_tests/0002-pov_recovery.toml Outdated Show resolved Hide resolved

test/service/src/main.rs Outdated Show resolved Hide resolved

Fix indentation (#27)

65a402d

* add migrated tests * rename test files * uncomment args and remove extra collator node * fix identation

davxy reviewed May 13, 2022

View reviewed changes

bkchr reviewed May 16, 2022

View reviewed changes

test/service/src/main.rs Outdated Show resolved Hide resolved

test/service/src/main.rs Outdated Show resolved Hide resolved

test/service/src/main.rs Outdated Show resolved Hide resolved

skunert added 2 commits May 18, 2022 10:52

Review comments

a8a851a

Remove unwanted version changes

fa7219c

davxy approved these changes May 18, 2022

View reviewed changes

skunert merged commit b1a5d70 into paritytech:master May 18, 2022

skunert deleted the skunert-cumulus-testing-node branch May 18, 2022 14:41

shawntabrizi mentioned this pull request May 19, 2022

Fix Cumulus Compile (into_account_truncating) #1277

Merged

skunert mentioned this pull request Nov 2, 2022

Member request polkadot-fellows/seeding#55

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create binary for cumulus testing node #1128 #1158

Create binary for cumulus testing node #1128 #1158

skunert commented Apr 8, 2022 •

edited

Loading

skunert commented Apr 20, 2022

koute commented May 10, 2022

skunert commented May 10, 2022

pepoviola commented May 10, 2022

koute commented May 12, 2022

koute left a comment •

edited

Loading

davxy May 13, 2022 •

edited

Loading

skunert May 18, 2022

davxy May 18, 2022 •

edited

Loading

skunert May 18, 2022

davxy commented May 13, 2022 •

edited

Loading

bkchr commented May 16, 2022

Create binary for cumulus testing node #1128 #1158

Create binary for cumulus testing node #1128 #1158

Conversation

skunert commented Apr 8, 2022 • edited Loading

skunert commented Apr 20, 2022

koute commented May 10, 2022

skunert commented May 10, 2022

pepoviola commented May 10, 2022

koute commented May 12, 2022

koute left a comment • edited Loading

Choose a reason for hiding this comment

davxy May 13, 2022 • edited Loading

Choose a reason for hiding this comment

skunert May 18, 2022

Choose a reason for hiding this comment

davxy May 18, 2022 • edited Loading

Choose a reason for hiding this comment

skunert May 18, 2022

Choose a reason for hiding this comment

davxy commented May 13, 2022 • edited Loading

bkchr commented May 16, 2022

skunert commented Apr 8, 2022 •

edited

Loading

koute left a comment •

edited

Loading

davxy May 13, 2022 •

edited

Loading

davxy May 18, 2022 •

edited

Loading

davxy commented May 13, 2022 •

edited

Loading