Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Run frame_system integrity tests in Externalities #13092

Merged

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented Jan 6, 2023

storage parameter types are currently not usable as BlockWeight, since the integrity_test of frame_system runs outside of externalities and therefore fails. This MR just wraps it in externalities to allow that.

Having quickly adjustable block PoV weights would be useful for testing the limits on Rococo and Westmint.

@ggwpez ggwpez added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Jan 6, 2023
@ggwpez ggwpez changed the title Run frame_system integrity tests in Externalities Run frame_system integrity tests in Externalities Jan 6, 2023
@ggwpez ggwpez requested a review from kianenigma January 6, 2023 21:35
@@ -361,7 +361,9 @@ pub mod pallet {
#[pallet::hooks]
impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
fn integrity_test() {
T::BlockWeights::get().validate().expect("The weights are invalid.");
sp_io::TestExternalities::default().execute_with(|| {
Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like it tries to compile this as no-std when benchmarks are enabled.
This is only a test and should not end up in the runtime, right?
So #[cfg(feature = "std")] should be fine?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

We could maybe also thing of running everything in externalities.

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Let's just run all of them in an empty externalities?

Although, I wonder if anyone ever wants to use a custom on there.

@ggwpez
Copy link
Member Author

ggwpez commented Jan 8, 2023

Let's just run all of them in an empty externalities?

This was also my first thought, but was not sure about unintended consequences and there is not really an advantage.
Generally we dont need it, and if the user want to have it, they can still wrap it inside Externalities.

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez
Copy link
Member Author

ggwpez commented Jan 8, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 5cbd126 into master Jan 8, 2023
@paritytech-processbot paritytech-processbot bot deleted the oty-system-integrity-test-externalities branch January 8, 2023 17:03
ltfschoen pushed a commit to ltfschoen/substrate that referenced this pull request Feb 22, 2023
* Run frame_system integrity tests in Externalities

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Use feature = 'std'

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* Run frame_system integrity tests in Externalities

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Use feature = 'std'

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants