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

Fix deprecated inbound fixtures #1011

Merged
merged 4 commits into from
Nov 20, 2023
Merged

Fix deprecated inbound fixtures #1011

merged 4 commits into from
Nov 20, 2023

Conversation

yrong
Copy link
Contributor

@yrong yrong commented Nov 16, 2023

Resolves: SNO-749

Btw, met a compiling error rust-lang/rust#112637 several times in my local setup, as suggested in the thread upgrade rust to 1.71.

Copy link

codecov bot commented Nov 16, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ae1358f) 80.92% compared to head (857c45d) 80.92%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1011   +/-   ##
=======================================
  Coverage   80.92%   80.92%           
=======================================
  Files          52       52           
  Lines        2155     2155           
  Branches       73       73           
=======================================
  Hits         1744     1744           
  Misses        395      395           
  Partials       16       16           
Flag Coverage Δ
rust 81.11% <100.00%> (ø)

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.

@yrong yrong marked this pull request as ready for review November 16, 2023 08:00
Copy link
Collaborator

@vgeddes vgeddes left a comment

Choose a reason for hiding this comment

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

Ron, I am not convinced this makes our tests any better.

parachain/pallets/inbound-queue/src/mock.rs Outdated Show resolved Hide resolved
parachain/primitives/router/src/inbound/mod.rs Outdated Show resolved Hide resolved
parachain/pallets/inbound-queue/src/lib.rs Outdated Show resolved Hide resolved
parachain/pallets/inbound-queue/src/test.rs Outdated Show resolved Hide resolved
parachain/pallets/inbound-queue/src/test.rs Outdated Show resolved Hide resolved
{
System: frame_system::{Pallet, Call, Storage, Event<T>},
Balances: pallet_balances::{Pallet, Call, Storage, Config<T>, Event<T>},
EthereumBeaconClient: snowbridge_ethereum_beacon_client::{Pallet, Call, Storage, Event<T>},
Copy link
Collaborator

@vgeddes vgeddes Nov 16, 2023

Choose a reason for hiding this comment

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

Also, I'm not sure how the beacon client pallet ended up in the mock runtime, sometime in the past. We don't really need as it, since there is a MockVerifier.

And the MockVerifier will verify any dummy proof, which is good enough to let us test the inbound queue processing logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is because I needed to implement the trait BenchmarkHelper https://github.com/Snowfork/snowbridge/blob/main/parachain/pallets/inbound-queue/src/test.rs#L140 that I added so that the inbound queue benchmarks would include the log verification in the inbound queue pallet.

@yrong
Copy link
Contributor Author

yrong commented Nov 16, 2023

I am not convinced this makes our tests any better.

@vgeddes Actually the previous fixture is deprecated and does not work any more so we need to update it.

cargo run --release -p polkadot-parachain-bin --bin polkadot-parachain \
--features runtime-benchmarks \
-- \
benchmark pallet \
--chain=bridge-hub-rococo-dev \
--pallet=snowbridge_inbound_queue \
--extrinsic="*" \
--execution=wasm --wasm-execution=compiled \
--steps 50 --repeat 2 --extra \
--output ./parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/snowbridge_inbound_queue.rs

2023-11-16 20:51:37.451  INFO main ethereum-beacon-client: 💫 Verifying message with block hash 0x5f46…0363
2023-11-16 20:51:37.451 ERROR main ethereum-beacon-client: 💫 Verification of receipt inclusion failed for block 0x5f46…0363: <wasm:stripped>

Since we've already introduced EthereumBeaconClient for the benchmark and have fixture data for it. So I would prefer to not use the mock and reuse the fixture for both benchmark and the unit test.

@vgeddes
Copy link
Collaborator

vgeddes commented Nov 16, 2023

Actually the previous fixture is deprecated and does not work any more so we need to update it.

Thanks for catching this - I've cut SNO-749 to track it.

Since we've already introduced EthereumBeaconClient for the benchmark and have fixture data for it. So I would prefer to not use the mock and reuse the fixture for both benchmark and the unit test.

We can't really reuse the the benchmarking fixture for all the negative tests - the ones that test invalid messages. So we need mocks for those anyway. So if we're going to use mocks, lets be consistent with how we use them.

There are already a bunch of tests in the beacon-client pallet for testing verification of receipts and inclusion of logs in receipts. I don't want to duplicate those tests in the inbound-queue pallet.

The inbound-queue tests should treat T::Verifier as a black box and not be concerned whether it works correctly or not, because it is not the focus of the tests.

For accurate benchmarking on the other hand, it is different matter, and the benchmarking code needs to initialize a beacon client. But this is still separate from unit tests.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
Integralist Mark McDonnell
@yrong yrong force-pushed the ron/improve-tests branch from a468a5a to ce8a9eb Compare November 16, 2023 15:59
@yrong yrong changed the title Improve inbound tests Fix deprecated inbound fixtures Nov 16, 2023
Copy link
Collaborator

@vgeddes vgeddes left a comment

Choose a reason for hiding this comment

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

Looks much better and focused! Though consider upgrading to Rust 1.73.0

@@ -3,7 +3,7 @@
# https://github.com/rust-lang/rustup/issues/2686
# The auto-installation behaviour in rustup will likely be removed:
# https://github.com/rust-lang/rustup/issues/1397
channel = "1.70.0"
channel = "1.71.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 yrong requested a review from alistair-singh November 17, 2023 01:18
@yrong yrong merged commit 75dbf3f into main Nov 20, 2023
7 checks passed
@yrong yrong deleted the ron/improve-tests branch November 20, 2023 01:03
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.

3 participants