remove Buffer from Mock*Service (#1440)

* remove Buffer from MockSubgraphService

* make mock services that can panic

the services based on tower-test were isolated in a task, which means
that any panic due to mockall would stay in that task and not affect the
rest of the tests.

This rewrites the mock service system to provide clonable mock services
that can panic

NEXT_CHANGELOG.md

@@ -63,7 +63,6 @@ when creating it. It will then be modified to integrate the telemetry plugin's l
 
 By [@geal](https://github.com/geal) in https://github.com/apollographql/router/pull/1463
 
-
 ### Reorder query planner execution ([PR #1484](https://github.com/apollographql/router/pull/1484))
 
 Query planning is deterministic, it only depends on the query, operation name and query planning
@@ -75,6 +74,14 @@ they should happen in a supergraph service.
 
 By [@Geal](https://github.com/Geal) in https://github.com/apollographql/router/pull/1464
 
+### 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
 
 ### Expose query plan in extensions for GraphQL response (experimental) ([PR #1470](https://github.com/apollographql/router/pull/1470))

apollo-router/Cargo.toml

@@ -136,7 +136,6 @@ tower-http = { version = "0.3.4", features = [
     "timeout"
 ] }
 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();