-
Notifications
You must be signed in to change notification settings - Fork 107
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this 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.
{ | ||
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>}, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@vgeddes Actually the previous fixture is deprecated and does not work any more so we need to update it.
Since we've already introduced |
Thanks for catching this - I've cut SNO-749 to track it.
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 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. |
a468a5a
to
ce8a9eb
Compare
There was a problem hiding this 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
rust-toolchain.toml
Outdated
@@ -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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Polkadot is already using 1.73.0: https://github.com/paritytech/polkadot-sdk/releases/tag/polkadot-v1.3.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.