Pass the World to after hook even if Step has failed (#207)

CHANGELOG.md

@@ -6,6 +6,22 @@ All user visible changes to `cucumber` crate will be documented in this file. Th
 
 
 
+## [0.13.0] · ???
+[0.13.0]: /../../tree/v0.13.0
+
+[Diff](/../../compare/v0.12.1...v0.13.0)
+
+### BC Breaks
+
+- [`Cucumber::after`][after_hook] now gets the `World` instance even if a `Step` before it has failed, but accepts a shared reference to it instance instead of a mutable one. ([#209], [#207])
+
+[#207]: /../../issues/207
+[#209]: /../../pull/209
+[after_hook]: https://docs.rs/cucumber/latest/cucumber/struct.Cucumber.html#method.after
+
+
+
+
 ## [0.12.1] · 2022-03-09
 [0.12.1]: /../../tree/v0.12.1
 
@@ -27,7 +43,7 @@ All user visible changes to `cucumber` crate will be documented in this file. Th
 
 ### BC Breaks
 
-- `step::Context::matches` now containes regex capturing group names in addition to captured values. ([#204])
+- `step::Context::matches` now contains regex capturing group names in addition to captured values. ([#204])
 
 ### Added
 

Cargo.toml

@@ -85,6 +85,10 @@ harness = false
 name = "wait"
 harness = false
 
+[[test]]
+name = "after_hook"
+harness = false
+
 [workspace]
 members = ["codegen"]
 exclude = ["book/tests"]

src/cucumber.rs

@@ -997,7 +997,7 @@ where
             &'a gherkin::Feature,
             Option<&'a gherkin::Rule>,
             &'a gherkin::Scenario,
-            Option<&'a mut W>,
+            Option<&'a W>,
         ) -> LocalBoxFuture<'a, ()>
         + 'static,
 {
@@ -1091,12 +1091,7 @@ where
     /// [`Step`]s, even after [`Skipped`] of [`Failed`] [`Step`]s.
     ///
     /// Last `World` argument is supplied to the function, in case it was
-    /// initialized before by running [`before`] hook or any non-failed
-    /// [`Step`]. In case the last [`Scenario`]'s [`Step`] failed, we want to
-    /// return event with an exact `World` state. Also, we don't want to impose
-    /// additional [`Clone`] bounds on `World`, so the only option left is to
-    /// pass [`None`] to the function.
-    ///
+    /// initialized before by running [`before`] hook or any [`Step`].
     ///
     /// [`before`]: Self::before()
     /// [`Failed`]: event::Step::Failed
@@ -1113,7 +1108,7 @@ where
                 &'a gherkin::Feature,
                 Option<&'a gherkin::Rule>,
                 &'a gherkin::Scenario,
-                Option<&'a mut W>,
+                Option<&'a W>,
             ) -> LocalBoxFuture<'a, ()>
             + 'static,
     {

src/runner/basic.rs

@@ -102,7 +102,7 @@ pub type AfterHookFn<World> = for<'a> fn(
     &'a gherkin::Feature,
     Option<&'a gherkin::Rule>,
     &'a gherkin::Scenario,
-    Option<&'a mut World>,
+    Option<&'a World>,
 ) -> LocalBoxFuture<'a, ()>;
 
 /// Alias for a failed [`Scenario`].
@@ -308,11 +308,7 @@ impl<World, Which, Before, After> Basic<World, Which, Before, After> {
     /// [`Step`]s, even after [`Skipped`] of [`Failed`] ones.
     ///
     /// Last `World` argument is supplied to the function, in case it was
-    /// initialized before by running [`before`] hook or any non-failed
-    /// [`Step`]. In case the last [`Scenario`]'s [`Step`] failed, we want to
-    /// return event with an exact `World` state. Also, we don't want to impose
-    /// additional [`Clone`] bounds on `World`, so the only option left is to
-    /// pass [`None`] to the function.
+    /// initialized before by running [`before`] hook or any [`Step`].
     ///
     /// [`before`]: Self::before()
     /// [`Failed`]: event::Step::Failed
@@ -326,7 +322,7 @@ impl<World, Which, Before, After> Basic<World, Which, Before, After> {
             &'a gherkin::Feature,
             Option<&'a gherkin::Rule>,
             &'a gherkin::Scenario,
-            Option<&'a mut World>,
+            Option<&'a World>,
         ) -> LocalBoxFuture<'a, ()>,
     {
         let Self {
@@ -405,7 +401,7 @@ where
             &'a gherkin::Feature,
             Option<&'a gherkin::Rule>,
             &'a gherkin::Scenario,
-            Option<&'a mut W>,
+            Option<&'a W>,
         ) -> LocalBoxFuture<'a, ()>
         + 'static,
 {
@@ -534,7 +530,7 @@ async fn execute<W, Before, After>(
             &'a gherkin::Feature,
             Option<&'a gherkin::Rule>,
             &'a gherkin::Scenario,
-            Option<&'a mut W>,
+            Option<&'a W>,
         ) -> LocalBoxFuture<'a, ()>,
 {
     // Those panic hook shenanigans are done to avoid console messages like
@@ -679,7 +675,7 @@ where
             &'a gherkin::Feature,
             Option<&'a gherkin::Rule>,
             &'a gherkin::Scenario,
-            Option<&'a mut W>,
+            Option<&'a W>,
         ) -> LocalBoxFuture<'a, ()>,
 {
     /// Creates a new [`Executor`].
@@ -767,7 +763,7 @@ where
         ));
 
         let mut is_failed = false;
-        let world = async {
+        let world: Option<Arc<W>> = async {
             let before_hook = self
                 .run_before_hook(&feature, rule.as_ref(), &scenario)
                 .await
@@ -812,6 +808,7 @@ where
                     self.run_step(world, step, into_step_ev).map_ok(Some)
                 })
                 .await
+                .map(|world| world.map(Arc::new))
         }
         .inspect_err(|e| {
             if e.is_none() {
@@ -921,11 +918,11 @@ where
     /// - Emits [`HookType::After`] event.
     async fn run_after_hook(
         &self,
-        mut world: Option<W>,
+        world: Option<Arc<W>>,
         feature: &Arc<gherkin::Feature>,
         rule: Option<&Arc<gherkin::Rule>>,
         scenario: &Arc<gherkin::Scenario>,
-    ) -> Result<Option<W>, ()> {
+    ) -> Result<Option<Arc<W>>, ()> {
         if let Some(hook) = self.after_hook.as_ref() {
             self.send_event(event::Cucumber::scenario(
                 Arc::clone(feature),
@@ -939,7 +936,7 @@ where
                     feature.as_ref(),
                     rule.as_ref().map(AsRef::as_ref),
                     scenario.as_ref(),
-                    world.as_mut(),
+                    world.as_ref().map(Arc::as_ref),
                 );
                 match AssertUnwindSafe(fut).catch_unwind().await {
                     Ok(()) => Ok(world),
@@ -965,7 +962,7 @@ where
                         Arc::clone(scenario),
                         event::Scenario::hook_failed(
                             HookType::After,
-                            world.map(Arc::new),
+                            world,
                             info.into(),
                         ),
                     ));
@@ -989,7 +986,7 @@ where
         world: Option<W>,
         step: Arc<gherkin::Step>,
         (started, passed, skipped, failed): (St, Ps, Sk, F),
-    ) -> Result<W, Option<W>>
+    ) -> Result<W, Option<Arc<W>>>
     where
         St: FnOnce(Arc<gherkin::Step>) -> event::Cucumber<W>,
         Ps: FnOnce(Arc<gherkin::Step>, CaptureLocations) -> event::Cucumber<W>,
@@ -1053,16 +1050,12 @@ where
             }
             Ok((_, world)) => {
                 self.send_event(skipped(step));
-                Err(world)
+                Err(world.map(Arc::new))
             }
             Err((err, captures, world)) => {
-                self.send_event(failed(
-                    step,
-                    captures,
-                    world.map(Arc::new),
-                    err,
-                ));
-                Err(None)
+                let world = world.map(Arc::new);
+                self.send_event(failed(step, captures, world.clone(), err));
+                Err(world)
             }
         }
     }

tests/after_hook.rs

@@ -0,0 +1,66 @@
+use std::{
+    convert::Infallible,
+    panic::AssertUnwindSafe,
+    sync::{
+        atomic::{AtomicUsize, Ordering},
+        Arc,
+    },
+    time::Duration,
+};
+
+use async_trait::async_trait;
+use cucumber::{given, then, when, Parameter, WorldInit};
+use derive_more::{Deref, FromStr};
+use futures::FutureExt as _;
+use tokio::time;
+
+#[tokio::main]
+async fn main() {
+    static NUMBER_OF_WORLDS: AtomicUsize = AtomicUsize::new(0);
+
+    let res = World::cucumber()
+        .after(move |_, _, _, w| {
+            async move {
+                if w.is_some() {
+                    NUMBER_OF_WORLDS.fetch_add(1, Ordering::SeqCst);
+                }
+            }
+            .boxed()
+        })
+        .run_and_exit("tests/features/wait");
+
+    let err = AssertUnwindSafe(res)
+        .catch_unwind()
+        .await
+        .expect_err("should err");
+    let err = err.downcast_ref::<String>().unwrap();
+
+    assert_eq!(err, "2 steps failed, 1 parsing error");
+    assert_eq!(NUMBER_OF_WORLDS.load(Ordering::SeqCst), 12);
+}
+
+#[given(regex = r"(\d+) secs?")]
+#[when(regex = r"(\d+) secs?")]
+#[then(expr = "{u64} sec(s)")]
+async fn step(world: &mut World, secs: CustomU64) {
+    time::sleep(Duration::from_secs(*secs)).await;
+
+    world.0 += 1;
+    assert!(world.0 < 4, "Too much!");
+}
+
+#[derive(Deref, FromStr, Parameter)]
+#[param(regex = "\\d+", name = "u64")]
+struct CustomU64(u64);
+
+#[derive(Clone, Copy, Debug, WorldInit)]
+struct World(usize);
+
+#[async_trait(?Send)]
+impl cucumber::World for World {
+    type Error = Infallible;
+
+    async fn new() -> Result<Self, Self::Error> {
+        Ok(World(0))
+    }
+}

tests/json.rs

@@ -11,9 +11,7 @@ use tempfile::NamedTempFile;
 #[then(regex = r"(\d+) secs?")]
 fn step(world: &mut World) {
     world.0 += 1;
-    if world.0 > 3 {
-        panic!("Too much!");
-    }
+    assert!(world.0 < 4, "Too much!");
 }
 
 #[tokio::main]
@@ -23,17 +21,16 @@ async fn main() {
         World::cucumber()
             .before(|_, _, sc, _| {
                 async {
-                    if sc.tags.iter().any(|t| t == "fail_before") {
-                        panic!("Tag!");
-                    }
+                    assert!(
+                        !sc.tags.iter().any(|t| t == "fail_before"),
+                        "Tag!",
+                    );
                 }
                 .boxed_local()
             })
             .after(|_, _, sc, _| {
                 async {
-                    if sc.tags.iter().any(|t| t == "fail_after") {
-                        panic!("Tag!");
-                    }
+                    assert!(!sc.tags.iter().any(|t| t == "fail_after"), "Tag!");
                 }
                 .boxed_local()
             })

tests/wait.rs

@@ -49,9 +49,7 @@ async fn step(world: &mut World, secs: CustomU64) {
     time::sleep(Duration::from_secs(*secs)).await;
 
     world.0 += 1;
-    if world.0 > 3 {
-        panic!("Too much!");
-    }
+    assert!(world.0 < 4, "Too much!");
 }
 
 #[derive(Deref, FromStr, Parameter)]