mock: correct contextual/explicit parent assertions

When recording the parent of an event or span, the `MockCollector`
treats an explicit parent of `None` (i.e. an event or span that is an
explicit root) in the same way as if there is no explicit root. This
leads to it picking up the contextual parent or treating the event or
span as a contextual root.

This change refactors the recording of the parent to use `is_contextual`
to distinguish whether or not an explicit parent has been specified. The
actual parent is also written into a `Parent` enum so that the expected
and actual values can be compared in a more explicit way.

Additionally, the `Parent` struct has been moved into its own module and
the check behavior has been fixed. The error message has also been
unified across all cases.

Given the number of different cases involved in checking parents,
separate integration tests have been added to `tracing-mock`
specifically for testing all the positive and negative cases when
asserting on the parents of events and spans.

There were two tests in `tracing-attributes` which specified both an
explicit and a contextual parent. This behavior was never intended to
work as all events and spans are either contextual or not. The tests
have been corrected to only expect one of the two.

Fixes: #2440

Author: hds

tracing-attributes/tests/parents.rs

@@ -18,26 +18,15 @@ fn default_parent_test() {
     let child = expect::span().named("with_default_parent");
 
     let (collector, handle) = collector::mock()
-        .new_span(
-            contextual_parent
-                .clone()
-                .with_contextual_parent(None)
-                .with_explicit_parent(None),
-        )
-        .new_span(
-            child
-                .clone()
-                .with_contextual_parent(Some("contextual_parent"))
-                .with_explicit_parent(None),
-        )
+        .new_span(contextual_parent.clone().with_contextual_parent(None))
+        .new_span(child.clone().with_contextual_parent(None))
         .enter(child.clone())
         .exit(child.clone())
         .enter(contextual_parent.clone())
         .new_span(
             child
                 .clone()
-                .with_contextual_parent(Some("contextual_parent"))
-                .with_explicit_parent(None),
+                .with_contextual_parent(Some("contextual_parent")),
         )
         .enter(child.clone())
         .exit(child)
@@ -65,24 +54,10 @@ fn explicit_parent_test() {
     let child = expect::span().named("with_explicit_parent");
 
     let (collector, handle) = collector::mock()
-        .new_span(
-            contextual_parent
-                .clone()
-                .with_contextual_parent(None)
-                .with_explicit_parent(None),
-        )
-        .new_span(
-            explicit_parent
-                .with_contextual_parent(None)
-                .with_explicit_parent(None),
-        )
+        .new_span(contextual_parent.clone().with_contextual_parent(None))
+        .new_span(explicit_parent.with_contextual_parent(None))
         .enter(contextual_parent.clone())
-        .new_span(
-            child
-                .clone()
-                .with_contextual_parent(Some("contextual_parent"))
-                .with_explicit_parent(Some("explicit_parent")),
-        )
+        .new_span(child.clone().with_explicit_parent(Some("explicit_parent")))
         .enter(child.clone())
         .exit(child)
         .exit(contextual_parent)

tracing-mock/src/collector.rs

@@ -141,6 +141,7 @@ use crate::{
     event::ExpectedEvent,
     expect::Expect,
     field::ExpectedFields,
+    parent::Parent,
     span::{ExpectedSpan, NewSpan},
 };
 use std::{
@@ -1034,16 +1035,38 @@ where
                         )
                     }
                 }
-                let get_parent_name = || {
-                    let stack = self.current.lock().unwrap();
+
+                let get_parent = || {
                     let spans = self.spans.lock().unwrap();
-                    event
-                        .parent()
-                        .and_then(|id| spans.get(id))
-                        .or_else(|| stack.last().and_then(|id| spans.get(id)))
-                        .map(|s| s.name.to_string())
+
+                    if event.is_contextual() {
+                        let stack = self.current.lock().unwrap();
+                        if let Some(parent_id) = stack.last() {
+                            let contextual_parent = spans.get(parent_id).expect(
+                                "tracing-mock: contextual parent cannot \
+                                be looked up by ID. Was it recorded correctly?",
+                            );
+                            Parent::Contextual(contextual_parent.name.to_string())
+                        } else {
+                            Parent::ContextualRoot
+                        }
+                    } else {
+                        if event.is_root() {
+                            Parent::ExplicitRoot
+                        } else {
+                            let parent_id = event.parent().expect(
+                                "tracing-mock: is_contextual=false is_root=false \
+                                but no explicit parent found. This is a bug!",
+                            );
+                            let explicit_parent = spans.get(parent_id).expect(
+                                "tracing-mock: explicit parent cannot be looked \
+                                up by ID. Is the provided Span ID valid: {parent_id}",
+                            );
+                            Parent::Explicit(explicit_parent.name.to_string())
+                        }
+                    }
                 };
-                expected.check(event, get_parent_name, &self.name);
+                expected.check(event, get_parent, &self.name);
             }
             Some(ex) => ex.bad(&self.name, format_args!("observed event {:#?}", event)),
         }
@@ -1100,14 +1123,35 @@ where
         let mut spans = self.spans.lock().unwrap();
         if was_expected {
             if let Expect::NewSpan(mut expected) = expected.pop_front().unwrap() {
-                let get_parent_name = || {
-                    let stack = self.current.lock().unwrap();
-                    span.parent()
-                        .and_then(|id| spans.get(id))
-                        .or_else(|| stack.last().and_then(|id| spans.get(id)))
-                        .map(|s| s.name.to_string())
+                let get_parent = || {
+                    if span.is_contextual() {
+                        let stack = self.current.lock().unwrap();
+                        if let Some(parent_id) = stack.last() {
+                            let contextual_parent = spans.get(parent_id).expect(
+                                "tracing-mock: contextual parent cannot \
+                                be looked up by ID. Was it recorded correctly?",
+                            );
+                            Parent::Contextual(contextual_parent.name.to_string())
+                        } else {
+                            Parent::ContextualRoot
+                        }
+                    } else {
+                        if span.is_root() {
+                            Parent::ExplicitRoot
+                        } else {
+                            let parent_id = span.parent().expect(
+                                "tracing-mock: is_contextual=false is_root=false \
+                                but no explicit parent found. This is a bug!",
+                            );
+                            let explicit_parent = spans.get(parent_id).expect(
+                                "tracing-mock: explicit parent cannot be looked \
+                                up by ID. Is the provided Span ID valid: {parent_id}",
+                            );
+                            Parent::Explicit(explicit_parent.name.to_string())
+                        }
+                    }
                 };
-                expected.check(span, get_parent_name, &self.name);
+                expected.check(span, get_parent, &self.name);
             }
         }
         spans.insert(

tracing-mock/src/event.rs

@@ -29,7 +29,7 @@
 //! [`collector`]: mod@crate::collector
 //! [`expect::event`]: fn@crate::expect::event
 #![allow(missing_docs)]
-use super::{expect, field, metadata::ExpectedMetadata, span, Parent};
+use super::{expect, field, metadata::ExpectedMetadata, parent::Parent, span};
 
 use std::fmt;
 
@@ -307,6 +307,7 @@ impl ExpectedEvent {
     ///     .run_with_handle();
     ///
     /// with_default(collector, || {
+    ///     let _guard = tracing::info_span!("contextual parent").entered();
     ///     tracing::info!(parent: None, field = &"value");
     /// });
     ///
@@ -557,7 +558,7 @@ impl ExpectedEvent {
     pub(crate) fn check(
         &mut self,
         event: &tracing::Event<'_>,
-        get_parent_name: impl FnOnce() -> Option<String>,
+        get_parent: impl FnOnce() -> Parent,
         collector_name: &str,
     ) {
         let meta = event.metadata();
@@ -578,13 +579,14 @@ impl ExpectedEvent {
         }
 
         if let Some(ref expected_parent) = self.parent {
-            let actual_parent = get_parent_name();
-            expected_parent.check_parent_name(
-                actual_parent.as_deref(),
-                event.parent().cloned(),
-                event.metadata().name(),
-                collector_name,
-            )
+            let actual_parent = get_parent();
+            expected_parent.check(&actual_parent, event.metadata().name(), collector_name);
+            // expected_parent.check_parent_name(
+            //     actual_parent.as_deref(),
+            //     event.parent().cloned(),
+            //     event.metadata().name(),
+            //     collector_name,
+            // )
         }
     }
 }

tracing-mock/src/lib.rs

@@ -4,92 +4,8 @@ pub mod event;
 pub mod expect;
 pub mod field;
 mod metadata;
+mod parent;
 pub mod span;
 
 #[cfg(feature = "tracing-subscriber")]
 pub mod subscriber;
-
-#[derive(Debug, Eq, PartialEq)]
-pub enum Parent {
-    ContextualRoot,
-    Contextual(String),
-    ExplicitRoot,
-    Explicit(String),
-}
-
-impl Parent {
-    pub fn check_parent_name(
-        &self,
-        parent_name: Option<&str>,
-        provided_parent: Option<tracing_core::span::Id>,
-        ctx: impl std::fmt::Display,
-        collector_name: &str,
-    ) {
-        match self {
-            Parent::ExplicitRoot => {
-                assert!(
-                    provided_parent.is_none(),
-                    "[{}] expected {} to be an explicit root, but its parent was actually {:?} (name: {:?})",
-                    collector_name,
-                    ctx,
-                    provided_parent,
-                    parent_name,
-                );
-            }
-            Parent::Explicit(expected_parent) => {
-                assert!(
-                    provided_parent.is_some(),
-                    "[{}] expected {} to have explicit parent {}, but it has no explicit parent",
-                    collector_name,
-                    ctx,
-                    expected_parent,
-                );
-                assert_eq!(
-                    Some(expected_parent.as_ref()),
-                    parent_name,
-                    "[{}] expected {} to have explicit parent {}, but its parent was actually {:?} (name: {:?})",
-                    collector_name,
-                    ctx,
-                    expected_parent,
-                    provided_parent,
-                    parent_name,
-                );
-            }
-            Parent::ContextualRoot => {
-                assert!(
-                    provided_parent.is_none(),
-                    "[{}] expected {} to be a contextual root, but its parent was actually {:?} (name: {:?})",
-                    collector_name,
-                    ctx,
-                    provided_parent,
-                    parent_name,
-                );
-                assert!(
-                    parent_name.is_none(),
-                    "[{}] expected {} to be contextual a root, but we were inside span {:?}",
-                    collector_name,
-                    ctx,
-                    parent_name,
-                );
-            }
-            Parent::Contextual(expected_parent) => {
-                assert!(provided_parent.is_none(),
-                    "[{}] expected {} to have a contextual parent\nbut it has the explicit parent {:?} (name: {:?})",
-                    collector_name,
-                    ctx,
-                    provided_parent,
-                    parent_name,
-                );
-                assert_eq!(
-                    Some(expected_parent.as_ref()),
-                    parent_name,
-                    "[{}] expected {} to have contextual parent {:?}, but got {:?}",
-                    collector_name,
-                    ctx,
-                    expected_parent,
-                    parent_name,
-                );
-            }
-        }
-    }
-}

tracing-mock/src/parent.rs

@@ -0,0 +1,41 @@
+#[derive(Debug, Eq, PartialEq)]
+pub(crate) enum Parent {
+    ContextualRoot,
+    Contextual(String),
+    ExplicitRoot,
+    Explicit(String),
+}
+
+impl Parent {
+    #[track_caller]
+    pub(crate) fn check(
+        &self,
+        actual_parent: &Parent,
+        ctx: impl std::fmt::Display,
+        collector_name: &str,
+    ) {
+        let expected_description = |parent: &Parent| match parent {
+            Self::ExplicitRoot => format!("be an explicit root"),
+            Self::Explicit(name) => format!("have an explicit parent with name='{name}'"),
+            Self::ContextualRoot => format!("be a contextual root"),
+            Self::Contextual(name) => format!("have a contextual parent with name='{name}'"),
+        };
+
+        let actual_description = |parent: &Parent| match parent {
+            Self::ExplicitRoot => format!("was actually an explicit root"),
+            Self::Explicit(name) => format!("actually has an explicit parent with name='{name}'"),
+            Self::ContextualRoot => format!("was actually a contextual root"),
+            Self::Contextual(name) => {
+                format!("actually has a contextual parent with name='{name}'")
+            }
+        };
+
+        assert_eq!(
+            self,
+            actual_parent,
+            "[{collector_name}] expected {ctx} to {expected_description}, but {actual_description}",
+            expected_description = expected_description(self),
+            actual_description = actual_description(actual_parent)
+        );
+    }
+}

tracing-mock/src/span.rs

@@ -92,7 +92,7 @@
 //! [`expect::span`]: fn@crate::expect::span
 #![allow(missing_docs)]
 use crate::{
-    collector::SpanState, expect, field::ExpectedFields, metadata::ExpectedMetadata, Parent,
+    collector::SpanState, expect, field::ExpectedFields, metadata::ExpectedMetadata, parent::Parent,
 };
 use std::fmt;
 
@@ -699,7 +699,7 @@ impl NewSpan {
     pub(crate) fn check(
         &mut self,
         span: &tracing_core::span::Attributes<'_>,
-        get_parent_name: impl FnOnce() -> Option<String>,
+        get_parent: impl FnOnce() -> Parent,
         collector_name: &str,
     ) {
         let meta = span.metadata();
@@ -711,14 +711,13 @@ impl NewSpan {
         span.record(&mut checker);
         checker.finish();
 
-        if let Some(expected_parent) = self.parent.as_ref() {
-            let actual_parent = get_parent_name();
-            expected_parent.check_parent_name(
-                actual_parent.as_deref(),
-                span.parent().cloned(),
+        if let Some(ref expected_parent) = self.parent {
+            let actual_parent = get_parent();
+            expected_parent.check(
+                &actual_parent,
                 format_args!("span `{}`", name),
                 collector_name,
-            )
+            );
         }
     }
 }