From f4fbddec420384e3550703ea7c9db3ea923f66e4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pablo=20Andr=C3=A9s=20Dorado=20Su=C3=A1rez?=
<hola@pablodorado.com>
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 2c40f4dd0ea..e2af74a6e60 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 943386a8427..3d6bf073778 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<T: Config<I>, I: 'static>(generic_event: <T as Config<I>>::RuntimeEvent) {
diff --git a/polkadot/runtime/common/src/identity_migrator.rs b/polkadot/runtime/common/src/identity_migrator.rs
index 0dfb03b06ba..bf334a63e95 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 2cb49c88d43..c6baf2f30cf 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<T> =
diff --git a/prdoc/pr_3574.prdoc b/prdoc/pr_3574.prdoc
new file mode 100644
index 00000000000..99868ea8a13
--- /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 9fe0e29b42c..4ccd0fc08f6 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 fe2fb0b0489..e0b45eeecd1 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 1510d250dbe..123b425b976 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 6ded82d91aa..d16ad2aaa51 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<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];
@@ -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 1721501dead..bfc3b4343a6 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 ebd16275bcb..c9f0869d3bc 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;
--
GitLab