Unverified Commit e9e7e73d authored by Bernhard Schuster's avatar Bernhard Schuster Committed by GitHub
Browse files

test: add unit test to catch missing distribution to subsystems faster (#2495)



* test: add unit test to catch missing distribution to subsystems faster

* add a simple count

* introduce proc macro to generate dispatch type

* refactor

* refactor

* chore: add license

* fixup unit test

* fixup merge

* better errors

* better fmt

* fix error spans

* better docs

* better error messages

* ui test foo

* Update node/subsystem/dispatch-gen/src/lib.rs
Co-authored-by: default avatarBastian Köcher <bkchr@users.noreply.github.com>

* Update node/network/bridge/src/lib.rs
Co-authored-by: default avatarBastian Köcher <bkchr@users.noreply.github.com>

* Update node/subsystem/Cargo.toml
Co-authored-by: default avatarBastian Köcher <bkchr@users.noreply.github.com>

* Update node/subsystem/dispatch-gen/src/lib.rs
Co-authored-by: default avatarBastian Köcher <bkchr@users.noreply.github.com>

* Update node/subsystem/dispatch-gen/src/lib.rs
Co-authored-by: default avatarBastian Köcher <bkchr@users.noreply.github.com>

* Update node/network/bridge/src/lib.rs
Co-authored-by: Andronik Ordian's avatarAndronik Ordian <write@reusable.software>

* fix compilation

* use find_map

* drop the silly 2, use _inner instead

* Update node/network/bridge/src/lib.rs
Co-authored-by: Andronik Ordian's avatarAndronik Ordian <write@reusable.software>

* Update node/subsystem/dispatch-gen/src/lib.rs
Co-authored-by: default avatarBastian Köcher <bkchr@users.noreply.github.com>

* nail deps down

* more into()

* flatten

* missing use statement

* fix messages order
Co-authored-by: default avatarBastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Andronik Ordian's avatarAndronik Ordian <write@reusable.software>
parent 01657e9d
Pipeline #125947 passed with stages
in 34 minutes and 13 seconds
......@@ -188,9 +188,9 @@ dependencies = [
[[package]]
name = "assert_matches"
version = "1.4.0"
version = "1.5.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "695579f0f2520f3774bb40461e5adb066459d4e0af4d59d20175484fb8e9edf1"
checksum = "9b34d609dfbaf33d6889b2b7106d3ca345eacad44200913df5ba02bfd31d2ba9"
[[package]]
name = "async-channel"
......@@ -5526,6 +5526,7 @@ dependencies = [
"polkadot-node-primitives",
"polkadot-node-subsystem-test-helpers",
"polkadot-primitives",
"polkadot-procmacro-subsystem-dispatch-gen",
"polkadot-statement-table",
"sc-network",
"smallvec 1.6.1",
......@@ -5686,6 +5687,17 @@ dependencies = [
"sp-version",
]
[[package]]
name = "polkadot-procmacro-subsystem-dispatch-gen"
version = "0.1.0"
dependencies = [
"assert_matches",
"proc-macro2",
"quote",
"syn",
"trybuild",
]
[[package]]
name = "polkadot-rpc"
version = "0.8.29"
......@@ -6402,9 +6414,9 @@ dependencies = [
[[package]]
name = "quote"
version = "1.0.7"
version = "1.0.9"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "aa563d17ecb180e500da1cfd2b028310ac758de548efdd203e18f283af693f37"
checksum = "c3d0b9745dc2debf507c8422de05d7226cc1f0644216dfdfead988f9b1ab32a7"
dependencies = [
"proc-macro2",
]
......@@ -9893,6 +9905,20 @@ dependencies = [
"structopt",
]
[[package]]
name = "trybuild"
version = "1.0.41"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "99471a206425fba51842a9186315f32d91c56eadc21ea4c21f847b59cf778f8b"
dependencies = [
"glob",
"lazy_static",
"serde",
"serde_json",
"termcolor",
"toml",
]
[[package]]
name = "twox-hash"
version = "1.5.0"
......
......@@ -65,6 +65,7 @@ members = [
"node/primitives",
"node/service",
"node/subsystem",
"node/subsystem/dispatch-gen",
"node/subsystem-test-helpers",
"node/subsystem-util",
"node/jaeger",
......
......@@ -14,27 +14,22 @@
// You should have received a copy of the GNU General Public License
// along with Substrate. If not, see <http://www.gnu.org/licenses/>.
use browser_utils::{browser_configuration, init_logging_and_telemetry, set_console_error_panic_hook, Client};
use log::info;
use wasm_bindgen::prelude::*;
use browser_utils::{
Client,
browser_configuration, init_logging_and_telemetry, set_console_error_panic_hook,
};
/// Starts the client.
#[wasm_bindgen]
pub async fn start_client(chain_spec: String, log_level: String) -> Result<Client, JsValue> {
start_inner(chain_spec, log_level)
.await
.map_err(|err| JsValue::from_str(&err.to_string()))
start_inner(chain_spec, log_level).await.map_err(|err| JsValue::from_str(&err.to_string()))
}
async fn start_inner(chain_spec: String, log_directives: String) -> Result<Client, Box<dyn std::error::Error>> {
set_console_error_panic_hook();
let telemetry_worker = init_logging_and_telemetry(&log_directives)?;
let chain_spec = service::PolkadotChainSpec::from_json_bytes(chain_spec.as_bytes().to_vec())
.map_err(|e| format!("{:?}", e))?;
let chain_spec =
service::PolkadotChainSpec::from_json_bytes(chain_spec.as_bytes().to_vec()).map_err(|e| format!("{:?}", e))?;
let telemetry_handle = telemetry_worker.handle();
let config = browser_configuration(chain_spec, Some(telemetry_handle)).await?;
......
......@@ -28,10 +28,8 @@ use polkadot_subsystem::{
SubsystemResult, jaeger,
};
use polkadot_subsystem::messages::{
NetworkBridgeMessage, AllMessages, AvailabilityDistributionMessage,
BitfieldDistributionMessage, PoVDistributionMessage, StatementDistributionMessage,
CollatorProtocolMessage, ApprovalDistributionMessage, NetworkBridgeEvent,
AvailabilityRecoveryMessage,
NetworkBridgeMessage, AllMessages,
CollatorProtocolMessage, NetworkBridgeEvent,
};
use polkadot_primitives::v1::{Hash, BlockNumber};
use polkadot_node_network_protocol::{
......@@ -565,35 +563,7 @@ async fn dispatch_validation_events_to_all<I>(
I: IntoIterator<Item = NetworkBridgeEvent<protocol_v1::ValidationProtocol>>,
I::IntoIter: Send,
{
let messages_for = |event: NetworkBridgeEvent<protocol_v1::ValidationProtocol>| {
let av_d = std::iter::once(event.focus().ok().map(|m| AllMessages::AvailabilityDistribution(
AvailabilityDistributionMessage::NetworkBridgeUpdateV1(m)
)));
let b = std::iter::once(event.focus().ok().map(|m| AllMessages::BitfieldDistribution(
BitfieldDistributionMessage::NetworkBridgeUpdateV1(m)
)));
let p = std::iter::once(event.focus().ok().map(|m| AllMessages::PoVDistribution(
PoVDistributionMessage::NetworkBridgeUpdateV1(m)
)));
let s = std::iter::once(event.focus().ok().map(|m| AllMessages::StatementDistribution(
StatementDistributionMessage::NetworkBridgeUpdateV1(m)
)));
let ap = std::iter::once(event.focus().ok().map(|m| AllMessages::ApprovalDistribution(
ApprovalDistributionMessage::NetworkBridgeUpdateV1(m)
)));
let av_r = std::iter::once(event.focus().ok().map(|m| AllMessages::AvailabilityRecovery(
AvailabilityRecoveryMessage::NetworkBridgeUpdateV1(m)
)));
av_d.chain(b).chain(p).chain(s).chain(ap).chain(av_r).filter_map(|x| x)
};
ctx.send_messages(events.into_iter().flat_map(messages_for)).await
ctx.send_messages(events.into_iter().flat_map(AllMessages::dispatch_iter)).await
}
#[tracing::instrument(level = "trace", skip(events, ctx), fields(subsystem = LOG_TARGET))]
......@@ -635,8 +605,12 @@ mod tests {
use polkadot_subsystem::{ActiveLeavesUpdate, FromOverseer, OverseerSignal};
use polkadot_subsystem::messages::{
StatementDistributionMessage, BitfieldDistributionMessage,
AvailabilityDistributionMessage,
AvailabilityRecoveryMessage,
ApprovalDistributionMessage,
BitfieldDistributionMessage,
PoVDistributionMessage,
StatementDistributionMessage
};
use polkadot_node_subsystem_test_helpers::{
SingleItemSink, SingleItemStream, TestSubsystemContextHandle,
......@@ -818,45 +792,47 @@ mod tests {
event: NetworkBridgeEvent<protocol_v1::ValidationProtocol>,
virtual_overseer: &mut TestSubsystemContextHandle<NetworkBridgeMessage>,
) {
// Ordering must match the enum variant order
// in `AllMessages`.
assert_matches!(
virtual_overseer.recv().await,
AllMessages::AvailabilityDistribution(
AvailabilityDistributionMessage::NetworkBridgeUpdateV1(e)
AllMessages::StatementDistribution(
StatementDistributionMessage::NetworkBridgeUpdateV1(e)
) if e == event.focus().expect("could not focus message")
);
assert_matches!(
virtual_overseer.recv().await,
AllMessages::BitfieldDistribution(
BitfieldDistributionMessage::NetworkBridgeUpdateV1(e)
AllMessages::AvailabilityDistribution(
AvailabilityDistributionMessage::NetworkBridgeUpdateV1(e)
) if e == event.focus().expect("could not focus message")
);
assert_matches!(
virtual_overseer.recv().await,
AllMessages::PoVDistribution(
PoVDistributionMessage::NetworkBridgeUpdateV1(e)
AllMessages::AvailabilityRecovery(
AvailabilityRecoveryMessage::NetworkBridgeUpdateV1(e)
) if e == event.focus().expect("could not focus message")
);
assert_matches!(
virtual_overseer.recv().await,
AllMessages::StatementDistribution(
StatementDistributionMessage::NetworkBridgeUpdateV1(e)
AllMessages::BitfieldDistribution(
BitfieldDistributionMessage::NetworkBridgeUpdateV1(e)
) if e == event.focus().expect("could not focus message")
);
assert_matches!(
virtual_overseer.recv().await,
AllMessages::ApprovalDistribution(
ApprovalDistributionMessage::NetworkBridgeUpdateV1(e)
AllMessages::PoVDistribution(
PoVDistributionMessage::NetworkBridgeUpdateV1(e)
) if e == event.focus().expect("could not focus message")
);
assert_matches!(
virtual_overseer.recv().await,
AllMessages::AvailabilityRecovery(
AvailabilityRecoveryMessage::NetworkBridgeUpdateV1(e)
AllMessages::ApprovalDistribution(
ApprovalDistributionMessage::NetworkBridgeUpdateV1(e)
) if e == event.focus().expect("could not focus message")
);
}
......@@ -1546,4 +1522,38 @@ mod tests {
}
});
}
#[test]
fn spread_event_to_subsystems_is_up_to_date() {
// Number of subsystems expected to be interested in a network event,
// and hence the network event broadcasted to.
const EXPECTED_COUNT: usize = 6;
let mut cnt = 0_usize;
for msg in AllMessages::dispatch_iter(NetworkBridgeEvent::PeerDisconnected(PeerId::random())) {
match msg {
AllMessages::CandidateValidation(_) => unreachable!("Not interested in network events"),
AllMessages::CandidateBacking(_) => unreachable!("Not interested in network events"),
AllMessages::CandidateSelection(_) => unreachable!("Not interested in network events"),
AllMessages::ChainApi(_) => unreachable!("Not interested in network events"),
AllMessages::CollatorProtocol(_) => unreachable!("Not interested in network events"),
AllMessages::StatementDistribution(_) => { cnt += 1; }
AllMessages::AvailabilityDistribution(_) => { cnt += 1; }
AllMessages::AvailabilityRecovery(_) => { cnt += 1; }
AllMessages::BitfieldDistribution(_) => { cnt += 1; }
AllMessages::BitfieldSigning(_) => unreachable!("Not interested in network events"),
AllMessages::Provisioner(_) => unreachable!("Not interested in network events"),
AllMessages::PoVDistribution(_) => { cnt += 1; }
AllMessages::RuntimeApi(_) => unreachable!("Not interested in network events"),
AllMessages::AvailabilityStore(_) => unreachable!("Not interested in network events"),
AllMessages::NetworkBridge(_) => unreachable!("Not interested in network events"),
AllMessages::CollationGeneration(_) => unreachable!("Not interested in network events"),
AllMessages::ApprovalVoting(_) => unreachable!("Not interested in network events"),
AllMessages::ApprovalDistribution(_) => { cnt += 1; }
// Add variants here as needed, `{ cnt += 1; }` for those that need to be
// notified, `unreachable!()` for those that should not.
}
}
assert_eq!(cnt, EXPECTED_COUNT);
}
}
......@@ -23,6 +23,7 @@ polkadot-node-network-protocol = { path = "../network/protocol" }
polkadot-primitives = { path = "../../primitives" }
polkadot-statement-table = { path = "../../statement-table" }
polkadot-node-jaeger = { path = "../jaeger" }
polkadot-procmacro-subsystem-dispatch-gen = { path = "dispatch-gen" }
sc-network = { git = "https://github.com/paritytech/substrate", branch = "master" }
smallvec = "1.6.1"
sp-core = { git = "https://github.com/paritytech/substrate", branch = "master" }
......
[package]
name = "polkadot-procmacro-subsystem-dispatch-gen"
version = "0.1.0"
authors = ["Parity Technologies <admin@parity.io>"]
edition = "2018"
description = "Small proc macro to create the distribution code for network events"
[lib]
proc-macro = true
[dependencies]
syn = { version = "1.0.60", features = ["full"] }
quote = "1.0.9"
proc-macro2 = "1.0.24"
assert_matches = "1.5.0"
[dev-dependencies]
trybuild = "1.0.41"
// Copyright 2021 Parity Technologies (UK) Ltd.
// This file is part of Polkadot.
// Polkadot is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
// Polkadot is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.
// You should have received a copy of the GNU General Public License
// along with Polkadot. If not, see <http://www.gnu.org/licenses/>.
use proc_macro2::TokenStream;
use quote::{quote, ToTokens};
use std::fmt;
use syn::{parse2, Error, Fields, FieldsNamed, FieldsUnnamed, Ident, ItemEnum, Path, Result, Type, Variant};
#[proc_macro_attribute]
pub fn subsystem_dispatch_gen(attr: proc_macro::TokenStream, item: proc_macro::TokenStream) -> proc_macro::TokenStream {
let attr: TokenStream = attr.into();
let item: TokenStream = item.into();
let mut backup = item.clone();
impl_subsystem_dispatch_gen(attr.into(), item).unwrap_or_else(|err| {
backup.extend(err.to_compile_error());
backup
}).into()
}
/// An enum variant without base type.
#[derive(Clone)]
struct EnumVariantDispatchWithTy {
// enum ty name
ty: Ident,
// variant
variant: EnumVariantDispatch,
}
impl fmt::Debug for EnumVariantDispatchWithTy {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "{}::{:?}", self.ty, self.variant)
}
}
impl ToTokens for EnumVariantDispatchWithTy {
fn to_tokens(&self, tokens: &mut proc_macro2::TokenStream) {
if let Some(inner) = &self.variant.inner {
let enum_name = &self.ty;
let variant_name = &self.variant.name;
let quoted = quote! {
#enum_name::#variant_name(#inner::from(event))
};
quoted.to_tokens(tokens);
}
}
}
/// An enum variant without the base type, contains the relevant inner type.
#[derive(Clone)]
struct EnumVariantDispatch {
/// variant name
name: Ident,
/// The inner type for which a `From::from` impl is anticipated from the input type.
/// No code will be generated for this enum variant if `inner` is `None`.
inner: Option<Type>,
}
impl fmt::Debug for EnumVariantDispatch {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "{}(..)", self.name)
}
}
fn prepare_enum_variant(variant: &mut Variant) -> Result<EnumVariantDispatch> {
let skip = variant.attrs.iter().find(|attr| attr.path.is_ident("skip")).is_some();
variant.attrs = variant.attrs.iter().filter(|attr| !attr.path.is_ident("skip")).cloned().collect::<Vec<_>>();
let variant = variant.clone();
let span = variant.ident.span();
let inner = match variant.fields.clone() {
// look for one called inner
Fields::Named(FieldsNamed { brace_token: _, named }) if !skip => named
.iter()
.find_map(
|field| {
if let Some(ident) = &field.ident {
if ident == "inner" {
return Some(Some(field.ty.clone()))
}
}
None
},
)
.ok_or_else(|| {
Error::new(span, "To dispatch with struct enum variant, one element must named `inner`")
})?,
// technically, if it has no inner types we cound not require the #[skip] annotation, but better make it consistent
Fields::Unnamed(FieldsUnnamed { paren_token: _, unnamed }) if !skip => unnamed
.first()
.map(|field| Some(field.ty.clone()))
.ok_or_else(|| Error::new(span, "Must be annotated with skip, even if no inner types exist."))?,
_ if skip => None,
Fields::Unit => {
return Err(Error::new(
span,
"Must be annotated with #[skip].",
))
}
Fields::Unnamed(_) => {
return Err(Error::new(
span,
"Must be annotated with #[skip] or have in `inner` element which impls `From<_>`.",
))
}
Fields::Named(_) => {
return Err(Error::new(
span,
"Must be annotated with #[skip] or the first wrapped type must impl `From<_>`.",
))
}
};
Ok(EnumVariantDispatch { name: variant.ident, inner })
}
fn impl_subsystem_dispatch_gen(attr: TokenStream, item: TokenStream) -> Result<proc_macro2::TokenStream> {
let event_ty = parse2::<Path>(attr)?;
let mut ie = parse2::<ItemEnum>(item)?;
let message_enum = ie.ident.clone();
let variants = ie.variants.iter_mut().try_fold(Vec::<EnumVariantDispatchWithTy>::new(), |mut acc, variant| {
let variant = prepare_enum_variant(variant)?;
if variant.inner.is_some() {
acc.push(EnumVariantDispatchWithTy { ty: message_enum.clone(), variant })
}
Ok::<_, syn::Error>(acc)
})?;
let mut orig = ie.to_token_stream();
let msg = "Generated by #[subsystem_dispatch_gen] proc-macro.";
orig.extend(quote! {
impl #message_enum {
#[doc = #msg]
pub fn dispatch_iter(event: #event_ty) -> impl Iterator<Item=Self> + Send {
let mut iter = None.into_iter();
#(
let mut iter = iter.chain(std::iter::once(event.focus().ok().map(|event| {
#variants
})));
)*
iter.filter_map(|x| x)
}
}
});
Ok(orig)
}
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn basic() {
let attr = quote! {
NetEvent<foo::Bar>
};
let item = quote! {
/// Documentation.
#[derive(Clone)]
enum AllMessages {
Sub1(Inner1),
#[skip]
/// D3
Sub3,
/// D4
#[skip]
Sub4(Inner2),
/// D2
Sub2(Inner2),
}
};
let output = impl_subsystem_dispatch_gen(attr, item).expect("Simple example always works. qed");
println!("//generated:");
println!("{}", output);
}
#[test]
fn ui() {
let t = trybuild::TestCases::new();
t.compile_fail("tests/ui/err-*.rs");
t.pass("tests/ui/ok-*.rs");
}
}
#![allow(dead_code)]
use polkadot_procmacro_subsystem_dispatch_gen::subsystem_dispatch_gen;
/// The event type in question.
#[derive(Clone, Copy)]
enum Event {
Smth,
Else,
}
impl Event {
fn focus(&self) -> std::result::Result<Inner, ()> {
unimplemented!("foo")
}
}
/// This should have a `From<Event>` impl but does not.
#[derive(Clone)]
enum Inner {
Foo,
Bar(Event),
}
#[subsystem_dispatch_gen(Event)]
#[derive(Clone)]
enum AllMessages {
/// Foo
Vvvvvv(Inner),
/// Missing a `#[skip]` annotation
Uuuuu,
}
fn main() {
let _x = AllMessages::dispatch_iter(Event::Else);
}
error: Must be annotated with #[skip].
--> $DIR/err-01-missing-skip.rs:32:5
|
32 | Uuuuu,
| ^^^^^
error[E0599]: no variant or associated item named `dispatch_iter` found for enum `AllMessages` in the current scope
--> $DIR/err-01-missing-skip.rs:36:27
|
27 | enum AllMessages {
| ---------------- variant or associated item `dispatch_iter` not found here
...
36 | let _x = AllMessages::dispatch_iter(Event::Else);
| ^^^^^^^^^^^^^ variant or associated item not found in `AllMessages`
#![allow(dead_code)]
use polkadot_procmacro_subsystem_dispatch_gen::subsystem_dispatch_gen;
/// The event type in question.
#[derive(Clone, Copy, Debug)]
enum Event {
Smth,
Else,
}
impl Event {
fn focus(&self) -> std::result::Result<Intermediate, ()> {
Ok(Intermediate(self.clone()))
}
}
#[derive(Debug, Clone)]
struct Intermediate(Event);
/// This should have a `From<Event>` impl but does not.
#[derive(Debug, Clone)]
enum Inner {
Foo,
Bar(Intermediate),
}
#[subsystem_dispatch_gen(Event)]
#[derive(Clone)]
enum AllMessages {
/// Foo
Vvvvvv(Inner),
#[skip]
Uuuuu,
}