Repository: denoland/deno
Pull request: feat(lsp): display JsErrors at their original line

<!--
Before submitting a PR, please read https://docs.deno.com/runtime/manual/references/contributing

1. Give the PR a descriptive title.

  Examples of good title:
    - fix(std/http): Fix race condition in server
    - docs(console): Update docstrings
    - feat(doc): Handle nested reexports

  Examples of bad title:
    - fix #7123
    - update docs
    - fix bugs

2. Ensure there is a related issue and it is referenced in the PR text.
3. Ensure there are tests that cover the changes.
4. Ensure `cargo test` passes.
5. Ensure `./tools/format.js` passes without changing files.
6. Ensure `./tools/lint.js` passes.
7. Open as a draft PR if your work is still in progress. The CI won't run
   all steps, but you can add '[ci]' to a commit message to force it to.
8. If you would like to run the benchmarks on the CI, add the 'ci-bench' label.
-->

fix https://github.com/denoland/vscode_deno/issues/1297

This displays the LSP errors in the correct line instead of displaying them in the `Deno.test` line. This gives us a much nicer user experience when a test has multiple assertions.

![image](https://github.com/user-attachments/assets/327c5fea-2bee-43e2-936f-46917cd31476)

I originally wanted to hook up the `expected` and `actual` reporters of the LSP as well, but decided against it, since that functionality can also be mimiced by having a smarter `assertEquals` function in the Deno standard library https://github.com/denoland/std/issues/6645


Changed files:
filename: cli/lsp/testing/execution.rs
patch: @@ -42,6 +42,7 @@ use crate::lsp::urls::url_to_uri;
 use crate::tools::test;
 use crate::tools::test::create_test_event_channel;
 use crate::tools::test::FailFastTracker;
+use crate::tools::test::TestFailure;
 use crate::tools::test::TestFailureFormatOptions;
 
 /// Logic to convert a test request into a set of test modules to be tested and
@@ -102,24 +103,32 @@ fn as_queue_and_filters(
   (queue, filters)
 }
 
-fn as_test_messages<S: AsRef<str>>(
-  message: S,
-  is_markdown: bool,
-) -> Vec<lsp_custom::TestMessage> {
+fn failure_to_test_message(failure: &TestFailure) -> lsp_custom::TestMessage {
   let message = lsp::MarkupContent {
-    kind: if is_markdown {
-      lsp::MarkupKind::Markdown
-    } else {
-      lsp::MarkupKind::PlainText
-    },
-    value: message.as_ref().to_string(),
+    kind: lsp::MarkupKind::PlainText,
+    value: failure
+      .format(&TestFailureFormatOptions::default())
+      .to_string(),
   };
-  vec![lsp_custom::TestMessage {
+  let location = failure.error_location().and_then(|v| {
+    let pos = lsp::Position {
+      line: v.line_number,
+      character: v.column_number,
+    };
+    // Does not have to match the test URI
+    // since one can write `Deno.test(importedFunction)`
+    let uri = uri_parse_unencoded(&v.file_name).ok()?;
+    Some(lsp::Location {
+      uri,
+      range: lsp::Range::new(pos, pos),
+    })
+  });
+  lsp_custom::TestMessage {
     message,
     expected_output: None,
     actual_output: None,
-    location: None,
-  }]
+    location,
+  }
 }
 
 #[derive(Debug, Clone, Default, PartialEq)]
@@ -669,10 +678,7 @@ impl LspTestReporter {
         let desc = self.tests.get(&desc.id).unwrap();
         self.progress(lsp_custom::TestRunProgressMessage::Failed {
           test: desc.as_test_identifier(&self.tests),
-          messages: as_test_messages(
-            failure.format(&TestFailureFormatOptions::default()),
-            false,
-          ),
+          messages: vec![failure_to_test_message(failure)],
           duration: Some(elapsed as u32),
         })
       }
@@ -694,7 +700,15 @@ impl LspTestReporter {
       origin,
       test::fmt::format_test_error(js_error, &TestFailureFormatOptions::default())
     );
-    let messages = as_test_messages(err_string, false);
+    let messages = vec![lsp_custom::TestMessage {
+      message: lsp::MarkupContent {
+        kind: lsp::MarkupKind::PlainText,
+        value: err_string,
+      },
+      expected_output: None,
+      actual_output: None,
+      location: None,
+    }];
     for desc in self.tests.values().filter(|d| d.origin() == origin) {
       self.progress(lsp_custom::TestRunProgressMessage::Failed {
         test: desc.as_test_identifier(&self.tests),
@@ -769,10 +783,7 @@ impl LspTestReporter {
       test::TestStepResult::Failed(failure) => {
         self.progress(lsp_custom::TestRunProgressMessage::Failed {
           test: desc.as_test_identifier(&self.tests),
-          messages: as_test_messages(
-            failure.format(&TestFailureFormatOptions::default()),
-            false,
-          ),
+          messages: vec![failure_to_test_message(failure)],
           duration: Some(elapsed as u32),
         })
       }

filename: cli/tools/test/mod.rs
patch: @@ -400,6 +400,38 @@ impl TestFailure {
     }
   }
 
+  pub fn error_location(&self) -> Option<TestLocation> {
+    const TEST_RUNNER: &str = "ext:cli/40_test.js";
+    match self {
+      TestFailure::JsError(js_error) => js_error
+        .frames
+        .iter()
+        // The first line of user code comes above the test file.
+        // The call stack usually contains the top 10 frames, and cuts off after that.
+        // We need to explicitly check for the test runner here.
+        // - Checking for a `ext:` is not enough, since other Deno `ext:`s can appear in the call stack.
+        // - This check guarantees that the next frame is inside of the Deno.test(),
+        //   and not somewhere else.
+        .position(|v| v.file_name.as_deref() == Some(TEST_RUNNER))
+        // Go one up in the stack frame, this is where the user code was
+        .and_then(|index| index.checked_sub(1))
+        .and_then(|index| {
+          let user_frame = &js_error.frames[index];
+          let file_name = user_frame.file_name.as_ref()?.to_string();
+          // Turn into zero based indices
+          let line_number = user_frame.line_number.map(|v| v - 1)? as u32;
+          let column_number =
+            user_frame.column_number.map(|v| v - 1).unwrap_or(0) as u32;
+          Some(TestLocation {
+            file_name,
+            line_number,
+            column_number,
+          })
+        }),
+      _ => None,
+    }
+  }
+
   fn format_label(&self) -> String {
     match self {
       TestFailure::Incomplete => colors::gray("INCOMPLETE").to_string(),

filename: tests/integration/lsp_tests.rs
patch: @@ -5,6 +5,7 @@ use std::fs;
 use ntest_timeout::timeout;
 use pretty_assertions::assert_eq;
 use serde::Deserialize;
+use serde::Serialize;
 use serde_json::json;
 use serde_json::Value;
 use test_util::assert_starts_with;
@@ -14152,7 +14153,7 @@ export function B() {
   client.shutdown();
 }
 
-#[derive(Debug, Clone, Deserialize, PartialEq)]
+#[derive(Debug, Clone, Deserialize, Serialize, PartialEq)]
 #[serde(rename_all = "camelCase")]
 struct TestData {
   id: String,
@@ -14450,6 +14451,125 @@ Deno.test({
   client.shutdown();
 }
 
+#[test]
+#[timeout(300_000)]
+fn lsp_testing_api_failure() {
+  let context = TestContextBuilder::new().use_temp_cwd().build();
+  let temp_dir = context.temp_dir();
+  temp_dir.write("./deno.json", json!({}).to_string());
+  let file = temp_dir.source_file(
+    "test.ts",
+    r#"
+      Deno.test("test a", () => {
+        throw new Error("Some message.");
+      });
+    "#,
+  );
+  let mut client = context.new_lsp_command().build();
+  client.initialize_default();
+  client.did_open_file(&file);
+  let notification =
+    client.read_notification_with_method::<Value>("deno/testModule");
+  let params: TestModuleNotificationParams =
+    serde_json::from_value(notification.unwrap()).unwrap();
+  assert_eq!(params.text_document.uri.as_str(), file.uri().as_str());
+  assert_eq!(params.kind, TestModuleNotificationKind::Replace);
+  assert_eq!(params.label, "test.ts");
+  assert_eq!(params.tests.len(), 1);
+  let test = params.tests.into_iter().next().unwrap();
+  assert_eq!(test.label, "test a");
+  assert_eq!(
+    json!(test.range),
+    json!({
+      "start": { "line": 1,  "character": 11 },
+      "end": { "line": 1,  "character": 15 },
+    }),
+  );
+  assert_eq!(json!(&test.steps), json!(null));
+  let res = client.write_request_with_res_as::<TestRunResponseParams>(
+    "deno/testRun",
+    json!({
+      "id": 1,
+      "kind": "run",
+    }),
+  );
+  assert_eq!(res.enqueued.len(), 1);
+  assert_eq!(
+    res.enqueued[0].text_document.uri.as_str(),
+    file.uri().as_str()
+  );
+  assert_eq!(res.enqueued[0].ids.len(), 1);
+  let id = res.enqueued[0].ids[0].clone();
+  let notification =
+    client.read_notification_with_method::<Value>("deno/testRunProgress");
+  assert_eq!(
+    notification,
+    Some(json!({
+      "id": 1,
+      "message": {
+        "type": "started",
+        "test": {
+          "textDocument": { "uri": file.uri() },
+          "id": id,
+        },
+      },
+    })),
+  );
+  let notification =
+    client.read_notification_with_method::<Value>("deno/testRunProgress");
+  let notification = notification.unwrap();
+  let obj = notification.as_object().unwrap();
+  assert_eq!(obj.get("id"), Some(&json!(1)));
+  let message = obj.get("message").unwrap().as_object().unwrap();
+  match message.get("type").and_then(|v| v.as_str()) {
+    Some("failed") => {
+      assert_eq!(
+        message.get("test"),
+        Some(&json!({
+          "textDocument": { "uri": file.uri() },
+          "id": id,
+        })),
+      );
+      assert!(message.contains_key("duration"));
+      assert_eq!(
+        *message.get("messages").unwrap(),
+        json!([
+          {
+            "message": {
+              "kind": "plaintext",
+              "value": format!("Error: Some message.\n        throw new Error(\"Some message.\");\n\u{1b}[0m\u{1b}[31m              ^\u{1b}[0m\n    at \u{1b}[0m\u{1b}[36m{}\u{1b}[0m:\u{1b}[0m\u{1b}[33m3\u{1b}[0m:\u{1b}[0m\u{1b}[33m15\u{1b}[0m", file.url()),
+            },
+            "location": {
+              "uri": file.uri(),
+              "range": {
+                "start": { "line": 2, "character": 14 },
+                "end": { "line": 2, "character": 14 },
+              },
+            },
+          },
+        ]),
+      );
+      let notification =
+        client.read_notification_with_method::<Value>("deno/testRunProgress");
+      assert_eq!(
+        notification,
+        Some(json!({
+          "id": 1,
+          "message": {
+            "type": "end",
+          },
+        })),
+      );
+    }
+    // sometimes on windows, the messages come out of order, but it actually is
+    // working, so if we do get the end before the passed, we will simply let
+    // the test pass
+    Some("end") => (),
+    _ => panic!("unexpected message {}", json!(notification)),
+  }
+  client.shutdown();
+}
+
 #[test]
 #[timeout(300_000)]
 fn lsp_closed_file_find_references() {