debugger beta: Fix gdb/delve JSON data conversion from New Session Modal (#31501)

Anthony Eid created

test that check's that each conversion works properly based on the
adapter's config validation function. 

Co-authored-by: Zed AI \<ai@zed.dev\>

Release Notes:

- debugger beta: Fix bug where Go/GDB configuration's wouldn't work from
NewSessionModal

Change summary

crates/dap_adapters/src/gdb.rs                    |   2 
crates/dap_adapters/src/go.rs                     |   4 
crates/dap_adapters/src/php.rs                    |  16 
crates/debugger_ui/src/tests.rs                   |   1 
crates/debugger_ui/src/tests/new_session_modal.rs | 155 +++++++++++++---
5 files changed, 134 insertions(+), 44 deletions(-)

Detailed changes

crates/dap_adapters/src/gdb.rs 🔗

@@ -26,10 +26,12 @@ impl DebugAdapter for GdbDebugAdapter {
 
         match &zed_scenario.request {
             dap::DebugRequest::Attach(attach) => {
+                obj.insert("request".into(), "attach".into());
                 obj.insert("pid".into(), attach.process_id.into());
             }
 
             dap::DebugRequest::Launch(launch) => {
+                obj.insert("request".into(), "launch".into());
                 obj.insert("program".into(), launch.program.clone().into());
 
                 if !launch.args.is_empty() {

crates/dap_adapters/src/go.rs 🔗

@@ -307,10 +307,14 @@ impl DebugAdapter for GoDebugAdapter {
         let mut args = match &zed_scenario.request {
             dap::DebugRequest::Attach(attach_config) => {
                 json!({
+                    "request": "attach",
+                    "mode": "debug",
                     "processId": attach_config.process_id,
                 })
             }
             dap::DebugRequest::Launch(launch_config) => json!({
+                "request": "launch",
+                "mode": "debug",
                 "program": launch_config.program,
                 "cwd": launch_config.cwd,
                 "args": launch_config.args,

crates/dap_adapters/src/php.rs 🔗

@@ -47,13 +47,6 @@ impl PhpDebugAdapter {
         })
     }
 
-    fn validate_config(
-        &self,
-        _: &serde_json::Value,
-    ) -> Result<StartDebuggingRequestArgumentsRequest> {
-        Ok(StartDebuggingRequestArgumentsRequest::Launch)
-    }
-
     async fn get_installed_binary(
         &self,
         delegate: &Arc<dyn DapDelegate>,
@@ -101,7 +94,7 @@ impl PhpDebugAdapter {
             envs: HashMap::default(),
             request_args: StartDebuggingRequestArguments {
                 configuration: task_definition.config.clone(),
-                request: self.validate_config(&task_definition.config)?,
+                request: <Self as DebugAdapter>::validate_config(self, &task_definition.config)?,
             },
         })
     }
@@ -303,6 +296,13 @@ impl DebugAdapter for PhpDebugAdapter {
         Some(SharedString::new_static("PHP").into())
     }
 
+    fn validate_config(
+        &self,
+        _: &serde_json::Value,
+    ) -> Result<StartDebuggingRequestArgumentsRequest> {
+        Ok(StartDebuggingRequestArgumentsRequest::Launch)
+    }
+
     fn config_from_zed_format(&self, zed_scenario: ZedDebugConfig) -> Result<DebugScenario> {
         let obj = match &zed_scenario.request {
             dap::DebugRequest::Attach(_) => {

crates/debugger_ui/src/tests.rs 🔗

@@ -25,7 +25,6 @@ mod inline_values;
 #[cfg(test)]
 mod module_list;
 #[cfg(test)]
-#[cfg(not(windows))]
 mod new_session_modal;
 #[cfg(test)]
 mod persistence;

crates/debugger_ui/src/tests/new_session_modal.rs 🔗

@@ -1,14 +1,14 @@
+use dap::DapRegistry;
 use gpui::{BackgroundExecutor, TestAppContext, VisualTestContext};
 use project::{FakeFs, Project};
 use serde_json::json;
 use std::sync::Arc;
 use std::sync::atomic::{AtomicBool, Ordering};
-use task::{DebugScenario, TaskContext, VariableName};
+use task::{DebugRequest, DebugScenario, LaunchRequest, TaskContext, VariableName, ZedDebugConfig};
 use util::path;
 
 use crate::tests::{init_test, init_test_workspace};
 
-// todo(tasks) figure out why task replacement is broken on windows
 #[gpui::test]
 async fn test_debug_session_substitutes_variables_and_relativizes_paths(
     executor: BackgroundExecutor,
@@ -29,10 +29,9 @@ async fn test_debug_session_substitutes_variables_and_relativizes_paths(
     let workspace = init_test_workspace(&project, cx).await;
     let cx = &mut VisualTestContext::from_window(*workspace, cx);
 
-    // Set up task variables to simulate a real environment
     let test_variables = vec![(
         VariableName::WorktreeRoot,
-        "/test/worktree/path".to_string(),
+        path!("/test/worktree/path").to_string(),
     )]
     .into_iter()
     .collect();
@@ -45,33 +44,35 @@ async fn test_debug_session_substitutes_variables_and_relativizes_paths(
 
     let home_dir = paths::home_dir();
 
-    let sep = std::path::MAIN_SEPARATOR;
-
-    // Test cases for different path formats
-    let test_cases: Vec<(Arc<String>, Arc<String>)> = vec![
+    let test_cases: Vec<(&'static str, &'static str)> = vec![
         // Absolute path - should not be relativized
         (
-            Arc::from(format!("{0}absolute{0}path{0}to{0}program", sep)),
-            Arc::from(format!("{0}absolute{0}path{0}to{0}program", sep)),
+            path!("/absolute/path/to/program"),
+            path!("/absolute/path/to/program"),
         ),
         // Relative path - should be prefixed with worktree root
         (
-            Arc::from(format!(".{0}src{0}program", sep)),
-            Arc::from(format!("{0}test{0}worktree{0}path{0}src{0}program", sep)),
+            format!(".{0}src{0}program", std::path::MAIN_SEPARATOR).leak(),
+            path!("/test/worktree/path/src/program"),
         ),
-        // Home directory path - should be prefixed with worktree root
+        // Home directory path - should be expanded to full home directory path
         (
-            Arc::from(format!("~{0}src{0}program", sep)),
-            Arc::from(format!(
-                "{1}{0}src{0}program",
-                sep,
-                home_dir.to_string_lossy()
-            )),
+            format!("~{0}src{0}program", std::path::MAIN_SEPARATOR).leak(),
+            home_dir
+                .join("src")
+                .join("program")
+                .to_string_lossy()
+                .to_string()
+                .leak(),
         ),
         // Path with $ZED_WORKTREE_ROOT - should be substituted without double appending
         (
-            Arc::from(format!("$ZED_WORKTREE_ROOT{0}src{0}program", sep)),
-            Arc::from(format!("{0}test{0}worktree{0}path{0}src{0}program", sep)),
+            format!(
+                "$ZED_WORKTREE_ROOT{0}src{0}program",
+                std::path::MAIN_SEPARATOR
+            )
+            .leak(),
+            path!("/test/worktree/path/src/program"),
         ),
     ];
 
@@ -80,44 +81,38 @@ async fn test_debug_session_substitutes_variables_and_relativizes_paths(
     for (input_path, expected_path) in test_cases {
         let _subscription = project::debugger::test::intercept_debug_sessions(cx, {
             let called_launch = called_launch.clone();
-            let input_path = input_path.clone();
-            let expected_path = expected_path.clone();
             move |client| {
                 client.on_request::<dap::requests::Launch, _>({
                     let called_launch = called_launch.clone();
-                    let input_path = input_path.clone();
-                    let expected_path = expected_path.clone();
 
                     move |_, args| {
                         let config = args.raw.as_object().unwrap();
 
-                        // Verify the program path was substituted correctly
                         assert_eq!(
                             config["program"].as_str().unwrap(),
-                            expected_path.as_str(),
+                            expected_path,
                             "Program path was not correctly substituted for input: {}",
-                            input_path.as_str()
+                            input_path
                         );
 
-                        // Verify the cwd path was substituted correctly
                         assert_eq!(
                             config["cwd"].as_str().unwrap(),
-                            expected_path.as_str(),
+                            expected_path,
                             "CWD path was not correctly substituted for input: {}",
-                            input_path.as_str()
+                            input_path
                         );
 
-                        // Verify that otherField was substituted but not relativized
-                        // It should still have $ZED_WORKTREE_ROOT substituted if present
                         let expected_other_field = if input_path.contains("$ZED_WORKTREE_ROOT") {
-                            input_path.replace("$ZED_WORKTREE_ROOT", "/test/worktree/path")
+                            input_path
+                                .replace("$ZED_WORKTREE_ROOT", &path!("/test/worktree/path"))
+                                .to_owned()
                         } else {
                             input_path.to_string()
                         };
 
                         assert_eq!(
                             config["otherField"].as_str().unwrap(),
-                            expected_other_field,
+                            &expected_other_field,
                             "Other field was incorrectly modified for input: {}",
                             input_path
                         );
@@ -155,3 +150,93 @@ async fn test_debug_session_substitutes_variables_and_relativizes_paths(
         called_launch.store(false, Ordering::SeqCst);
     }
 }
+
+#[gpui::test]
+async fn test_dap_adapter_config_conversion_and_validation(cx: &mut TestAppContext) {
+    init_test(cx);
+
+    let mut expected_adapters = vec![
+        "CodeLLDB",
+        "Debugpy",
+        "PHP",
+        "JavaScript",
+        "Ruby",
+        "Delve",
+        "GDB",
+        "fake-adapter",
+    ];
+
+    let adapter_names = cx.update(|cx| {
+        let registry = DapRegistry::global(cx);
+        registry.enumerate_adapters()
+    });
+
+    let zed_config = ZedDebugConfig {
+        label: "test_debug_session".into(),
+        adapter: "test_adapter".into(),
+        request: DebugRequest::Launch(LaunchRequest {
+            program: "test_program".into(),
+            cwd: None,
+            args: vec![],
+            env: Default::default(),
+        }),
+        stop_on_entry: Some(true),
+    };
+
+    for adapter_name in adapter_names {
+        let adapter_str = adapter_name.to_string();
+        if let Some(pos) = expected_adapters.iter().position(|&x| x == adapter_str) {
+            expected_adapters.remove(pos);
+        }
+
+        let adapter = cx
+            .update(|cx| {
+                let registry = DapRegistry::global(cx);
+                registry.adapter(adapter_name.as_ref())
+            })
+            .unwrap_or_else(|| panic!("Adapter {} should exist", adapter_name));
+
+        let mut adapter_specific_config = zed_config.clone();
+        adapter_specific_config.adapter = adapter_name.to_string().into();
+
+        let debug_scenario = adapter
+            .config_from_zed_format(adapter_specific_config)
+            .unwrap_or_else(|_| {
+                panic!(
+                    "Adapter {} should successfully convert from Zed format",
+                    adapter_name
+                )
+            });
+
+        assert!(
+            debug_scenario.config.is_object(),
+            "Adapter {} should produce a JSON object for config",
+            adapter_name
+        );
+
+        let request_type = adapter
+            .validate_config(&debug_scenario.config)
+            .unwrap_or_else(|_| {
+                panic!(
+                    "Adapter {} should validate the config successfully",
+                    adapter_name
+                )
+            });
+
+        match request_type {
+            dap::StartDebuggingRequestArgumentsRequest::Launch => {}
+            dap::StartDebuggingRequestArgumentsRequest::Attach => {
+                panic!(
+                    "Expected Launch request but got Attach for adapter {}",
+                    adapter_name
+                );
+            }
+        }
+    }
+
+    assert!(
+        expected_adapters.is_empty(),
+        "The following expected adapters were not found in the registry: {:?}",
+        expected_adapters
+    );
+}