Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds runtime tests #1114

Merged
merged 16 commits into from
Jan 26, 2024
Merged

Adds runtime tests #1114

merged 16 commits into from
Jan 26, 2024

Conversation

claravanstaden
Copy link
Contributor

@claravanstaden claravanstaden commented Jan 15, 2024

Adds runtime test for relayer extrinsics:

  • beacon relayer
  • execution relayer
  • check nonce and digest after sending transfer token message (digest is empty 🤷🏻‍♀️ )

Resolves: SNO-811
Polkadot-sdk companion: Snowfork/polkadot-sdk#98

@claravanstaden claravanstaden force-pushed the runtime-tests branch 2 times, most recently from c3ea24c to 2aab26b Compare January 25, 2024 07:39
Copy link

codecov bot commented Jan 25, 2024

Codecov Report

Attention: 97 lines in your changes are missing coverage. Please review.

Comparison is base (3b17d2a) 75.77% compared to head (801d17c) 50.09%.

Files Patch % Lines
parachain/runtime/test-common/src/lib.rs 0.00% 92 Missing ⚠️
...achain/pallets/ethereum-client/fixtures/src/lib.rs 0.00% 5 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1114       +/-   ##
===========================================
- Coverage   75.77%   50.09%   -25.68%     
===========================================
  Files          60       61        +1     
  Lines        2464     3727     +1263     
  Branches       72       72               
===========================================
  Hits         1867     1867               
- Misses        580     1843     +1263     
  Partials       17       17               
Flag Coverage Δ
rust 46.47% <0.00%> (-28.36%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@claravanstaden claravanstaden marked this pull request as ready for review January 25, 2024 14:23
Comment on lines 185 to 210
let digest = included_head.digest();

//let digest = frame_system::Pallet::<Runtime>::digest();
let digest_items = digest.logs();
assert!(digest_items.len() == 1 && digest_items[0].as_other().is_some());

//assert_eq!(Messages::<Test>::decode_len(), Some(4));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yrong I am checking the nonce and digest here after running to the next block. The nonce has increased, but the digest items are empty. Can you see anything wrong here? 😄

Copy link
Contributor

@yrong yrong Jan 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems it's because the run_to_block function in test-utils does not include the finalized phase. Actually the phase we commit the digest item.

I'll check if possible to make some fixes here.

Copy link
Contributor

@yrong yrong Jan 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@claravanstaden I've made #1126 to resolve the issue and the test passed. The bad thing it requires more changes in polkadot-sdk to add another run_to_bock_with_finalize to include the finalized phase. Though I think it is general enough but not sure worth introducing at this moment.

Another option is to limit the scope of the change and maybe add another run_to_block function in our test case to only finalize the pallets as required. Something like we do in the unit test

pub fn run_to_end_of_next_block() {
// finish current block
MessageQueue::on_finalize(System::block_number());
OutboundQueue::on_finalize(System::block_number());
System::on_finalize(System::block_number());
// start next block
System::set_block_number(System::block_number() + 1);
System::on_initialize(System::block_number());
OutboundQueue::on_initialize(System::block_number());
MessageQueue::on_initialize(System::block_number());
// finish next block
MessageQueue::on_finalize(System::block_number());
OutboundQueue::on_finalize(System::block_number());
System::on_finalize(System::block_number());
}

I’ve no preference and leave it up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @yrong! Thanks for the effort you put in here, I appreciate it. Let me try the local "run_to_end_of_next_block" in our test case first, as I guess that will be the least disruptive.

@@ -73,6 +75,7 @@ std = [
"serde",
"snowbridge-core/std",
"snowbridge-ethereum/std",
"snowbridge-pallet-ethereum-client-fixtures/std",
Copy link
Contributor

@yrong yrong Jan 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems the fixture crate is only for tests so unnecessary here?

Comment on lines +38 to +44
runtime-benchmarks = [
"frame-benchmarking",
"frame-benchmarking/runtime-benchmarks",
"frame-support/runtime-benchmarks",
"frame-system/runtime-benchmarks",
"snowbridge-core/runtime-benchmarks",
]
Copy link
Contributor

@yrong yrong Jan 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

runtime-benchmarks seems also unnecessary here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I referenced the inbound fixtures crate you added. 😅 https://github.com/Snowfork/snowbridge/blob/main/parachain/pallets/inbound-queue/fixtures/Cargo.toml#L38 Is it necessary in that crate?

Copy link
Contributor

@yrong yrong Jan 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, seems also unnecessary in that crate because IIUC we're using the fixture from local setup for the benchmark?

IMO the benefit of introducing the runtime-benchmarks feature is to differentiate the fixture between public network(goerli/sepolia) and local setup(hacked mainnet), As we know the fork version is different and we may want to use the real data(from public network) for the benchmark.

But it's nitpicky and not the scope of this PR, so feel free to ignore it.

Copy link
Contributor

@yrong yrong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@claravanstaden claravanstaden merged commit 252df12 into main Jan 26, 2024
7 checks passed
@claravanstaden claravanstaden deleted the runtime-tests branch January 26, 2024 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants