From cb9ec622721c13f63583484c105b05edc598a031 Mon Sep 17 00:00:00 2001 From: Hayden Stainsby Date: Tue, 29 Oct 2024 15:38:35 +0100 Subject: [PATCH] mock: improve ergonomics when an `ExpectedSpan` is needed (#3097) Many of the methods on `MockCollector` take an `ExpectedSpan`. This often requires significant boilerplate. For example, to expect that a span with a specific name enters and then exits, the following code is needed: ```rust let span = expect::span().named("span name"); let (collector, handle) = collector::mock() .enter(span.clone()) .exit(span) .run_with_handle(); ``` In order to make using `tracing-mock` more ergonomic and also more compact, the `MockCollector` and `MockSubscriber` methods that previous took an `ExpectedSpan`, are now generic over `Into`. There are currently 3 implementations of `From` for `ExpectedSpan` which allow the following shorthand uses: `T: Into` - an `ExpectedSpan` will be created that expects to have a name specified by `T`. ```rust let (collector, handle) = collector::mock() .enter("span name") .exit("span name") .run_with_handle(); ``` `&ExpectedId` - an `ExpectedSpan` will be created that expects to have the expected Id. A reference is taken and cloned internally because the caller always needs to use an `ExpectedId` in at least 2 calls to the mock collector/subscriber. ```rust let id = expect::id(); let (collector, handle) = collector::mock() .new_span(&id) .enter(&id) .run_with_handle(); ``` `&ExpectedSpan` - The expected span is taken by reference and cloned. ```rust let span = expect::span().named("span name"); let (collector, handle) = collector::mock() .enter(&span) .exit(&span) .run_with_handle(); ``` In Rust, taking a reference to an object and immediately cloning it is an anti-pattern. It is considered better to force the user to clone outside the API to make the cloning explict. However, in the case of a testing framework, it seems reasonable to prefer a more concise API, rather than having it more explicit. To reduce the size of this PR and to avoid unnecessary churn in other crates, the tests within the tracing repo which use `tracing-mock` will not be updated to use the new `Into` capabilities. The new API is backwards compatible and those tests can remain as they are. --- tracing-mock/src/collector.rs | 74 ++++++++++------- tracing-mock/src/span.rs | 142 +++++++++++++++++++++++++++------ tracing-mock/src/subscriber.rs | 42 +++++----- 3 files changed, 190 insertions(+), 68 deletions(-) diff --git a/tracing-mock/src/collector.rs b/tracing-mock/src/collector.rs index 89f74d05d..4789b7ce6 100644 --- a/tracing-mock/src/collector.rs +++ b/tracing-mock/src/collector.rs @@ -38,11 +38,11 @@ //! .named("my_span"); //! let (collector, handle) = collector::mock() //! // Enter a matching span -//! .enter(span.clone()) +//! .enter(&span) //! // Record an event with message "collect parting message" //! .event(expect::event().with_fields(expect::message("collect parting message"))) //! // Record a value for the field `parting` on a matching span -//! .record(span.clone(), expect::field("parting").with_value(&"goodbye world!")) +//! .record(&span, expect::field("parting").with_value(&"goodbye world!")) //! // Exit a matching span //! .exit(span) //! // Expect no further messages to be recorded @@ -80,9 +80,9 @@ //! let span = expect::span() //! .named("my_span"); //! let (collector, handle) = collector::mock() -//! .enter(span.clone()) +//! .enter(&span) //! .event(expect::event().with_fields(expect::message("collect parting message"))) -//! .record(span.clone(), expect::field("parting").with_value(&"goodbye world!")) +//! .record(&span, expect::field("parting").with_value(&"goodbye world!")) //! .exit(span) //! .only() //! .run_with_handle(); @@ -225,11 +225,11 @@ pub struct MockHandle(Arc>>, String); /// .named("my_span"); /// let (collector, handle) = collector::mock() /// // Enter a matching span -/// .enter(span.clone()) +/// .enter(&span) /// // Record an event with message "collect parting message" /// .event(expect::event().with_fields(expect::message("collect parting message"))) /// // Record a value for the field `parting` on a matching span -/// .record(span.clone(), expect::field("parting").with_value(&"goodbye world!")) +/// .record(&span, expect::field("parting").with_value(&"goodbye world!")) /// // Exit a matching span /// .exit(span) /// // Expect no further messages to be recorded @@ -472,8 +472,8 @@ where /// .at_level(tracing::Level::INFO) /// .named("the span we're testing"); /// let (collector, handle) = collector::mock() - /// .enter(span.clone()) - /// .exit(span) + /// .enter(&span) + /// .exit(&span) /// .only() /// .run_with_handle(); /// @@ -495,8 +495,8 @@ where /// .at_level(tracing::Level::INFO) /// .named("the span we're testing"); /// let (collector, handle) = collector::mock() - /// .enter(span.clone()) - /// .exit(span) + /// .enter(&span) + /// .exit(&span) /// .only() /// .run_with_handle(); /// @@ -511,8 +511,11 @@ where /// /// [`exit`]: fn@Self::exit /// [`only`]: fn@Self::only - pub fn enter(mut self, span: ExpectedSpan) -> Self { - self.expected.push_back(Expect::Enter(span)); + pub fn enter(mut self, span: S) -> Self + where + S: Into, + { + self.expected.push_back(Expect::Enter(span.into())); self } @@ -536,8 +539,8 @@ where /// .at_level(tracing::Level::INFO) /// .named("the span we're testing"); /// let (collector, handle) = collector::mock() - /// .enter(span.clone()) - /// .exit(span) + /// .enter(&span) + /// .exit(&span) /// .run_with_handle(); /// /// tracing::collect::with_default(collector, || { @@ -558,8 +561,8 @@ where /// .at_level(tracing::Level::INFO) /// .named("the span we're testing"); /// let (collector, handle) = collector::mock() - /// .enter(span.clone()) - /// .exit(span) + /// .enter(&span) + /// .exit(&span) /// .run_with_handle(); /// /// tracing::collect::with_default(collector, || { @@ -572,8 +575,11 @@ where /// ``` /// /// [`enter`]: fn@Self::enter - pub fn exit(mut self, span: ExpectedSpan) -> Self { - self.expected.push_back(Expect::Exit(span)); + pub fn exit(mut self, span: S) -> Self + where + S: Into, + { + self.expected.push_back(Expect::Exit(span.into())); self } @@ -627,8 +633,11 @@ where /// /// handle.assert_finished(); /// ``` - pub fn clone_span(mut self, span: ExpectedSpan) -> Self { - self.expected.push_back(Expect::CloneSpan(span)); + pub fn clone_span(mut self, span: S) -> Self + where + S: Into, + { + self.expected.push_back(Expect::CloneSpan(span.into())); self } @@ -644,8 +653,11 @@ where /// /// [`Collect::drop_span`]: fn@tracing::Collect::drop_span #[allow(deprecated)] - pub fn drop_span(mut self, span: ExpectedSpan) -> Self { - self.expected.push_back(Expect::DropSpan(span)); + pub fn drop_span(mut self, span: S) -> Self + where + S: Into, + { + self.expected.push_back(Expect::DropSpan(span.into())); self } @@ -710,9 +722,15 @@ where /// ``` /// /// [`Span::follows_from`]: fn@tracing::Span::follows_from - pub fn follows_from(mut self, consequence: ExpectedSpan, cause: ExpectedSpan) -> Self { - self.expected - .push_back(Expect::FollowsFrom { consequence, cause }); + pub fn follows_from(mut self, consequence: S1, cause: S2) -> Self + where + S1: Into, + S2: Into, + { + self.expected.push_back(Expect::FollowsFrom { + consequence: consequence.into(), + cause: cause.into(), + }); self } @@ -775,11 +793,13 @@ where /// ``` /// /// [`field`]: mod@crate::field - pub fn record(mut self, span: ExpectedSpan, fields: I) -> Self + pub fn record(mut self, span: S, fields: I) -> Self where + S: Into, I: Into, { - self.expected.push_back(Expect::Visit(span, fields.into())); + self.expected + .push_back(Expect::Visit(span.into(), fields.into())); self } diff --git a/tracing-mock/src/span.rs b/tracing-mock/src/span.rs index a568484f7..34568551b 100644 --- a/tracing-mock/src/span.rs +++ b/tracing-mock/src/span.rs @@ -18,8 +18,8 @@ //! .at_level(tracing::Level::INFO); //! //! let (collector, handle) = collector::mock() -//! .enter(span.clone()) -//! .exit(span) +//! .enter(&span) +//! .exit(&span) //! .run_with_handle(); //! //! tracing::collect::with_default(collector, || { @@ -30,6 +30,25 @@ //! handle.assert_finished(); //! ``` //! +//! Instead of passing an `ExpectedSpan`, the collector methods will also accept +//! anything that implements `Into` which is shorthand for +//! `expect::span().named(name)`. +//! +//! ``` +//! use tracing_mock::collector; +//! +//! let (collector, handle) = collector::mock() +//! .enter("interesting_span") +//! .run_with_handle(); +//! +//! tracing::collect::with_default(collector, || { +//! let span = tracing::info_span!("interesting_span"); +//! let _guard = span.enter(); +//! }); +//! +//! handle.assert_finished(); +//! ``` +// //! The following example asserts the name, level, parent, and fields of the span: //! //! ``` @@ -44,10 +63,10 @@ //! .with_ancestry(expect::has_explicit_parent("parent_span")); //! //! let (collector, handle) = collector::mock() -//! .new_span(expect::span().named("parent_span")) +//! .new_span("parent_span") //! .new_span(new_span) -//! .enter(span.clone()) -//! .exit(span) +//! .enter(&span) +//! .exit(&span) //! .run_with_handle(); //! //! tracing::collect::with_default(collector, || { @@ -75,8 +94,8 @@ //! .at_level(tracing::Level::INFO); //! //! let (collector, handle) = collector::mock() -//! .enter(span.clone()) -//! .exit(span) +//! .enter(&span) +//! .exit(&span) //! .run_with_handle(); //! //! tracing::collect::with_default(collector, || { @@ -115,6 +134,27 @@ pub struct ExpectedSpan { pub(crate) metadata: ExpectedMetadata, } +impl From for ExpectedSpan +where + I: Into, +{ + fn from(name: I) -> Self { + ExpectedSpan::default().named(name) + } +} + +impl From<&ExpectedId> for ExpectedSpan { + fn from(id: &ExpectedId) -> Self { + ExpectedSpan::default().with_id(id.clone()) + } +} + +impl From<&ExpectedSpan> for ExpectedSpan { + fn from(span: &ExpectedSpan) -> Self { + span.clone() + } +} + /// A mock new span. /// /// **Note**: This struct contains expectations that can only be asserted @@ -166,7 +206,8 @@ pub struct ExpectedId { impl ExpectedSpan { /// Sets a name to expect when matching a span. /// - /// If an event is recorded with a name that differs from the one provided to this method, the expectation will fail. + /// If an event is recorded with a name that differs from the one provided to this method, the + /// expectation will fail. /// /// # Examples /// @@ -187,6 +228,25 @@ impl ExpectedSpan { /// handle.assert_finished(); /// ``` /// + /// If only the name of the span needs to be validated, then + /// instead of using the `named` method, a string can be passed + /// to the [`MockCollector`] functions directly. + /// + /// ``` + /// use tracing_mock::collector; + /// + /// let (collector, handle) = collector::mock() + /// .enter("span name") + /// .run_with_handle(); + /// + /// tracing::collect::with_default(collector, || { + /// let span = tracing::info_span!("span name"); + /// let _guard = span.enter(); + /// }); + /// + /// handle.assert_finished(); + /// ``` + /// /// When the span name is different, the assertion will fail: /// /// ```should_panic @@ -205,6 +265,8 @@ impl ExpectedSpan { /// /// handle.assert_finished(); /// ``` + /// + /// [`MockCollector`]: struct@crate::collector::MockCollector pub fn named(self, name: I) -> Self where I: Into, @@ -247,10 +309,41 @@ impl ExpectedSpan { /// let span2 = expect::span().named("span").with_id(id2.clone()); /// /// let (collector, handle) = collector::mock() - /// .new_span(span1.clone()) - /// .new_span(span2.clone()) - /// .enter(span2) - /// .enter(span1) + /// .new_span(&span1) + /// .new_span(&span2) + /// .enter(&span2) + /// .enter(&span1) + /// .run_with_handle(); + /// + /// tracing::collect::with_default(collector, || { + /// fn create_span() -> tracing::Span { + /// tracing::info_span!("span") + /// } + /// + /// let span1 = create_span(); + /// let span2 = create_span(); + /// + /// let _guard2 = span2.enter(); + /// let _guard1 = span1.enter(); + /// }); + /// + /// handle.assert_finished(); + /// ``` + /// + /// Since `ExpectedId` implements `Into`, in cases where + /// only checking on Id is desired, a shorthand version of the previous + /// example can be used. + /// + /// ``` + /// use tracing_mock::{collector, expect}; + /// let id1 = expect::id(); + /// let id2 = expect::id(); + /// + /// let (collector, handle) = collector::mock() + /// .new_span(&id1) + /// .new_span(&id2) + /// .enter(&id2) + /// .enter(&id1) /// .run_with_handle(); /// /// tracing::collect::with_default(collector, || { @@ -279,10 +372,10 @@ impl ExpectedSpan { /// let span2 = expect::span().named("span").with_id(id2.clone()); /// /// let (collector, handle) = collector::mock() - /// .new_span(span1.clone()) - /// .new_span(span2.clone()) - /// .enter(span2) - /// .enter(span1) + /// .new_span(&span1) + /// .new_span(&span2) + /// .enter(&span2) + /// .enter(&span1) /// .run_with_handle(); /// /// tracing::collect::with_default(collector, || { @@ -496,8 +589,8 @@ impl ExpectedSpan { /// .with_ancestry(expect::has_contextual_parent("parent_span")); /// /// let (collector, handle) = collector::mock() - /// .new_span(parent_span.clone()) - /// .enter(parent_span) + /// .new_span(&parent_span) + /// .enter(&parent_span) /// .new_span(span) /// .run_with_handle(); /// @@ -542,8 +635,8 @@ impl ExpectedSpan { /// .with_ancestry(expect::has_explicit_parent("parent_span")); /// /// let (collector, handle) = collector::mock() - /// .new_span(parent_span.clone()) - /// .enter(parent_span) + /// .new_span(&parent_span) + /// .enter(&parent_span) /// .new_span(span) /// .run_with_handle(); /// @@ -695,10 +788,13 @@ impl fmt::Display for ExpectedSpan { } } -impl From for NewSpan { - fn from(span: ExpectedSpan) -> Self { +impl From for NewSpan +where + S: Into, +{ + fn from(span: S) -> Self { Self { - span, + span: span.into(), ..Default::default() } } diff --git a/tracing-mock/src/subscriber.rs b/tracing-mock/src/subscriber.rs index 07d8bb39a..16ed3a59f 100644 --- a/tracing-mock/src/subscriber.rs +++ b/tracing-mock/src/subscriber.rs @@ -40,11 +40,11 @@ //! .named("my_span"); //! let (subscriber, handle) = subscriber::mock() //! // Enter a matching span -//! .enter(span.clone()) +//! .enter(&span) //! // Record an event with message "collect parting message" //! .event(expect::event().with_fields(expect::message("say hello"))) //! // Exit a matching span -//! .exit(span) +//! .exit(&span) //! // Expect no further messages to be recorded //! .only() //! // Return the collector and handle @@ -82,11 +82,11 @@ //! .named("my_span"); //! let (subscriber, handle) = subscriber::mock() //! // Enter a matching span -//! .enter(span.clone()) +//! .enter(&span) //! // Record an event with message "collect parting message" //! .event(expect::event().with_fields(expect::message("say hello"))) //! // Exit a matching span -//! .exit(span) +//! .exit(&span) //! // Expect no further messages to be recorded //! .only() //! // Return the collector and handle @@ -153,11 +153,11 @@ use std::{ /// .named("my_span"); /// let (subscriber, handle) = subscriber::mock() /// // Enter a matching span -/// .enter(span.clone()) +/// .enter(&span) /// // Record an event with message "collect parting message" /// .event(expect::event().with_fields(expect::message("say hello"))) /// // Exit a matching span -/// .exit(span) +/// .exit(&span) /// // Expect no further messages to be recorded /// .only() /// // Return the collector and handle @@ -505,8 +505,8 @@ impl MockSubscriberBuilder { /// .at_level(tracing::Level::INFO) /// .named("the span we're testing"); /// let (subscriber, handle) = subscriber::mock() - /// .enter(span.clone()) - /// .exit(span) + /// .enter(&span) + /// .exit(&span) /// .only() /// .run_with_handle(); /// @@ -533,8 +533,8 @@ impl MockSubscriberBuilder { /// .at_level(tracing::Level::INFO) /// .named("the span we're testing"); /// let (subscriber, handle) = subscriber::mock() - /// .enter(span.clone()) - /// .exit(span) + /// .enter(&span) + /// .exit(&span) /// .only() /// .run_with_handle(); /// @@ -553,8 +553,11 @@ impl MockSubscriberBuilder { /// /// [`exit`]: fn@Self::exit /// [`only`]: fn@Self::only - pub fn enter(mut self, span: ExpectedSpan) -> Self { - self.expected.push_back(Expect::Enter(span)); + pub fn enter(mut self, span: S) -> Self + where + S: Into, + { + self.expected.push_back(Expect::Enter(span.into())); self } @@ -582,8 +585,8 @@ impl MockSubscriberBuilder { /// .at_level(tracing::Level::INFO) /// .named("the span we're testing"); /// let (subscriber, handle) = subscriber::mock() - /// .enter(span.clone()) - /// .exit(span) + /// .enter(&span) + /// .exit(&span) /// .only() /// .run_with_handle(); /// @@ -609,8 +612,8 @@ impl MockSubscriberBuilder { /// .at_level(tracing::Level::INFO) /// .named("the span we're testing"); /// let (subscriber, handle) = subscriber::mock() - /// .enter(span.clone()) - /// .exit(span) + /// .enter(&span) + /// .exit(&span) /// .only() /// .run_with_handle(); /// @@ -630,8 +633,11 @@ impl MockSubscriberBuilder { /// [`enter`]: fn@Self::enter /// [`MockHandle::assert_finished`]: fn@crate::collector::MockHandle::assert_finished /// [`Span::enter`]: fn@tracing::Span::enter - pub fn exit(mut self, span: ExpectedSpan) -> Self { - self.expected.push_back(Expect::Exit(span)); + pub fn exit(mut self, span: S) -> Self + where + S: Into, + { + self.expected.push_back(Expect::Exit(span.into())); self }