From 6c14d97aefd8cddf03b8477a5e0c1447ebbcbf08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Andr=C3=A9s=20Dorado=20Su=C3=A1rez?= Date: Tue, 5 Mar 2024 04:42:43 -0500 Subject: [PATCH 1/7] fix(pallet-benchmarking): split test functions in v2 --- substrate/frame/benchmarking/src/v1.rs | 2 +- .../frame/support/procedural/src/benchmark.rs | 25 +++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/substrate/frame/benchmarking/src/v1.rs b/substrate/frame/benchmarking/src/v1.rs index 4ad8cc0edd46..dccaa992a47f 100644 --- a/substrate/frame/benchmarking/src/v1.rs +++ b/substrate/frame/benchmarking/src/v1.rs @@ -861,7 +861,7 @@ macro_rules! impl_bench_name_tests { // Same per-case logic as when all cases are run in the // same function. match std::panic::catch_unwind(|| { - $module::<$test>::[< test_benchmark_ $name >] () + $module::<$test>::test_bench_by_name(stringify!($name).as_bytes()) }) { Err(err) => { panic!("{}: {:?}", stringify!($name), err); diff --git a/substrate/frame/support/procedural/src/benchmark.rs b/substrate/frame/support/procedural/src/benchmark.rs index 6ded82d91aa5..bf3dba1540d2 100644 --- a/substrate/frame/support/procedural/src/benchmark.rs +++ b/substrate/frame/support/procedural/src/benchmark.rs @@ -441,6 +441,29 @@ pub fn benchmarks( benchmarks_by_name_mappings.push(quote!(#name_str => Self::#test_ident())) } + let impl_test_function = if let Some(item) = content.iter_mut().find(|item| matches!(item, Item::Macro(_))) { + let Item::Macro(item_macro) = item else { + return Err(syn::Error::new(item.span(), "Expected an item macro, as found such item")); + }; + + if item_macro + .mac + .path + .segments + .iter() + .any(|s| s.ident == "impl_benchmark_test_suite") + { + let tokens = item_macro.mac.tokens.clone(); + *item = Item::Verbatim(quote! {}); + + quote! { impl_test_function!((#( {} #benchmark_names )*)()()#tokens); } + } else { + quote! {} + } + } else { + quote! {} + }; + // emit final quoted tokens let res = quote! { #(#mod_attrs) @@ -676,6 +699,8 @@ pub fn benchmarks( } } } + + #impl_test_function } #mod_vis use #mod_name::*; }; From 9d6c510c85fee1b2ab85668eb575185f32fb3533 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Andr=C3=A9s=20Dorado=20Su=C3=A1rez?= Date: Tue, 5 Mar 2024 22:51:45 -0500 Subject: [PATCH 2/7] lint: remove unused imports --- cumulus/pallets/collator-selection/src/benchmarking.rs | 2 +- .../parachains/pallets/collective-content/src/benchmarking.rs | 2 +- polkadot/runtime/common/src/identity_migrator.rs | 2 +- polkadot/runtime/parachains/src/hrmp/benchmarking.rs | 2 +- substrate/frame/alliance/src/benchmarking.rs | 2 +- substrate/frame/identity/src/benchmarking.rs | 2 +- substrate/frame/lottery/src/benchmarking.rs | 1 - substrate/frame/system/benchmarking/src/extensions.rs | 2 +- substrate/frame/system/benchmarking/src/lib.rs | 2 +- 9 files changed, 8 insertions(+), 9 deletions(-) diff --git a/cumulus/pallets/collator-selection/src/benchmarking.rs b/cumulus/pallets/collator-selection/src/benchmarking.rs index 2c40f4dd0eac..f77c1a98960d 100644 --- a/cumulus/pallets/collator-selection/src/benchmarking.rs +++ b/cumulus/pallets/collator-selection/src/benchmarking.rs @@ -23,7 +23,7 @@ use super::*; use crate::Pallet as CollatorSelection; use codec::Decode; use frame_benchmarking::{ - account, impl_benchmark_test_suite, v2::*, whitelisted_caller, BenchmarkError, + account, v2::*, whitelisted_caller, BenchmarkError, }; use frame_support::traits::{Currency, EnsureOrigin, Get, ReservableCurrency}; use frame_system::{pallet_prelude::BlockNumberFor, EventRecord, RawOrigin}; 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/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..e8e606c4e406 100644 --- a/substrate/frame/identity/src/benchmarking.rs +++ b/substrate/frame/identity/src/benchmarking.rs @@ -24,7 +24,7 @@ use super::*; use crate::Pallet as Identity; use codec::Encode; use frame_benchmarking::{ - account, impl_benchmark_test_suite, v2::*, whitelisted_caller, BenchmarkError, + account, v2::*, whitelisted_caller, BenchmarkError, }; use frame_support::{ assert_ok, ensure, 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/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; From 4db6d0e73174788d9325bf40d6449cdb74ea37de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Andr=C3=A9s=20Dorado=20Su=C3=A1rez?= Date: Tue, 5 Mar 2024 23:37:13 -0500 Subject: [PATCH 3/7] change(frame-support-proc): include extras and skips on v2 splitted bench tests expansion --- .../frame/support/procedural/src/benchmark.rs | 42 +++++++++++-------- 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/substrate/frame/support/procedural/src/benchmark.rs b/substrate/frame/support/procedural/src/benchmark.rs index bf3dba1540d2..46665f59960a 100644 --- a/substrate/frame/support/procedural/src/benchmark.rs +++ b/substrate/frame/support/procedural/src/benchmark.rs @@ -85,12 +85,12 @@ impl syn::parse::Parse for BenchmarkAttrKeyword { let lookahead = input.lookahead1(); if lookahead.peek(keywords::extra) { let _extra: keywords::extra = input.parse()?; - return Ok(BenchmarkAttrKeyword::Extra) + return Ok(BenchmarkAttrKeyword::Extra); } else if lookahead.peek(keywords::skip_meta) { let _skip_meta: keywords::skip_meta = input.parse()?; - return Ok(BenchmarkAttrKeyword::SkipMeta) + return Ok(BenchmarkAttrKeyword::SkipMeta); } else { - return Err(lookahead.error()) + return Err(lookahead.error()); } } } @@ -104,13 +104,13 @@ impl syn::parse::Parse for BenchmarkAttrs { match arg { BenchmarkAttrKeyword::Extra => { if extra { - return Err(input.error("`extra` can only be specified once")) + return Err(input.error("`extra` can only be specified once")); } extra = true; }, BenchmarkAttrKeyword::SkipMeta => { if skip_meta { - return Err(input.error("`skip_meta` can only be specified once")) + return Err(input.error("`skip_meta` can only be specified once")); } skip_meta = true; }, @@ -169,7 +169,7 @@ fn ensure_valid_return_type(item_fn: &ItemFn) -> Result<()> { return Err(Error::new( typ.span(), "Only `Result<(), BenchmarkError>` or a blank return type is allowed on benchmark function definitions", - )) + )); }; let seg = path .segments @@ -179,7 +179,7 @@ fn ensure_valid_return_type(item_fn: &ItemFn) -> Result<()> { // ensure T in Result is () let Type::Tuple(tup) = res.unit else { return non_unit(res.unit.span()) }; if !tup.elems.is_empty() { - return non_unit(tup.span()) + return non_unit(tup.span()); } let TypePath { path, qself: _ } = res.e_type; let seg = path @@ -199,7 +199,7 @@ fn parse_params(item_fn: &ItemFn) -> Result> { return Err(Error::new( span, "Invalid benchmark function param. A valid example would be `x: Linear<5, 10>`.", - )) + )); }; let FnArg::Typed(arg) = arg else { return invalid_param(arg.span()) }; @@ -215,11 +215,11 @@ fn parse_params(item_fn: &ItemFn) -> Result> { }; let name = ident.ident.to_token_stream().to_string(); if name.len() > 1 { - return invalid_param_name() + return invalid_param_name(); }; let Some(name_char) = name.chars().next() else { return invalid_param_name() }; if !name_char.is_alphabetic() || !name_char.is_lowercase() { - return invalid_param_name() + return invalid_param_name(); } // parse type @@ -284,11 +284,12 @@ fn parse_call_def(item_fn: &ItemFn) -> Result<(usize, BenchmarkCallDef)> { Ok(match &call_defs[..] { [(i, call_def)] => (*i, call_def.clone()), // = 1 [] => return missing_call(item_fn), - _ => + _ => { return Err(Error::new( call_defs[1].1.attr_span(), "Only one #[extrinsic_call] or #[block] attribute is allowed per benchmark.", - )), + )) + }, }) } @@ -302,7 +303,9 @@ impl BenchmarkDef { let (verify_stmts, last_stmt) = match item_fn.sig.output { ReturnType::Default => // no return type, last_stmt should be None - (Vec::from(&item_fn.block.stmts[(i + 1)..item_fn.block.stmts.len()]), None), + { + (Vec::from(&item_fn.block.stmts[(i + 1)..item_fn.block.stmts.len()]), None) + }, ReturnType::Type(_, _) => { // defined return type, last_stmt should be Result<(), BenchmarkError> // compatible and should not be included in verify_stmts @@ -314,7 +317,7 @@ impl BenchmarkDef { defined a return type. You should return something compatible \ with Result<(), BenchmarkError> (i.e. `Ok(())`) as the last statement \ or change your signature to a blank return type.", - )) + )); } let Some(stmt) = item_fn.block.stmts.last() else { return missing_call(item_fn) }; ( @@ -441,7 +444,9 @@ pub fn benchmarks( benchmarks_by_name_mappings.push(quote!(#name_str => Self::#test_ident())) } - let impl_test_function = if let Some(item) = content.iter_mut().find(|item| matches!(item, Item::Macro(_))) { + let impl_test_function = if let Some(item) = + content.iter_mut().find(|item| matches!(item, Item::Macro(_))) + { let Item::Macro(item_macro) = item else { return Err(syn::Error::new(item.span(), "Expected an item macro, as found such item")); }; @@ -456,7 +461,7 @@ pub fn benchmarks( let tokens = item_macro.mac.tokens.clone(); *item = Item::Verbatim(quote! {}); - quote! { impl_test_function!((#( {} #benchmark_names )*)()()#tokens); } + quote! { impl_test_function!((#( {} #benchmark_names )*)(#( #extra_benchmark_names )*)(#( #skip_meta_benchmark_names )*)#tokens); } } else { quote! {} } @@ -856,8 +861,9 @@ fn expand_benchmark( }, ) }, - BenchmarkCallDef::Block { block, attr_span: _ } => - (quote!(), quote!(#block), quote!(#block)), + BenchmarkCallDef::Block { block, attr_span: _ } => { + (quote!(), quote!(#block), quote!(#block)) + }, }; let vis = benchmark_def.fn_vis; From 05b0820fd8c13e0e8d644814ac21968627c95884 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Andr=C3=A9s=20Dorado=20Su=C3=A1rez?= Date: Wed, 6 Mar 2024 11:12:04 -0500 Subject: [PATCH 4/7] change(frame-benchmarking): apply requested --- substrate/frame/benchmarking/src/v1.rs | 6 +-- .../frame/support/procedural/src/benchmark.rs | 51 +++++++++++-------- 2 files changed, 32 insertions(+), 25 deletions(-) diff --git a/substrate/frame/benchmarking/src/v1.rs b/substrate/frame/benchmarking/src/v1.rs index dccaa992a47f..4c423cc7da52 100644 --- a/substrate/frame/benchmarking/src/v1.rs +++ b/substrate/frame/benchmarking/src/v1.rs @@ -861,7 +861,7 @@ macro_rules! impl_bench_name_tests { // Same per-case logic as when all cases are run in the // same function. match std::panic::catch_unwind(|| { - $module::<$test>::test_bench_by_name(stringify!($name).as_bytes()) + $module::<$test>::[< test_benchmark_ $name >] () }) { Err(err) => { panic!("{}: {:?}", stringify!($name), err); @@ -920,7 +920,7 @@ macro_rules! impl_bench_name_tests { // Every variant must implement [`BenchmarkingSetup`]. // // ```nocompile -// +// // struct Transfer; // impl BenchmarkingSetup for Transfer { ... } // @@ -1866,7 +1866,7 @@ macro_rules! add_benchmark { verify, e, ); - return Err(e.into()) + return Err(e.into()); }, Err($crate::BenchmarkError::Skip) => { $crate::__private::log::error!( diff --git a/substrate/frame/support/procedural/src/benchmark.rs b/substrate/frame/support/procedural/src/benchmark.rs index 46665f59960a..0be69abd0be5 100644 --- a/substrate/frame/support/procedural/src/benchmark.rs +++ b/substrate/frame/support/procedural/src/benchmark.rs @@ -434,7 +434,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]; @@ -444,30 +444,36 @@ pub fn benchmarks( benchmarks_by_name_mappings.push(quote!(#name_str => Self::#test_ident())) } - let impl_test_function = if let Some(item) = - content.iter_mut().find(|item| matches!(item, Item::Macro(_))) - { - let Item::Macro(item_macro) = item else { - return Err(syn::Error::new(item.span(), "Expected an item macro, as found such item")); - }; + 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; + } - if item_macro - .mac - .path - .segments - .iter() - .any(|s| s.ident == "impl_benchmark_test_suite") - { let tokens = item_macro.mac.tokens.clone(); *item = Item::Verbatim(quote! {}); - quote! { impl_test_function!((#( {} #benchmark_names )*)(#( #extra_benchmark_names )*)(#( #skip_meta_benchmark_names )*)#tokens); } - } else { - quote! {} - } - } else { - 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! { @@ -763,7 +769,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); From 5312b58dfd6babe7aaaca62d0a7d8137ea0349a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Andr=C3=A9s=20Dorado=20Su=C3=A1rez?= Date: Wed, 6 Mar 2024 11:26:06 -0500 Subject: [PATCH 5/7] fix(pallet-support-procedural): remove unwanted formatting changes --- substrate/frame/benchmarking/src/v1.rs | 4 +-- .../frame/support/procedural/src/benchmark.rs | 36 +++++++++---------- 2 files changed, 18 insertions(+), 22 deletions(-) diff --git a/substrate/frame/benchmarking/src/v1.rs b/substrate/frame/benchmarking/src/v1.rs index 4c423cc7da52..4ad8cc0edd46 100644 --- a/substrate/frame/benchmarking/src/v1.rs +++ b/substrate/frame/benchmarking/src/v1.rs @@ -920,7 +920,7 @@ macro_rules! impl_bench_name_tests { // Every variant must implement [`BenchmarkingSetup`]. // // ```nocompile -// +// // struct Transfer; // impl BenchmarkingSetup for Transfer { ... } // @@ -1866,7 +1866,7 @@ macro_rules! add_benchmark { verify, e, ); - return Err(e.into()); + return Err(e.into()) }, Err($crate::BenchmarkError::Skip) => { $crate::__private::log::error!( diff --git a/substrate/frame/support/procedural/src/benchmark.rs b/substrate/frame/support/procedural/src/benchmark.rs index 0be69abd0be5..d16ad2aaa515 100644 --- a/substrate/frame/support/procedural/src/benchmark.rs +++ b/substrate/frame/support/procedural/src/benchmark.rs @@ -85,12 +85,12 @@ impl syn::parse::Parse for BenchmarkAttrKeyword { let lookahead = input.lookahead1(); if lookahead.peek(keywords::extra) { let _extra: keywords::extra = input.parse()?; - return Ok(BenchmarkAttrKeyword::Extra); + return Ok(BenchmarkAttrKeyword::Extra) } else if lookahead.peek(keywords::skip_meta) { let _skip_meta: keywords::skip_meta = input.parse()?; - return Ok(BenchmarkAttrKeyword::SkipMeta); + return Ok(BenchmarkAttrKeyword::SkipMeta) } else { - return Err(lookahead.error()); + return Err(lookahead.error()) } } } @@ -104,13 +104,13 @@ impl syn::parse::Parse for BenchmarkAttrs { match arg { BenchmarkAttrKeyword::Extra => { if extra { - return Err(input.error("`extra` can only be specified once")); + return Err(input.error("`extra` can only be specified once")) } extra = true; }, BenchmarkAttrKeyword::SkipMeta => { if skip_meta { - return Err(input.error("`skip_meta` can only be specified once")); + return Err(input.error("`skip_meta` can only be specified once")) } skip_meta = true; }, @@ -169,7 +169,7 @@ fn ensure_valid_return_type(item_fn: &ItemFn) -> Result<()> { return Err(Error::new( typ.span(), "Only `Result<(), BenchmarkError>` or a blank return type is allowed on benchmark function definitions", - )); + )) }; let seg = path .segments @@ -179,7 +179,7 @@ fn ensure_valid_return_type(item_fn: &ItemFn) -> Result<()> { // ensure T in Result is () let Type::Tuple(tup) = res.unit else { return non_unit(res.unit.span()) }; if !tup.elems.is_empty() { - return non_unit(tup.span()); + return non_unit(tup.span()) } let TypePath { path, qself: _ } = res.e_type; let seg = path @@ -199,7 +199,7 @@ fn parse_params(item_fn: &ItemFn) -> Result> { return Err(Error::new( span, "Invalid benchmark function param. A valid example would be `x: Linear<5, 10>`.", - )); + )) }; let FnArg::Typed(arg) = arg else { return invalid_param(arg.span()) }; @@ -215,11 +215,11 @@ fn parse_params(item_fn: &ItemFn) -> Result> { }; let name = ident.ident.to_token_stream().to_string(); if name.len() > 1 { - return invalid_param_name(); + return invalid_param_name() }; let Some(name_char) = name.chars().next() else { return invalid_param_name() }; if !name_char.is_alphabetic() || !name_char.is_lowercase() { - return invalid_param_name(); + return invalid_param_name() } // parse type @@ -284,12 +284,11 @@ fn parse_call_def(item_fn: &ItemFn) -> Result<(usize, BenchmarkCallDef)> { Ok(match &call_defs[..] { [(i, call_def)] => (*i, call_def.clone()), // = 1 [] => return missing_call(item_fn), - _ => { + _ => return Err(Error::new( call_defs[1].1.attr_span(), "Only one #[extrinsic_call] or #[block] attribute is allowed per benchmark.", - )) - }, + )), }) } @@ -303,9 +302,7 @@ impl BenchmarkDef { let (verify_stmts, last_stmt) = match item_fn.sig.output { ReturnType::Default => // no return type, last_stmt should be None - { - (Vec::from(&item_fn.block.stmts[(i + 1)..item_fn.block.stmts.len()]), None) - }, + (Vec::from(&item_fn.block.stmts[(i + 1)..item_fn.block.stmts.len()]), None), ReturnType::Type(_, _) => { // defined return type, last_stmt should be Result<(), BenchmarkError> // compatible and should not be included in verify_stmts @@ -317,7 +314,7 @@ impl BenchmarkDef { defined a return type. You should return something compatible \ with Result<(), BenchmarkError> (i.e. `Ok(())`) as the last statement \ or change your signature to a blank return type.", - )); + )) } let Some(stmt) = item_fn.block.stmts.last() else { return missing_call(item_fn) }; ( @@ -868,9 +865,8 @@ fn expand_benchmark( }, ) }, - BenchmarkCallDef::Block { block, attr_span: _ } => { - (quote!(), quote!(#block), quote!(#block)) - }, + BenchmarkCallDef::Block { block, attr_span: _ } => + (quote!(), quote!(#block), quote!(#block)), }; let vis = benchmark_def.fn_vis; From b502b6fee78985e042033c12ce9340d62b44ac6b Mon Sep 17 00:00:00 2001 From: command-bot <> Date: Thu, 7 Mar 2024 12:11:52 +0000 Subject: [PATCH 6/7] ".git/.scripts/commands/fmt/fmt.sh" --- cumulus/pallets/collator-selection/src/benchmarking.rs | 4 +--- substrate/frame/identity/src/benchmarking.rs | 4 +--- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/cumulus/pallets/collator-selection/src/benchmarking.rs b/cumulus/pallets/collator-selection/src/benchmarking.rs index f77c1a98960d..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, 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/substrate/frame/identity/src/benchmarking.rs b/substrate/frame/identity/src/benchmarking.rs index e8e606c4e406..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, v2::*, whitelisted_caller, BenchmarkError, -}; +use frame_benchmarking::{account, v2::*, whitelisted_caller, BenchmarkError}; use frame_support::{ assert_ok, ensure, traits::{EnsureOrigin, Get, OnFinalize, OnInitialize}, From be2d8c5a431a19d8fc1b1638b62b1d3f1f75501c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Andr=C3=A9s=20Dorado=20Su=C3=A1rez?= Date: Thu, 7 Mar 2024 13:26:19 -0500 Subject: [PATCH 7/7] chore: create prdoc --- prdoc/pr_3574.prdoc | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 prdoc/pr_3574.prdoc 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