From f4fbddec420384e3550703ea7c9db3ea923f66e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Andr=C3=A9s=20Dorado=20Su=C3=A1rez?= Date: Thu, 7 Mar 2024 18:19:52 -0500 Subject: [PATCH] fix(pallet-benchmarking): split test functions in v2 (#3574) Closes #376 --------- Co-authored-by: command-bot <> --- .../collator-selection/src/benchmarking.rs | 4 +- .../collective-content/src/benchmarking.rs | 2 +- .../runtime/common/src/identity_migrator.rs | 2 +- .../parachains/src/hrmp/benchmarking.rs | 2 +- prdoc/pr_3574.prdoc | 23 +++++++++++ substrate/frame/alliance/src/benchmarking.rs | 2 +- substrate/frame/identity/src/benchmarking.rs | 4 +- substrate/frame/lottery/src/benchmarking.rs | 1 - .../frame/support/procedural/src/benchmark.rs | 38 ++++++++++++++++++- .../system/benchmarking/src/extensions.rs | 2 +- .../frame/system/benchmarking/src/lib.rs | 2 +- 11 files changed, 67 insertions(+), 15 deletions(-) create mode 100644 prdoc/pr_3574.prdoc diff --git a/cumulus/pallets/collator-selection/src/benchmarking.rs b/cumulus/pallets/collator-selection/src/benchmarking.rs index 2c40f4dd0eac..e2af74a6e60e 100644 --- a/cumulus/pallets/collator-selection/src/benchmarking.rs +++ b/cumulus/pallets/collator-selection/src/benchmarking.rs @@ -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; diff --git a/cumulus/parachains/pallets/collective-content/src/benchmarking.rs b/cumulus/parachains/pallets/collective-content/src/benchmarking.rs index 943386a84276..3d6bf073778a 100644 --- a/cumulus/parachains/pallets/collective-content/src/benchmarking.rs +++ b/cumulus/parachains/pallets/collective-content/src/benchmarking.rs @@ -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, I: 'static>(generic_event: >::RuntimeEvent) { diff --git a/polkadot/runtime/common/src/identity_migrator.rs b/polkadot/runtime/common/src/identity_migrator.rs index 0dfb03b06ba3..bf334a63e958 100644 --- a/polkadot/runtime/common/src/identity_migrator.rs +++ b/polkadot/runtime/common/src/identity_migrator.rs @@ -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; diff --git a/polkadot/runtime/parachains/src/hrmp/benchmarking.rs b/polkadot/runtime/parachains/src/hrmp/benchmarking.rs index 2cb49c88d437..c6baf2f30cf5 100644 --- a/polkadot/runtime/parachains/src/hrmp/benchmarking.rs +++ b/polkadot/runtime/parachains/src/hrmp/benchmarking.rs @@ -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 = diff --git a/prdoc/pr_3574.prdoc b/prdoc/pr_3574.prdoc new file mode 100644 index 000000000000..99868ea8a132 --- /dev/null +++ b/prdoc/pr_3574.prdoc @@ -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 diff --git a/substrate/frame/alliance/src/benchmarking.rs b/substrate/frame/alliance/src/benchmarking.rs index 9fe0e29b42cd..4ccd0fc08f6a 100644 --- a/substrate/frame/alliance/src/benchmarking.rs +++ b/substrate/frame/alliance/src/benchmarking.rs @@ -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}; diff --git a/substrate/frame/identity/src/benchmarking.rs b/substrate/frame/identity/src/benchmarking.rs index fe2fb0b04893..e0b45eeecd1c 100644 --- a/substrate/frame/identity/src/benchmarking.rs +++ b/substrate/frame/identity/src/benchmarking.rs @@ -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}, diff --git a/substrate/frame/lottery/src/benchmarking.rs b/substrate/frame/lottery/src/benchmarking.rs index 1510d250dbea..123b425b976f 100644 --- a/substrate/frame/lottery/src/benchmarking.rs +++ b/substrate/frame/lottery/src/benchmarking.rs @@ -23,7 +23,6 @@ use super::*; use crate::Pallet as Lottery; use frame_benchmarking::{ - impl_benchmark_test_suite, v1::{account, whitelisted_caller, BenchmarkError}, v2::*, }; diff --git a/substrate/frame/support/procedural/src/benchmark.rs b/substrate/frame/support/procedural/src/benchmark.rs index 6ded82d91aa5..d16ad2aaa515 100644 --- a/substrate/frame/support/procedural/src/benchmark.rs +++ b/substrate/frame/support/procedural/src/benchmark.rs @@ -431,7 +431,7 @@ pub fn benchmarks( let mut benchmarks_by_name_mappings: Vec = Vec::new(); let test_idents: Vec = 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]; @@ -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) @@ -676,6 +707,8 @@ pub fn benchmarks( } } } + + #impl_test_function } #mod_vis use #mod_name::*; }; @@ -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); diff --git a/substrate/frame/system/benchmarking/src/extensions.rs b/substrate/frame/system/benchmarking/src/extensions.rs index 1721501dead4..bfc3b4343a6c 100644 --- a/substrate/frame/system/benchmarking/src/extensions.rs +++ b/substrate/frame/system/benchmarking/src/extensions.rs @@ -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, diff --git a/substrate/frame/system/benchmarking/src/lib.rs b/substrate/frame/system/benchmarking/src/lib.rs index ebd16275bcbf..c9f0869d3bc8 100644 --- a/substrate/frame/system/benchmarking/src/lib.rs +++ b/substrate/frame/system/benchmarking/src/lib.rs @@ -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;