Feature: eng 483 trim and fix the tests in shuttle-service (#693)

* tests: update build_crate tests

* tests: remove loader tests

* feat: cleanup service deps

* feat: setup integration tests in runtime

* feat: expected panic message for not_shuttle

* refactor: simplify dummy provisioner

* feat: re-export main and service from runtime

runtime/Cargo.toml

@@ -52,6 +52,11 @@ workspace = true
 workspace = true
 features = ["builder", "web-rocket"] # TODO: remove web-rocket
 
+[dev-dependencies]
+crossbeam-channel = "0.5.6"
+portpicker = "0.1.1"
+futures = { version = "0.3.25" }
+
 [features]
 next = [
     "cap-std", 

runtime/src/bin/rocket.rs

@@ -6,7 +6,7 @@ async fn main() {
 
 async fn loader<S: shuttle_common::storage_manager::StorageManager>(
     mut factory: shuttle_runtime::ProvisionerFactory<S>,
-    logger: shuttle_runtime::Logger,
+    _logger: shuttle_runtime::Logger,
 ) -> shuttle_service::ShuttleRocket {
     use shuttle_service::ResourceBuilder;
 

runtime/src/legacy/mod.rs

@@ -231,7 +231,7 @@ where
         let kill_tx = self.kill_tx.lock().unwrap().deref_mut().take();
 
         if let Some(kill_tx) = kill_tx {
-            if kill_tx.send(format!("stopping deployment")).is_err() {
+            if kill_tx.send("stopping deployment".to_owned()).is_err() {
                 error!("the receiver dropped");
                 return Err(Status::internal("failed to stop deployment"));
             }

runtime/src/lib.rs

@@ -10,3 +10,4 @@ pub use logger::Logger;
 pub use next::{AxumWasm, NextArgs};
 pub use provisioner_factory::ProvisionerFactory;
 pub use shuttle_common::storage_manager::StorageManager;
+pub use shuttle_service::{main, Error, Service};

runtime/src/next/mod.rs

@@ -155,7 +155,7 @@ impl Runtime for AxumWasm {
         let kill_tx = self.kill_tx.lock().unwrap().deref_mut().take();
 
         if let Some(kill_tx) = kill_tx {
-            if kill_tx.send(format!("stopping deployment")).is_err() {
+            if kill_tx.send("stopping deployment".to_owned()).is_err() {
                 error!("the receiver dropped");
                 return Err(Status::internal("failed to stop deployment"));
             }

runtime/tests/integration/helpers.rs

@@ -0,0 +1,95 @@
+use std::{
+    collections::HashMap,
+    net::{Ipv4Addr, SocketAddr},
+    path::{Path, PathBuf},
+};
+
+use anyhow::Result;
+use async_trait::async_trait;
+use shuttle_proto::{
+    provisioner::{
+        provisioner_server::{Provisioner, ProvisionerServer},
+        DatabaseRequest, DatabaseResponse,
+    },
+    runtime::{self, runtime_client::RuntimeClient},
+};
+use shuttle_service::builder::{build_crate, Runtime};
+use tonic::{
+    transport::{Channel, Server},
+    Request, Response, Status,
+};
+
+pub struct TestRuntime {
+    pub runtime_client: RuntimeClient<Channel>,
+    pub bin_path: String,
+    pub service_name: String,
+    pub runtime_address: SocketAddr,
+    pub secrets: HashMap<String, String>,
+}
+
+pub async fn spawn_runtime(project_path: String, service_name: &str) -> Result<TestRuntime> {
+    let provisioner_address = SocketAddr::new(
+        Ipv4Addr::LOCALHOST.into(),
+        portpicker::pick_unused_port().unwrap(),
+    );
+    let runtime_port = portpicker::pick_unused_port().unwrap();
+    let runtime_address = SocketAddr::new(Ipv4Addr::LOCALHOST.into(), runtime_port);
+
+    let (tx, _) = crossbeam_channel::unbounded();
+    let runtime = build_crate(Path::new(&project_path), false, tx).await?;
+
+    let secrets: HashMap<String, String> = Default::default();
+
+    let (is_wasm, bin_path) = match runtime {
+        Runtime::Next(path) => (true, path),
+        Runtime::Legacy(path) => (false, path),
+    };
+
+    start_provisioner(DummyProvisioner, provisioner_address);
+
+    // TODO: update this to work with shuttle-next projects, see cargo-shuttle local run
+    let runtime_path = || bin_path.clone();
+
+    let (_, runtime_client) = runtime::start(
+        is_wasm,
+        runtime::StorageManagerType::WorkingDir(PathBuf::from(project_path.clone())),
+        &format!("http://{}", provisioner_address),
+        runtime_port,
+        runtime_path,
+    )
+    .await?;
+
+    Ok(TestRuntime {
+        runtime_client,
+        bin_path: bin_path
+            .into_os_string()
+            .into_string()
+            .expect("to convert path to string"),
+        service_name: service_name.to_string(),
+        runtime_address,
+        secrets,
+    })
+}
+
+/// A dummy provisioner for tests, a provisioner connection is required
+/// to start a project runtime.
+pub struct DummyProvisioner;
+
+fn start_provisioner(provisioner: DummyProvisioner, address: SocketAddr) {
+    tokio::spawn(async move {
+        Server::builder()
+            .add_service(ProvisionerServer::new(provisioner))
+            .serve(address)
+            .await
+    });
+}
+
+#[async_trait]
+impl Provisioner for DummyProvisioner {
+    async fn provision_database(
+        &self,
+        _request: Request<DatabaseRequest>,
+    ) -> Result<Response<DatabaseResponse>, Status> {
+        panic!("did not expect any runtime test to use dbs")
+    }
+}

runtime/tests/integration/loader.rs

@@ -0,0 +1,48 @@
+use std::time::Duration;
+
+use shuttle_proto::runtime::{LoadRequest, StartRequest};
+use uuid::Uuid;
+
+use crate::helpers::{spawn_runtime, TestRuntime};
+
+/// This test does panic, but the panic happens in a spawned task inside the project runtime,
+/// so we get this output: `thread 'tokio-runtime-worker' panicked at 'panic in bind', src/main.rs:6:9`,
+/// but `should_panic(expected = "panic in bind")` doesn't catch it.
+#[tokio::test]
+#[should_panic(expected = "panic in bind")]
+async fn bind_panic() {
+    let project_path = format!("{}/tests/resources/bind-panic", env!("CARGO_MANIFEST_DIR"));
+
+    let TestRuntime {
+        bin_path,
+        service_name,
+        secrets,
+        mut runtime_client,
+        runtime_address,
+    } = spawn_runtime(project_path, "bind-panic").await.unwrap();
+
+    let load_request = tonic::Request::new(LoadRequest {
+        path: bin_path,
+        service_name,
+        secrets,
+    });
+
+    let _ = runtime_client.load(load_request).await.unwrap();
+
+    let start_request = StartRequest {
+        deployment_id: Uuid::default().as_bytes().to_vec(),
+        ip: runtime_address.to_string(),
+    };
+
+    // I also tried this without spawning, but it gave the same result. Panic but it isn't caught.
+    tokio::spawn(async move {
+        runtime_client
+            .start(tonic::Request::new(start_request))
+            .await
+            .unwrap();
+        // Give it a second to panic.
+        tokio::time::sleep(Duration::from_secs(1)).await;
+    })
+    .await
+    .unwrap();
+}

runtime/tests/integration/main.rs

@@ -0,0 +1,2 @@
+pub mod helpers;
+pub mod loader;

service/tests/resources/bind-panic/Cargo.toml → runtime/tests/resources/bind-panic/Cargo.toml

@@ -3,10 +3,9 @@ name = "bind-panic"
 version = "0.1.0"
 edition = "2021"
 
-[lib]
-crate-type = ["cdylib"]
 
 [workspace]
 
 [dependencies]
-shuttle-service = { path = "../../../" }
+shuttle-runtime = { path = "../../../" }
+tokio = { version = "1.22.0" }

runtime/tests/resources/bind-panic/src/main.rs

@@ -0,0 +1,13 @@
+struct MyService;
+
+#[shuttle_runtime::async_trait]
+impl shuttle_runtime::Service for MyService {
+    async fn bind(mut self, _: std::net::SocketAddr) -> Result<(), shuttle_runtime::Error> {
+        panic!("panic in bind");
+    }
+}
+
+#[shuttle_runtime::main]
+async fn bind_panic() -> Result<MyService, shuttle_runtime::Error> {
+    Ok(MyService)
+}

service/Cargo.toml

@@ -19,7 +19,6 @@ bincode = { version = "1.3.3", optional = true }
 cargo = { version = "0.65.0", optional = true }
 cargo_metadata = "0.15.2"
 crossbeam-channel = "0.5.6"
-futures = { version = "0.3.25", features = ["std"] }
 hyper = { version = "0.14.23", features = ["server", "tcp", "http1"], optional = true }
 num_cpus = { version = "1.14.0", optional = true }
 pipe = "0.4.0"
@@ -36,7 +35,6 @@ tokio = { version = "1.22.0", features = ["sync"] }
 tower = { version = "0.4.13", features = ["make"], optional = true }
 tracing = { workspace = true }
 tracing-subscriber = { workspace = true, features = ["env-filter"] }
-uuid = { workspace = true, features = ["v4"] }
 warp = { version = "0.3.3", optional = true }
 
 # Tide does not have tokio support. So make sure async-std is compatible with tokio
@@ -55,10 +53,7 @@ workspace = true
 features = ["tracing", "service"]
 
 [dev-dependencies]
-portpicker = "0.1.1"
-sqlx = { version = "0.6.2", features = ["runtime-tokio-native-tls", "postgres"] }
 tokio = { version = "1.22.0", features = ["macros", "rt"] }
-uuid = { workspace = true, features = ["v4"] }
 
 [features]
 default = ["codegen"]

service/src/builder.rs

@@ -1,8 +1,8 @@
 use std::path::{Path, PathBuf};
 
-use anyhow::{anyhow, Context};
+use anyhow::{anyhow, bail, Context};
 use cargo::core::compiler::{CompileKind, CompileMode, CompileTarget, MessageFormat};
-use cargo::core::{Shell, Summary, Verbosity, Workspace};
+use cargo::core::{Manifest, Shell, Summary, Verbosity, Workspace};
 use cargo::ops::{clean, compile, CleanOptions, CompileOptions};
 use cargo::util::interning::InternedString;
 use cargo::util::{homedir, ToSemver};
@@ -51,14 +51,18 @@ pub async fn build_crate(
     let manifest_path = project_path.join("Cargo.toml");
     let mut ws = Workspace::new(&manifest_path, &config)?;
 
-    let current = ws.current_mut().map_err(|_| anyhow!("A Shuttle project cannot have a virtual manifest file - please ensure your Cargo.toml file specifies it as a library."))?;
+    let current = ws.current_mut().map_err(|_| anyhow!("A Shuttle project cannot have a virtual manifest file - please ensure the `package` table is present in your Cargo.toml file."))?;
 
     let summary = current.manifest_mut().summary_mut();
-
     let is_next = is_next(summary);
+
     if !is_next {
         check_version(summary)?;
+        ensure_binary(current.manifest())?;
+    } else {
+        ensure_cdylib(current.manifest_mut())?;
     }
+
     check_no_panic(&ws)?;
 
     let opts = get_compile_options(&config, release_mode, is_next)?;
@@ -173,6 +177,37 @@ fn is_next(summary: &Summary) -> bool {
         .any(|dependency| dependency.package_name() == NEXT_NAME)
 }
 
+/// Make sure the project is a binary for legacy projects.
+fn ensure_binary(manifest: &Manifest) -> anyhow::Result<()> {
+    if manifest.targets().iter().any(|target| target.is_bin()) {
+        Ok(())
+    } else {
+        bail!("Your Shuttle project must be a binary.")
+    }
+}
+
+/// Make sure "cdylib" is set for shuttle-next projects, else set it if possible.
+fn ensure_cdylib(manifest: &mut Manifest) -> anyhow::Result<()> {
+    if let Some(target) = manifest
+        .targets_mut()
+        .iter_mut()
+        .find(|target| target.is_lib())
+    {
+        if !target.is_cdylib() {
+            *target = cargo::core::manifest::Target::lib_target(
+                target.name(),
+                vec![cargo::core::compiler::CrateType::Cdylib],
+                target.src_path().path().unwrap().to_path_buf(),
+                target.edition(),
+            );
+        }
+
+        Ok(())
+    } else {
+        bail!("Your Shuttle project must be a library. Please add `[lib]` to your Cargo.toml file.")
+    }
+}
+
 /// Check that the crate being build is compatible with this version of loader
 fn check_version(summary: &Summary) -> anyhow::Result<()> {
     let valid_version = VERSION.to_semver().unwrap();