Integration tests: fudge support for the generic environment #1588

lemunozm · 2023-10-10T15:30:30Z

Description

This PR adds support for fudge to be used as a generic environment over #1583

Moving from runtime environment to fudge environment

If you had a test as the following:

fn foo<T: Runtime>() {
    let env = RuntimeEnv::<T>::from_storage(...);
    //... unaltered rest of the test...
}

crate::test_for_runtimes!([development, altair, centrifuge], foo);

you can upgrade the test to use fudge as follows:

fn foo<T: Runtime + FudgeSupport>() {
    let env = FudgeEnv::<T>::from_storage(...);
    //... unaltered rest of the test...
}

crate::test_for_runtimes!([development, altair, centrifuge], foo);

The FudgeSupport trait allows all features from fudge, and FudgeEnv implements an environment that uses them under the hood.

How does it work?

A fudge struct (fudge::companion struct) for each runtime implements the runtime agnostic trait FudgeHandle that acts as a wrapper and unlocks all fudge features in a generic way. The FudgeEnv type implements the Env trait but makes use of the FudgeHandle that is only available from the tests if the FudgeSupport marker trait is added to the Runtime.

Progress

Basic structure
FudgeHandle
FudgeEnv implementation
Support for all runtimes
Consolidate submit() behavior.
- RuntimeEnv modifies the state after submit() and events can be read directly.
- FudgeEnv required one block to modify the state after submit() and be able to read events.
- Finally, chosen Option 3, having two different submit() methods
Make a loans test with this PR to polish the framework in the process

runtime/integration-tests/src/generic/environment.rs

lemunozm · 2023-10-13T07:19:03Z

I wanted to expand this point from the description a bit more:

Consolidate submit() behavior.
- RuntimeEnv modifies the state after submit(extrinsic) and events can be read directly.
- FudgeEnv required one block to modify the state after submit(extrinsic) and be able to read events.

Basically, we have different behaviors for the same Env trait API depending of the environment used. RuntimeEnv applies changes instantly, to the storage and FudgeEnv requires one block to make them effective. Options to solve this dichotomy:

Option 1: We transform RuntimeEnv to work as FudgeEnv. This solution is easy to achieve, but I think we lose the instant feedback from RuntimeEnv, which is a desired behavior and makes testing much easier for many test cases.
Option 2: We create a "temporal storage" in FudgeEnv when a submit(extrinsic) is done, emulating its application in the storage, to get instant feedback from the test (frederic suggestion). The question then is, if tests then would check against such "temporal storage", why use FudgeEnv instead of RuntimeEnv? How do we debug something that seems correct but fails later for the real fudge storage?
Option 3 (chosen): We live with this "dichotomy", some tests can be done easier with RuntimeEnv expecting instant feedback at reading events just after submitting extrinsic, and another test will be done by fudge where the state doesn't change until the chain evolves at least 1 block. The tester should choose the environment depending on their needs and know about this difference in behavior.
- If this difference makes sense, the Env trait can be changed to show explicitly this, so submit() can be divided into submit_now() submit_later(). RuntimeEnv can implement both, but FudgeEnv will implement only submit_later().

lemunozm · 2023-10-16T09:26:37Z

The PR with the proposed changes is done.

I'm adding loan's tests to polish some minor part, but the main structure can be reviewed or receive feedback

NunoAlexandre · 2023-10-16T09:27:46Z

The PR with the proposed changes is done.

I'm adding loan's tests to polish some minor part, but the main structure can be reviewed or receive feedback

I would love to review this later today 🤟

runtime/integration-tests/src/generic/envs/fudge_env.rs

runtime/integration-tests/src/generic/envs/fudge_env/handle.rs

cdamian · 2023-10-16T14:59:26Z

runtime/integration-tests/src/generic/envs/fudge_env/handle.rs

+	) -> RelaychainBuilder<Self::RelayConstructApi, Self::RelayRuntime> {
+		sp_tracing::enter_span!(sp_tracing::Level::INFO, "Relay - StartUp");
+
+		let code = Self::RELAY_CODE.unwrap();


I would prefer handling this and following unwraps or at least have some meaningful messages that can help our future selves in debugging this better ^^

I would say it's fine not to propagate the error here, because from a tester perspective, you always want a builder(), if not, you're doing something wrong in using this API.

I think unwrap/expect are fine to be used when you want to force a precondition of the usage of the API. In production, we should not use them because of the security implications of reaching a unwrap someday after some refactoring. However, having an unwrap near the originator of the error helps debugging and for testing purposes, I think improves the feedback cycle.

Yea, I agree. I think we should have this consistent throughout the calls that we're doing here and maybe just use expect with some meaningful messages?

Ok, I've restored the original expect() messages from util/env.rs file.

As a personal opinion, I usually try to reject expect() because most cases, we end up with duplicated obvious descriptions of why it could not fail, and the descriptions makes the code more overwhelming. i.e: list.find(...).expect("Cannot fail, it must exist"). Usually, Option does not need an expect description because the option meaning implies absence regarding the action you call, and Result does not need it because the panic message and error type explain the reason why it panicked in case of unexpected panic. For me, this is the case for 90% of unwrap/expect methods.

Of course, this is a personal point of view regarding this. Sure exist an opposite thought

runtime/integration-tests/src/generic/envs/fudge_env/handle.rs

…ge-generic-env

lemunozm · 2023-10-18T12:26:06Z

runtime/integration-tests/src/generic/utils/genesis.rs

+	pub trait CurrencyInfo {
+		const ID: CurrencyId;
+		const DECIMALS: u32;
+		const UNIT: Balance = 10u128.pow(Self::DECIMALS);
+		const SYMBOL: &'static str;
+		const NAME: &'static str = Self::SYMBOL;
+		const LOCATION: Option<xcm::VersionedMultiLocation> = None;
+		const CUSTOM: CustomMetadata;
+		const ED: Balance = 0;
+
+		fn metadata() -> AssetMetadata<Balance, CustomMetadata> {
+			AssetMetadata {
+				decimals: Self::DECIMALS,
+				name: Self::NAME.as_bytes().to_vec(),
+				symbol: Self::SYMBOL.as_bytes().to_vec(),
+				existential_deposit: Self::ED,
+				location: None,
+				additional: CustomMetadata {
+					pool_currency: true,
+					..Default::default()
+				},
+			}
+		}
+	}


This is experimental, to have good ergonomics later when testing something related to a currency, i.e (following comment):

lemunozm · 2023-10-18T12:26:26Z

runtime/integration-tests/src/generic/cases/loans.rs

+		Genesis::<T>::default()
+			.add(genesis::balances(T::ExistentialDeposit::get() + FOR_FEES))
+			.add(genesis::assets(vec![Usd6::ID]))
+			.add(genesis::tokens(vec![(Usd6::ID, Usd6::ED)]))
+			.storage(),


The above initializes a currency in orml_asset_registry called Usd6 (6 decimals) with a metadata whose ED correctly matches the orml_tokens. No more balance than ED is given to all users (ALICE, BOB...) for that currency.

lemunozm · 2023-10-20T15:50:20Z

runtime/integration-tests/src/generic/utils/mod.rs

+
+	// In order to later close the epoch fastly,
+	// we mofify here that requirement to significalty reduce the testing time.
+	// The only way to do it is breaking the integration tests rules mutating
+	// this state directly.
+	pallet_pool_system::Pool::<T>::mutate(pool_id, |pool| {
+		pool.as_mut().unwrap().parameters.min_epoch_time = POOL_MIN_EPOCH_TIME;
+	});


@mustermeiszer, my question about epoch time in slack came from this. If I call:

env.pass(Blocks::BySeconds(T::DefaultMinEpochTime::get()))

it takes around 10 min per test, even without fudge, only passing blocks to reach the point in time where I can actually closing the epoch.

If I modify the pool to set to the min bound, it takes around 60 seconds, but I think it's still not feasible if we expect to have a lot of loans tests (or any other thing that requires a funded pool).

The solution was to hardcode a really low value, but I don't like it because I should use something more high-level to deal with it. Doing this is really obscure.

lemunozm · 2023-10-20T15:52:35Z

This is ready for a final review & merge

mustermeiszer

Nit. Otherwise locks sick!

mustermeiszer · 2023-10-23T08:33:37Z

runtime/integration-tests/src/generic/utils/genesis.rs

+pub fn balances<T: Runtime>(balance: Balance) -> impl GenesisBuild<T> {
+	pallet_balances::GenesisConfig::<T> {
+		balances: default_accounts()
+			.into_iter()
+			.map(|keyring| (keyring.id(), balance))
+			.collect(),
+	}
+}
+
+pub fn tokens<T: Runtime>(values: Vec<(CurrencyId, Balance)>) -> impl GenesisBuild<T> {
+	orml_tokens::GenesisConfig::<T> {
+		balances: default_accounts()
+			.into_iter()
+			.map(|keyring| {
+				values
+					.clone()
+					.into_iter()
+					.map(|(curency_id, balance)| (keyring.id(), curency_id, balance))
+					.collect::<Vec<_>>()
+			})
+			.flatten()
+			.collect(),
+	}
+}
+
+pub fn assets<T: Runtime>(currency_ids: Vec<CurrencyId>) -> impl GenesisBuild<T> {
+	orml_asset_registry::GenesisConfig::<T> {
+		assets: currency_ids
+			.into_iter()
+			.map(|currency_id| (currency_id, currency::find_metadata(currency_id).encode()))
+			.collect(),
+		last_asset_id: Default::default(), // It seems deprecated
+	}


Why redo what was already in the non generic part?

If you mean, why not reuse the genesis functions from utils/genesis.rs is because:

I've tried, based on them, to make them more generic. The genesis methods from that module assume some currencies and initial values that maybe are not needed for some tests. These implementations are a bit more generic. Ideally, I would need to modify the others but doing it would make a lot of tests fail. Out of the scope of this PR, but we'll end up unifying them.

Nit. Returning impl GenesisBuild<T> improves the ergonomics with the Genesis<T> type (added in the previous PR). Passing a mutable ref to storage and later assimilating is a bit awkward, not sure why Substrate has chosen this.

i.e: with this:

Genesis::<T>::default() .add(genesis::balances(T::ExistentialDeposit::get() + FOR_FEES)) .add(genesis::assets(vec![Usd6::ID])) .add(genesis::tokens(vec![(Usd6::ID, Usd6::ED)])) .add(/* anything who implements GenesisBuild */) .storage()

You know exactly how your initial state is for your current test without looking into the genesis functions. No more balances that can you see, nor more assets or tokens

I am against this. For integration tests I would rally like to see not setting everything up everywhere.

But let's see where we drift of from here

You can always wrap a common initial state under some method and then call this method for a set of tests that requires that specific state. In the loan example, the common state is a function intended to be called from different loan tests that need the same pool state.

These genesis methods are a very low-level foundation you can compose to create your domain related genesis

Actually, each test domain could implement a GenesisBuild where its initial state is built using the previous pieces, and later:

Genesis::<T>::default() .add(genesis::loans_initial_state()) .storage()

or

Genesis::<T>::default() .add(genesis::empty_pool()) .storage()

Things like those. Just drafting an idea of how it can evolve

My point in favor of this is that I feel quite frustrated when a test I didn't write fails, and I need to dig into the first calls to know precisely what was the initial state it was built. And it was because of some global thing I didn't consider that was related to the failure.

But let's see where we drift of from here

Thanks anyway for your "let's see" vote!

lemunozm added I4-tests Test needs fixing or improving. I8-enhancement An additional feature. crcl-protocol Circle protocol related. labels Oct 10, 2023

lemunozm self-assigned this Oct 10, 2023

lemunozm requested review from mustermeiszer, NunoAlexandre, wischli and cdamian as code owners October 10, 2023 15:30

lemunozm changed the base branch from main to integration-test/generic-env-and-runtime October 10, 2023 15:31

This was referenced Oct 11, 2023

PoC: Integration tests with generic fudge environment #1578

Closed

Common trait for companion generated structs centrifuge/fudge#21

Open

lemunozm commented Oct 11, 2023

View reviewed changes

runtime/integration-tests/src/generic/environment.rs Outdated Show resolved Hide resolved

lemunozm mentioned this pull request Oct 11, 2023

Integration test: Generic runtime and environment #1583

Merged

2 tasks

lemunozm force-pushed the integration-tests/fudge-generic-env branch from fa5d308 to f65d330 Compare October 11, 2023 17:55

lemunozm added 11 commits October 12, 2023 21:47

Add handle and base env structure

b6eb261

evolving and state support for fudge env

ad491e2

Support Client APIs

6c5f848

simplify test cases

fa48bea

Add spetial fudge methos

5f17c11

Add T to FudgeHandle

c45a04f

sumbmit extrinsic works

12beb07

use correct nonce

170b4a2

minor Env trait simplification

b6c8075

minor example reorder

3032d09

fudge support for all runtimes

084d148

lemunozm force-pushed the integration-tests/fudge-generic-env branch from f65d330 to 084d148 Compare October 12, 2023 19:50

lemunozm changed the base branch from integration-test/generic-env-and-runtime to main October 12, 2023 20:40

fix nonce and divide api between submit now and later

c94c1ad

lemunozm force-pushed the integration-tests/fudge-generic-env branch from 380b817 to c94c1ad Compare October 13, 2023 09:58

lemunozm added 3 commits October 13, 2023 13:20

minor organization

2fb97d5

init tests for loans

bf6269a

fix ongoing loans test

21577d0

fix compilation

f998636

cdamian reviewed Oct 16, 2023

View reviewed changes

runtime/integration-tests/src/generic/envs/fudge_env.rs Show resolved Hide resolved

cdamian reviewed Oct 16, 2023

View reviewed changes

runtime/integration-tests/src/generic/envs/fudge_env/handle.rs Outdated Show resolved Hide resolved

cdamian reviewed Oct 16, 2023

View reviewed changes

runtime/integration-tests/src/generic/envs/fudge_env/handle.rs Outdated Show resolved Hide resolved

lemunozm added 3 commits October 16, 2023 18:27

Merge remote-tracking branch 'origin/main' into integration-tests/fud…

848339d

…ge-generic-env

minor renames

4b159c5

restore expect

5d056ab

cdamian previously approved these changes Oct 16, 2023

View reviewed changes

loan testing in progress

ef4d158

lemunozm dismissed cdamian’s stale review via ef4d158 October 17, 2023 05:42

lemunozm added 3 commits October 18, 2023 10:21

Merge remote-tracking branch 'origin/main' into integration-tests/fud…

774c241

…ge-generic-env

fixed issue with asset_registry

ab7138b

add CurrencyInfo genesis utility

5e79525

lemunozm commented Oct 18, 2023

View reviewed changes

lemunozm added 5 commits October 20, 2023 09:53

extend loan tests, fix block by seconds

93a0dda

Api from Runtime into an associated type for expliciteness

6b29b2b

minor changes

83bd44c

fix and extend loan testing

ed92aab

fix and extend loan testing

a496e35

lemunozm commented Oct 20, 2023

View reviewed changes

remove unused pallet sudo addition

744f59f

mustermeiszer approved these changes Oct 23, 2023

View reviewed changes

lemunozm merged commit 8e32e01 into main Oct 23, 2023
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integration tests: fudge support for the generic environment #1588

Integration tests: fudge support for the generic environment #1588

lemunozm commented Oct 10, 2023 •

edited

Loading

lemunozm commented Oct 13, 2023 •

edited

Loading

lemunozm commented Oct 16, 2023

NunoAlexandre commented Oct 16, 2023

cdamian Oct 16, 2023

lemunozm Oct 16, 2023

cdamian Oct 16, 2023

lemunozm Oct 16, 2023 •

edited

Loading

lemunozm Oct 18, 2023

lemunozm Oct 18, 2023

lemunozm Oct 19, 2023

lemunozm Oct 20, 2023

lemunozm commented Oct 20, 2023

mustermeiszer left a comment

mustermeiszer Oct 23, 2023

lemunozm Oct 23, 2023

lemunozm Oct 23, 2023 •

edited

Loading

mustermeiszer Oct 23, 2023

lemunozm Oct 23, 2023 •

edited

Loading

lemunozm Oct 23, 2023

lemunozm Oct 23, 2023

lemunozm Oct 23, 2023

Integration tests: fudge support for the generic environment #1588

Integration tests: fudge support for the generic environment #1588

Conversation

lemunozm commented Oct 10, 2023 • edited Loading

Description

Moving from runtime environment to fudge environment

How does it work?

Progress

lemunozm commented Oct 13, 2023 • edited Loading

lemunozm commented Oct 16, 2023

NunoAlexandre commented Oct 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm commented Oct 20, 2023

mustermeiszer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm commented Oct 10, 2023 •

edited

Loading

lemunozm commented Oct 13, 2023 •

edited

Loading

lemunozm Oct 16, 2023 •

edited

Loading

lemunozm Oct 23, 2023 •

edited

Loading

lemunozm Oct 23, 2023 •

edited

Loading