remove Buffer from Mock*Service

NEXT_CHANGELOG.md

@@ -35,6 +35,15 @@ This reduces the number of copies of this string we keep in memory, as schemas c
 
 By [@SimonSapin](https://github.com/SimonSapin)
 
+### Remove Buffer from Mock*Service ([PR #1440](https://github.com/apollographql/router/pull/1440)
+
+This removes the usage of `tower_test::mock::Mock` in mocked services because it isolated the service in a task
+so panics triggered by mockall were not transmitted up to the unit test that should catch it.
+This rewrites the mocked services API to remove the `build()` method, and make them clonable if needed,
+using an `expect_clone` call with mockall.
+
+By [@Geal](https://github.com/Geal) in https://github.com/apollographql/router/pull/1440
+
 ## 🚀 Features
 
 ## 🐛 Fixes

apollo-router/Cargo.toml

@@ -135,7 +135,6 @@ tower-http = { version = "0.3.4", features = [
     "decompression-gzip",
 ] }
 tower-service = "0.3.2"
-tower-test = "0.4.0"
 tracing = "=0.1.34"
 tracing-core = "=0.1.26"
 tracing-futures = { version = "0.2.5", features = ["futures-03"] }

apollo-router/src/layers/async_checkpoint.rs

@@ -153,23 +153,24 @@ mod async_checkpoint_tests {
         let expected_label = "from_mock_service";
 
         let mut execution_service = MockExecutionService::new();
-
-        execution_service
-            .expect_call()
-            .times(1)
-            .returning(move |_req: crate::ExecutionRequest| {
-                Ok(ExecutionResponse::fake_builder()
-                    .label(expected_label.to_string())
-                    .build())
-            });
-
-        let service = execution_service.build();
+        execution_service.expect_clone().return_once(move || {
+            let mut execution_service = MockExecutionService::new();
+            execution_service.expect_call().times(1).returning(
+                move |_req: crate::ExecutionRequest| {
+                    Ok(ExecutionResponse::fake_builder()
+                        .label(expected_label.to_string())
+                        .build())
+                },
+            );
+
+            execution_service
+        });
 
         let service_stack = ServiceBuilder::new()
             .checkpoint_async(|req: crate::ExecutionRequest| async {
                 Ok(ControlFlow::Continue(req))
             })
-            .service(service);
+            .service(execution_service);
 
         let request = ExecutionRequest::fake_builder().build();
 
@@ -191,20 +192,22 @@ mod async_checkpoint_tests {
         let expected_label = "from_mock_service";
         let mut router_service = MockExecutionService::new();
 
-        router_service
-            .expect_call()
-            .times(1)
-            .returning(move |_req| {
-                Ok(ExecutionResponse::fake_builder()
-                    .label(expected_label.to_string())
-                    .build())
-            });
-
-        let service = router_service.build();
+        router_service.expect_clone().return_once(move || {
+            let mut router_service = MockExecutionService::new();
+            router_service
+                .expect_call()
+                .times(1)
+                .returning(move |_req| {
+                    Ok(ExecutionResponse::fake_builder()
+                        .label(expected_label.to_string())
+                        .build())
+                });
+            router_service
+        });
 
         let service_stack =
             AsyncCheckpointLayer::new(|req| async { Ok(ControlFlow::Continue(req)) })
-                .layer(service);
+                .layer(router_service);
 
         let request = ExecutionRequest::fake_builder().build();
 
@@ -224,9 +227,10 @@ mod async_checkpoint_tests {
     #[tokio::test]
     async fn test_return() {
         let expected_label = "returned_before_mock_service";
-        let router_service = MockExecutionService::new();
-
-        let service = router_service.build();
+        let mut router_service = MockExecutionService::new();
+        router_service
+            .expect_clone()
+            .return_once(MockExecutionService::new);
 
         let service_stack = AsyncCheckpointLayer::new(|_req| async {
             Ok(ControlFlow::Break(
@@ -235,7 +239,7 @@ mod async_checkpoint_tests {
                     .build(),
             ))
         })
-        .layer(service);
+        .layer(router_service);
 
         let request = ExecutionRequest::fake_builder().build();
 
@@ -255,15 +259,16 @@ mod async_checkpoint_tests {
     #[tokio::test]
     async fn test_error() {
         let expected_error = "checkpoint_error";
-        let router_service = MockExecutionService::new();
-
-        let service = router_service.build();
+        let mut router_service = MockExecutionService::new();
+        router_service
+            .expect_clone()
+            .return_once(MockExecutionService::new);
 
         let service_stack =
             AsyncCheckpointLayer::new(
                 move |_req| async move { Err(BoxError::from(expected_error)) },
             )
-            .layer(service);
+            .layer(router_service);
 
         let request = ExecutionRequest::fake_builder().build();
 

apollo-router/src/layers/cache.rs

@@ -170,6 +170,7 @@ where
 }
 
 #[cfg(test)]
+#[allow(dead_code, unreachable_pub)]
 mod test {
     use std::time::Duration;
 
@@ -225,6 +226,8 @@ mod test {
             })
         });
 
+        mock_service.expect_clone().return_once(MockABService::new);
+
         let mut service = create_service(mock_service);
 
         let expected = Ok(B {
@@ -255,13 +258,14 @@ mod test {
     }
 
     #[tokio::test]
-    async fn is_should_cache_err() {
+    async fn it_should_cache_err() {
         let mut mock_service = MockABService::new();
 
         mock_service
             .expect_call()
             .times(1)
             .returning(move |a| Err(BoxError::from(format!("{} err", a.key))));
+        mock_service.expect_clone().return_once(MockABService::new);
 
         let mut service = create_service(mock_service);
 
@@ -295,7 +299,7 @@ mod test {
 
         mock_service
             .expect_call()
-            .times(2)
+            .times(1)
             .with(eq(A {
                 key: "Not cacheable".into(),
                 value: "Needed".into(),
@@ -306,6 +310,25 @@ mod test {
                     value: "there".into(),
                 })
             });
+        mock_service.expect_clone().return_once(move || {
+            let mut mock_service = MockABService::new();
+            mock_service
+                .expect_call()
+                .times(1)
+                .with(eq(A {
+                    key: "Not cacheable".into(),
+                    value: "Needed".into(),
+                }))
+                .returning(move |a| {
+                    Ok(B {
+                        key: a.key,
+                        value: "there".into(),
+                    })
+                });
+            mock_service.expect_clone().return_once(MockABService::new);
+
+            mock_service
+        });
 
         let mut service = create_service(mock_service);
 
@@ -324,7 +347,7 @@ mod test {
     }
 
     #[tokio::test]
-    async fn it_should_dedupe_in_flight_calls() {
+    async fn it_should_deduplicate_in_flight_calls() {
         let mut mock_service = MockABService::new();
 
         mock_service
@@ -340,6 +363,7 @@ mod test {
                     value: "there".into(),
                 })
             });
+        mock_service.expect_clone().return_once(MockABService::new);
 
         let mut service = create_service(mock_service);
 
@@ -387,7 +411,7 @@ mod test {
                 },
             ))
             .filter_async(Slow::default())
-            .service(mock_service.build())
+            .service(mock_service)
             .map_err(|e: BoxError| e.to_string())
     }
 }

apollo-router/src/layers/map_future_with_context.rs

@@ -114,7 +114,7 @@ mod test {
                     })
                 },
             )
-            .service(mock_service.build());
+            .service(mock_service);
 
         let result = service
             .ready()

apollo-router/src/layers/sync_checkpoint.rs

@@ -191,11 +191,9 @@ mod checkpoint_tests {
                     .build())
             });
 
-        let service = execution_service.build();
-
         let service_stack = ServiceBuilder::new()
             .checkpoint(|req: crate::ExecutionRequest| Ok(ControlFlow::Continue(req)))
-            .service(service);
+            .service(execution_service);
 
         let request = ExecutionRequest::fake_builder().build();
 
@@ -226,10 +224,8 @@ mod checkpoint_tests {
                     .build())
             });
 
-        let service = router_service.build();
-
         let service_stack =
-            CheckpointLayer::new(|req| Ok(ControlFlow::Continue(req))).layer(service);
+            CheckpointLayer::new(|req| Ok(ControlFlow::Continue(req))).layer(router_service);
 
         let request = ExecutionRequest::fake_builder().build();
 
@@ -251,16 +247,14 @@ mod checkpoint_tests {
         let expected_label = "returned_before_mock_service";
         let router_service = MockExecutionService::new();
 
-        let service = router_service.build();
-
         let service_stack = CheckpointLayer::new(|_req| {
             Ok(ControlFlow::Break(
                 ExecutionResponse::fake_builder()
                     .label("returned_before_mock_service".to_string())
                     .build(),
             ))
         })
-        .layer(service);
+        .layer(router_service);
 
         let request = ExecutionRequest::fake_builder().build();
 
@@ -282,10 +276,8 @@ mod checkpoint_tests {
         let expected_error = "checkpoint_error";
         let router_service = MockExecutionService::new();
 
-        let service = router_service.build();
-
-        let service_stack =
-            CheckpointLayer::new(move |_req| Err(BoxError::from(expected_error))).layer(service);
+        let service_stack = CheckpointLayer::new(move |_req| Err(BoxError::from(expected_error)))
+            .layer(router_service);
 
         let request = ExecutionRequest::fake_builder().build();