paritytech / substrate

Substrate: The platform for blockchain innovators
Apache License 2.0
8.39k stars 2.65k forks source link

Refactor `substrate-test-runtime` #9782

Open bkchr opened 2 years ago

bkchr commented 2 years ago

The substrate-test-runtime is some special kind of runtime that incorporates some FRAME stuff and some custom logic. As the name suggests, it is used for testing. The idea here being to rewrite the test runtime into some "pure" FRAME runtime and to remove other cruft. The runtime contains a lot of stuff for testing sp-api behavior in wasm vs native. Most of this stuff can probably be removed as we will gonna deprecate the native runtime in the future. This means that we can remove this huge cfg-if block. Then the runtime contains the custom extrinsic. These are all the calls this test runtime supports. They are being used all over the code base to test different things. These calls need to rewritten as some FRAME pallet. This means we there should be written one pallet as part of this runtime that exposes the same kind of calls, but through the "FRAME abstraction". All the code that is calling these calls need to be changed to use the FRAME Call enum to achieve the same outcome.

The runtime currently also only supports BABE. There should be two features with-babe and with-aura. Depending on which feature is activated, the respective pallets are activated. As we already enable the disable-logging feature while building the wasm binary, it should be changed in the same way to build with once with BABE and once with AURA.

The general idea of this whole issue is that we can continue using the runtime for our unit tests, but we also can write "proper" unit tests for AURA. In a later future we would also like to have zombienet tests based with AURA so that we can test things like warp syncing etc. Currently we use the kitchensink-runtime for the zombienet tests. Maybe it would be a good idea to move the "special testing" pallet just to the kithchensink-runtime and to make this runtime build able with AURA/BABE. Then remove the testing runtime and only use the kitchenshink-runtime for all tests. However, this may adds more complexity and things that can fail in the unit tests. A simple runtime with just the consensus pallets, the testing pallets and some minimal set of required pallets is probably easier to maintain and to use.

gilescope commented 2 years ago

Sorry I should ask questions sooner rather than waste time trying to figure things out by myself. The substrate-test-runtime is used by the cumulus test environment so I am assuming that we are looking to make the substrate-test-runtime more general and split it into a babe and a aura (and maybe a manual seal one?).

I have been copying across the cumulus as an example, stripping out the relay chain and seeing if I can make that the basis to start from and port the tests to use that rather than using the substrate-test-runtime directly. (and then subsume the bits of substrate-test-runtime that are still needed).

So there will be a substrate/tests dir like cumulus and polkadot and the substrate/test-utils dir will hopefully disappear.

I am not sure if there was a question in there - more a statement of what I am trying to do...

bkchr commented 2 years ago

The substrate-test-runtime is used by the cumulus test environment

No it is not used in Cumulus.

the substrate-test-runtime more general and split it into a babe and a aura (and maybe a manual seal one?)

Yes that should be the way forward. A manual seal one should probably used in most of the tests that just want to interact with "a runtime" to test stuff. And a Babe + Aura version should be used by the respective crates to test their code, these runtimes could be stripped down to the minimum.

So there will be a substrate/tests dir like cumulus and polkadot and the substrate/test-utils dir will hopefully disappear.

This is only about Substrate, nothing that touches Polkadot. test-utils also contains the test-client which would stay, but could also be moved tests.

gilescope commented 2 years ago

Ah yes I was thinking of the references to sustrate-test-client - there are no direct references to the substrate-test-runtime.

bkchr commented 2 years ago

As I just the days thought about all this code here: https://github.com/paritytech/substrate/blob/a76ed8c6d4c0655f208ff51684efbeb64d32dea7/test-utils/runtime/src/lib.rs#L696-L1183

In this new "model" we should put this code in its own crate that is compiled to WASM etc and is only used by sp-api-test.

gilescope commented 2 years ago

Agreed. impl_runtime_apis! feels like a hack to do hand rolled stuff.

bkchr commented 2 years ago

Agreed. impl_runtime_apis! feels like a hack to do hand rolled stuff.

Not sure what you mean by hand rolled stuff here. You can just use the macro, also for sp-api-test