use-ink
Fix E2E workflow for use with long-running contracts node
In https://github.com/paritytech/ink/pull/1642, the E2E testing framework will now automatically spawn a contracts node instance per test. The primary motivation is to make the tests independent from one another, making the CI test runs more robust.
However, for developer workflows this assumes that there is a locally accessible pre-built binary. Previously it was possible to
- Run a
substrate-contracts-nodeon a remote machine and run the tests against that - Easily run a node in the background from source via
cargo run, and then run the E2E tests against that.
We should make these workflows possible again, which both assume a long running contracts node rather than the auto spawn per test.
One way to achieve this would be via an environment variable e.g. E2E_BACKGROUND_CONTRACTS_NODE_URL or something shorter and equally descriptive. Setting this would disable the default functionality of spawning a test per node, and connect with a client to the assumed-top-be-running node at the supplied URL. e.g.
E2E_BACKGROUND_CONTRACTS_NODE_URL=ws://localhost:9944 cargo test --features e2e-tests
Admittedly that is a bit verbose, so here are some ideas off the top of my head:
- Short term: user can set that env variable in their shell.
- Allow setting this in the
Cargo.tomlof the target contract? Downside is this could be committed by mistake to source control in shared repos. - Bring back
cargo contract test(which I just removed), and then we can provide CLI options for this e.g.cargo contract test --node-url ws://localhost:9944
The reason to originally switch to one node process per test was to work around a failure in the CI, which we couldn't reproduce locally, correct?
I think it would be better to have one process in the background be the default behavior of our E2E tests. It could also be spawned automatically, like we do now, but only once. This will make things go faster and also help with the performance of https://github.com/paritytech/ink/pull/1648.
Can we also investigate why the CI was hanging on a single running instance? AFAIK it works just fine locally
The reason to originally switch to one node process per test was to work around a failure in the CI, which we couldn't reproduce locally, correct?
This failure was another manifestation of the fundamental issue of the individual tests running in parallel against the same node. In this configuration it is impossible to prevent race conditions between the tests. Previously it was required that each test in a suite was using a different account for interacting with contracts, otherwise the tests could interfere with one another.
This was a problem when requiring more than 9 tests (the number of known prefunded accounts), which was the motivation behind https://github.com/paritytech/ink/pull/1615. Even that required some hacky retry code to ensure success.
Can we also investigate why the CI was hanging on a single running instance?
My hunch (not proven) is that the retry spamming from all tests running in parallel was the cause of the hanging of the single instance, since it was always the test fixture using the create_and_fund_account. Imagine 9 tests spinning up at once and attempting to send balance_transfer tests to the same node repeatedly for the same account.
Note that since we merged #1642 we have not had any failures of the example-test job (AFAIK) that have required restarting the job to get it to run again, or investigation into why the tests are flaky, Less :t-rex: time is a win IMO.
I think it would be better to have one process in the background be the default behavior of our E2E tests. It could also be spawned automatically, like we do now, but only once.
I disagree. I think it is more important to have individual tests be deterministic and reliable so the default should be instance per test.
This will make things go faster
In fact the CI job gitlab-examples-tests can just a fast (or faster) now e.g.
- before: 16 minutes 17 seconds. https://gitlab.parity.io/parity/mirrors/ink/-/jobs/2347234
- after: 14 minutes 8 seconds. https://gitlab.parity.io/parity/mirrors/ink/-/jobs/2364791
and also help with the performance of https://github.com/paritytech/ink/pull/1648.
You could still run once instance per quickcheck fixture, it would just be maintained across iterations of the same test, and you'd get some guarantee of determinism assuming the quickcheck iterations are run serially.
So by default an individual substrate-contracts-node is spun up per test.
I like the idea of providing an environment variable like E2E_BACKGROUND_CONTRACTS_NODE_URL=ws://localhost:9944.
But instead of that environment variable referring to a just a single node I think it should be a comma separated list of one or more local and remote nodes like:
E2E_BACKGROUND_CONTRACTS_NODE_URLS=ws://localhost:9944,ws://localhost:9945,ws://localhost:9946
If a user wanted to test cross-chain functionality across multiple nodes in a single test or a group of tests then they could refer to that env variable and parse each endpoint address to spin up more than one of those node endpoints in a singleton setup block like this before any of those tests run.
Then run a script like https://github.com/ltfschoen/InkTemplate/blob/main/docker/reset.sh to kill each of them after the group of tests have finished, so they'd have a fresh chain db in subsequent tests that restart them but would have to mock the expected chain state that was destroyed.
This was a problem when requiring more than 9 tests (the number of known prefunded accounts)
Is there any reason why we can't update Substrate so the user can specify a vanity account address pattern that generates a custom amount of prefunded accounts (that the user specifies) that are each endowed with a custom amount (also specified by the user) via the CLI when they spin up a Substrate-based node, so that we can overcome this limitation of only having 9 prefunded accounts limitation (i.e. currently only Alice, Bob, Charlie, Dave, Eve, Ferdie, .... )? Maybe that's a feature I could add?
