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(pallet-benchmarking): split test functions in v2 #3574

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions cumulus/pallets/collator-selection/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,7 @@ use super::*;
#[allow(unused)]
use crate::Pallet as CollatorSelection;
use codec::Decode;
use frame_benchmarking::{
account, impl_benchmark_test_suite, v2::*, whitelisted_caller, BenchmarkError,
};
use frame_benchmarking::{account, v2::*, whitelisted_caller, BenchmarkError};
use frame_support::traits::{Currency, EnsureOrigin, Get, ReservableCurrency};
use frame_system::{pallet_prelude::BlockNumberFor, EventRecord, RawOrigin};
use pallet_authorship::EventHandler;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
//! The pallet benchmarks.

use super::{Pallet as CollectiveContent, *};
use frame_benchmarking::{impl_benchmark_test_suite, v2::*};
use frame_benchmarking::v2::*;
use frame_support::traits::EnsureOrigin;

fn assert_last_event<T: Config<I>, I: 'static>(generic_event: <T as Config<I>>::RuntimeEvent) {
Expand Down
2 changes: 1 addition & 1 deletion polkadot/runtime/common/src/identity_migrator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ use pallet_identity;
use sp_core::Get;

#[cfg(feature = "runtime-benchmarks")]
use frame_benchmarking::{account, impl_benchmark_test_suite, v2::*, BenchmarkError};
use frame_benchmarking::{account, v2::*, BenchmarkError};

pub trait WeightInfo {
fn reap_identity(r: u32, s: u32) -> Weight;
Expand Down
2 changes: 1 addition & 1 deletion polkadot/runtime/parachains/src/hrmp/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use crate::{
paras::{Pallet as Paras, ParaKind, ParachainsCache},
shared::Pallet as Shared,
};
use frame_benchmarking::{impl_benchmark_test_suite, v2::*, whitelisted_caller};
use frame_benchmarking::{v2::*, whitelisted_caller};
use frame_support::{assert_ok, traits::Currency};

type BalanceOf<T> =
Expand Down
23 changes: 23 additions & 0 deletions prdoc/pr_3574.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
title: Generate test functions for each benchmark with benchmarking v2

doc:
- audience: Runtime Dev
description: |
This PR fixes an issue where using `impl_benchmark_test_suite` macro
within modules that use the benchmarking v2 macros (`#[benchmarks]`
and `#[instance_benchmarks]`) always produced a single test called
`test_benchmarks` instead of a separate benchmark test for every
benchmark (noted with the `#[benchmark]` macro).

By using this macro from now on, new tests will be created named
`test_benchmark_{name}` where `name` is the name of the benchmark
function. Those tests will be nested inside the module intended for
benchmark functions.

Also, when using `impl_benchmark_test_suite` inside the module,
the import of such marco will not be necessary, so any explicit
import of it will be marked as unused, the same way it works for
v1 macros so far.

crates:
- name: frame-support-procedural
2 changes: 1 addition & 1 deletion substrate/frame/alliance/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use core::{
};
use sp_runtime::traits::{Bounded, Hash, StaticLookup};

use frame_benchmarking::{account, impl_benchmark_test_suite, v2::*, BenchmarkError};
use frame_benchmarking::{account, v2::*, BenchmarkError};
use frame_support::traits::{EnsureOrigin, Get, UnfilteredDispatchable};
use frame_system::{pallet_prelude::BlockNumberFor, Pallet as System, RawOrigin as SystemOrigin};

Expand Down
4 changes: 1 addition & 3 deletions substrate/frame/identity/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,7 @@ use super::*;

use crate::Pallet as Identity;
use codec::Encode;
use frame_benchmarking::{
account, impl_benchmark_test_suite, v2::*, whitelisted_caller, BenchmarkError,
};
use frame_benchmarking::{account, v2::*, whitelisted_caller, BenchmarkError};
use frame_support::{
assert_ok, ensure,
traits::{EnsureOrigin, Get, OnFinalize, OnInitialize},
Expand Down
1 change: 0 additions & 1 deletion substrate/frame/lottery/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ use super::*;

use crate::Pallet as Lottery;
use frame_benchmarking::{
impl_benchmark_test_suite,
v1::{account, whitelisted_caller, BenchmarkError},
v2::*,
};
Expand Down
38 changes: 36 additions & 2 deletions substrate/frame/support/procedural/src/benchmark.rs
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,7 @@ pub fn benchmarks(
let mut benchmarks_by_name_mappings: Vec<TokenStream2> = Vec::new();
let test_idents: Vec<Ident> = benchmark_names_str
.iter()
.map(|n| Ident::new(format!("test_{}", n).as_str(), Span::call_site()))
.map(|n| Ident::new(format!("test_benchmark_{}", n).as_str(), Span::call_site()))
.collect();
for i in 0..benchmark_names.len() {
let name_ident = &benchmark_names[i];
Expand All @@ -441,6 +441,37 @@ pub fn benchmarks(
benchmarks_by_name_mappings.push(quote!(#name_str => Self::#test_ident()))
}

let impl_test_function = content
.iter_mut()
.find_map(|item| {
let Item::Macro(item_macro) = item else {
return None;
};

if !item_macro
.mac
.path
.segments
.iter()
.any(|s| s.ident == "impl_benchmark_test_suite")
{
return None;
}

let tokens = item_macro.mac.tokens.clone();
*item = Item::Verbatim(quote! {});

Some(quote! {
impl_test_function!(
(#( {} #benchmark_names )*)
(#( #extra_benchmark_names )*)
(#( #skip_meta_benchmark_names )*)
#tokens
);
})
})
.unwrap_or(quote! {});

// emit final quoted tokens
let res = quote! {
#(#mod_attrs)
Expand Down Expand Up @@ -676,6 +707,8 @@ pub fn benchmarks(
}
}
}

#impl_test_function
}
#mod_vis use #mod_name::*;
};
Expand Down Expand Up @@ -733,7 +766,8 @@ fn expand_benchmark(
let setup_stmts = benchmark_def.setup_stmts;
let verify_stmts = benchmark_def.verify_stmts;
let last_stmt = benchmark_def.last_stmt;
let test_ident = Ident::new(format!("test_{}", name.to_string()).as_str(), Span::call_site());
let test_ident =
Ident::new(format!("test_benchmark_{}", name.to_string()).as_str(), Span::call_site());

// unroll params (prepare for quoting)
let unrolled = UnrolledParams::from(&benchmark_def.params);
Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/system/benchmarking/src/extensions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

#![cfg(feature = "runtime-benchmarks")]

use frame_benchmarking::{account, impl_benchmark_test_suite, v2::*, BenchmarkError};
use frame_benchmarking::{account, v2::*, BenchmarkError};
use frame_support::{
dispatch::{DispatchClass, DispatchInfo, PostDispatchInfo},
weights::Weight,
Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/system/benchmarking/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
#![cfg(feature = "runtime-benchmarks")]

use codec::Encode;
use frame_benchmarking::{impl_benchmark_test_suite, v2::*};
use frame_benchmarking::v2::*;
use frame_support::{dispatch::DispatchClass, storage, traits::Get};
use frame_system::{Call, Pallet as System, RawOrigin};
use sp_core::storage::well_known_keys;
Expand Down
Loading