Integration test: Generic runtime and environment #1583

lemunozm · 2023-10-06T09:53:37Z

Description

This PR adds a testing framework with the following features:

Tests are now parameterized by T: Runtime where T contains all expected types from a runtime as Config, Block, Events, Calls, Apis... (check env.rs file).
Tests are generic over an environment. The Env API abstracts between:
- fudge (pending work for a future PR)
- a more simple/low-level environment close to the runtime itself
No more macros in the test cases (easier to read/debug)

The framework and tests cases are under a generic folder. Check the example to get an introduction of this.

Pending extra changes for this PR:

Fix the fees calculation method.
Macro to run tests parameterized with the runtimes to test.

lemunozm

Pending things:

lemunozm · 2023-10-06T09:55:36Z

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

+			match dispatch_info.pays_fee {
+				Pays::Yes => WeightToFee::weight_to_fee(&dispatch_info.weight),
+				Pays::No => 0,
+			}


This is not exactly the fees the last executed extrinsic has consumed. Not sure why or what I'm missing.

I would have expected your solution to be correct. Have you compared the current result against the fee returned by transaction_payment::Event::<T>:TransactionFeePaid { actual_fee, tip } which should be the event before the last one?

They're different, not sure why, but the way of calculating them comes from different paths. The difference is really subtle anyway:

From TransactionFeePaid:

2004154679487179

From ExtrinsicSuccess:

2003074679487179

Still investigating, thanks for addressing me to the TransactionFeePaid event!

Not sure why the difference. But as far as I am concerned, using the value from TransactionFeePaid works so... I'll give this as solved.

Maybe TransactionFeePaid includes another signature check which is not included in ExtrinsicSuccess. Just a guess, haven't looked into the code further.

Maybe there's some extra fees for dispatching the transaction? That happens with xcm-related transactions, maybe there's a similar mechanism here involved?

Yeah, there must be some extra fees that do not depend on the weight of the call, so when I do WeightToFee::weight_to_fee(&dispatch_info.weight) I'm leaving some fees behind.

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

runtime/development/src/lib.rs

wischli

Some comments. Really looking forward to getting hands on with this new approach! Thanks for your great work, as always. Super clean and a pleasure to read.

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

wischli · 2023-10-06T13:15:57Z

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

+			match dispatch_info.pays_fee {
+				Pays::Yes => WeightToFee::weight_to_fee(&dispatch_info.weight),
+				Pays::No => 0,
+			}


I would have expected your solution to be correct. Have you compared the current result against the fee returned by transaction_payment::Event::<T>:TransactionFeePaid { actual_fee, tip } which should be the event before the last one?

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

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

NunoAlexandre

Overall this looks fantastic 👏 awesome work, Luis.

I am wondering how we plan on handling certain behaviour deviations between runtimes; for example, DOT and KSM have different decimals so when testing XCM transfers involving such a case, we need to take those differences in consideration. Or we might have certain restrictions on centrifuge that we don't have on development which would make it hard to build generic tests over the two.

Either way, this is a beautiful step further ❤️

NunoAlexandre · 2023-10-09T07:08:55Z

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

@@ -0,0 +1 @@
+// TODO: implement generic::env::Env for an environment using fudge


Do we still want to do this in this PR?

No, this will be a future PR not to block this. That file is just a placeholder.

I should create an issue for this

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

…ric-env-and-runtime

lemunozm · 2023-10-09T07:37:02Z

I am wondering how we plan on handling certain behaviour deviations between runtimes; for example, DOT and KSM have different decimals so when testing XCM transfers involving such a case, we need to take those differences in consideration. Or we might have certain restrictions on centrifuge that we don't have on development which would make it hard to build generic tests over the two.

For such things, the Runtime trait has a T::KIND where you can match and distinguish your behavior. i.e:

match T::KIND {
   KindRuntime::Development => //do X,
   KindRuntime::Altair => //do Y,
   KindRuntime::Centrifuge => //do Z
}

But I think most of the time, differentiations as decimals or other properties can be used under some T::property::get() without looking into what it really is.

lemunozm · 2023-10-09T07:41:22Z

Thanks for all your comments and feedback!

I think the PR is ready. The Cargo.toml from integration-tests can be simplify a lot (removing some features, runtime-benchmarks block which I think it's not needed, default-features = false parts, etc). But I think it's better to do this once @NunoAlexandre finalizes with the upgrade to avoid creating many conflicts.

NunoAlexandre · 2023-10-09T07:42:47Z

Thanks for all your comments and feedback!

I think the PR is ready. The Cargo.toml from integration-tests can be simplify a lot (removing some features, runtime-benchmarks block which I think it's not needed, default-features = false parts, etc). But I think it's better to do this once @NunoAlexandre finalizes with the upgrade to avoid creating many conflicts.

I think it's better if you move ahead, it's no problem for me to deal with the conflicts. The world needs this PR merged asap 😄

lemunozm · 2023-10-09T07:50:06Z

There would be a lot of line changes. I think we can merge it without touching Cargo.toml, and in another PR after the upgrade, simplify the dependencies. WDYT?

…ric-env-and-runtime

lemunozm · 2023-10-10T11:53:27Z

Ready for a final review/merge

lemunozm · 2023-10-11T17:38:56Z

I consider this PR done, although some minor parts will be modified in a next PR #1588 to be able to support a FudgeEnv

wischli

Looks great! We should streamline the setup process by adding setter functions for common tasks such as setting the authorities, see comment below. Not a blocker for sure.

wischli · 2023-10-12T07:47:58Z

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

+			.add(pallet_aura::GenesisConfig::<T> {
+				authorities: vec![AuraId::from(Keyring::Charlie.public())],
+			})


Not a blocker: The authority pattern is and will be heavily used. IMO, we should add setter functions for commonly executed calls such as this one, i.e. with_authorities(vec![AuraId::from(Keyring::Charlie.public())].

Yeah! The idea is to add this kind of thing. By now, in #1588 such genesis are moved to the environment because it's mandatory needed to make aura works.

It still need a lot of utilities for common stuff as creating assets, etc

mustermeiszer · 2023-10-16T08:39:41Z

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

+	/// Retrieve the fees used in the last submit call
+	fn last_fee(&self) -> Balance {
+		self.find_event(|e| match e {
+			pallet_transaction_payment::Event::TransactionFeePaid { actual_fee, .. } => {
+				Some(actual_fee)
+			}
+			_ => None,
+		})
+		.expect("Expected transaction")
+	}


This seems pretty detailed for the trait itself...

Yes, it is, and I've removed it in #1588

Now, submit_now() returns the fee it has cost. In order to later check in tests the exact balance required for some actions

Thanks for your post-morten review!

mustermeiszer · 2023-10-16T08:41:53Z

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

+		let cumulus_inherent = ParachainInherentData {
+			validation_data: PersistedValidationData {
+				parent_head: vec![].into(),
+				relay_parent_number: i,


Nit: I think this might become problemativ once we rely on the relay chain block number in some code locations. But not the case right now. But we will probably switch to the relay-chain as a our clock sooner than later.

once we rely on the relay chain block number in some code locations.

Makes sense, taken into account!

mustermeiszer · 2023-10-16T08:42:13Z

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

+			downward_messages: Default::default(),
+			horizontal_messages: Default::default(),


We could inject XCM messages here. That would be great.

@NunoAlexandre do you think we can get rid of the existing xcm-simulator like that?

This seems like a standalone env - no relay nor other parachains being involved. So, even if we populate the horizontal_messages, there's no receiving end for it afaict.

Could we mock the receiving end somehow?

The receiving end would have to be an actual parachain that's onboarded etc. Not sure if that's something that we want to add at this point.

I think that for the purpose of most integration tests, this setup is fine and we can leave it as it is. For other tests, such as XCM ones, we might need to use a fudge companion with multiple paras. So, I wouldn't mingle with this, for now.

Ok Cool!

BTW, I think for the FudgeEnv is quite easy to add new parachains, just adding it in the fudge::companion struct. Something to think about in the future, but I think it's feasible.

lemunozm added 12 commits October 5, 2023 22:50

Basic compiling draft

45168fa

refactor structure

200d1b4

example test working

25d054e

add Blocks enum

d05c5f5

different runtime implementations

18aeee6

fix altair & centrifuge

320945a

add event support

7eb6778

simplify weight calculation

99e6892

inmutable state method

dbf5ce9

API support

5a44c3b

file renaming

b4f4cb1

minor changes

649381b

lemunozm added I4-tests Test needs fixing or improving. P2-nice-to-have Issue is worth doing. crcl-protocol Circle protocol related. labels Oct 6, 2023

lemunozm self-assigned this Oct 6, 2023

lemunozm requested review from mustermeiszer, NunoAlexandre, wischli and cdamian as code owners October 6, 2023 09:53

lemunozm commented Oct 6, 2023

View reviewed changes

runtime/development/src/lib.rs Outdated Show resolved Hide resolved

improve macro support

0a7fa0b

lemunozm mentioned this pull request Oct 6, 2023

Integration tests (experimental runtime tests framework) #1574

Closed

2 tasks

wischli reviewed Oct 6, 2023

View reviewed changes

lemunozm added 3 commits October 6, 2023 17:46

minor style changes. add failing check_fees test

0d514bf

fees workings

b65d350

remove unused things

b3e52fd

NunoAlexandre reviewed Oct 9, 2023

View reviewed changes

update to use Tokens in treasury configs

e8fedd5

lemunozm force-pushed the integration-test/generic-env-and-runtime branch from 05228ce to e8fedd5 Compare October 9, 2023 07:23

Merge remote-tracking branch 'origin/main' into integration-test/gene…

8ef7fe8

…ric-env-and-runtime

lemunozm mentioned this pull request Oct 9, 2023

Integration-test: Generic environment for fudge #1586

Closed

minor rename

ef9955d

Merge remote-tracking branch 'origin/main' into integration-test/gene…

d562ace

…ric-env-and-runtime

lemunozm requested review from wischli and NunoAlexandre October 10, 2023 11:53

lemunozm mentioned this pull request Oct 10, 2023

Integration tests: fudge support for the generic environment #1588

Merged

7 tasks

wischli approved these changes Oct 12, 2023

View reviewed changes

lemunozm merged commit b8effb0 into main Oct 12, 2023
10 checks passed

mustermeiszer reviewed Oct 16, 2023

View reviewed changes

		@@ -0,0 +1 @@
		// TODO: implement generic::env::Env for an environment using fudge

		downward_messages: Default::default(),
		horizontal_messages: Default::default(),

Integration test: Generic runtime and environment #1583

Integration test: Generic runtime and environment #1583

Conversation

lemunozm commented Oct 6, 2023 • edited Loading

Description

Pending extra changes for this PR:

lemunozm left a 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

Choose a reason for hiding this comment

wischli Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wischli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NunoAlexandre left a 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

Choose a reason for hiding this comment

lemunozm commented Oct 9, 2023 • edited Loading

lemunozm commented Oct 9, 2023

NunoAlexandre commented Oct 9, 2023

lemunozm commented Oct 9, 2023

lemunozm commented Oct 10, 2023

lemunozm commented Oct 11, 2023

wischli left a 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

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

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

Choose a reason for hiding this comment

lemunozm commented Oct 6, 2023 •

edited

Loading

wischli Oct 9, 2023 •

edited

Loading

lemunozm commented Oct 9, 2023 •

edited

Loading