scalameta / metals

Scala language server with rich IDE features 🚀
https://scalameta.org/metals/
Apache License 2.0
2.08k stars 328 forks source link

Environment Variables are not passed to BSP when debugging a test. #6746

Closed masonedmison closed 3 weeks ago

masonedmison commented 4 weeks ago

Describe the bug

Description

When debugging a test, reading variables from the environment fails. Specifically, I'm "running" a test that reads "FOO" from the environment by executing dap.continue(config) in Neovim where config is

dap.configurations.scala = {
  {
    type = 'scala',
    request = 'launch',
    name = 'RunOrTest',
    metals = {
      runType = 'runOrTestFile',
      env = { FOO = 'BAR' },
    },
  }
}

I have a minimal project here of which I'm able to reproduce the issue: running dap.continue with the above configuration against Main.scala succeeds, against MySuite a "java.util.NoSuchElementException: FOO" exception is thrown.

Expected behavior

I would expect the test in the the minimal example shared above to pass; environment variables declared in my configuration should be passed along to the debug server.

Operating system

macOS

Editor/Extension

Nvim/Vim (other)

Version of Metals

v1.3.5

Extra context or search terms

I've started to poke around a bit to see if I could figure out the issue. I looked at the $WORKSPACE/.metals/bsp.trace.json logs. When "debugging" Main I see the parameters of the outgoing debugSession/start request to be

{
  "targets": [
    {
      "uri": "file:/Users/medmison/programming/scala/sandbox3/?id\u003droot"
    }
  ],
  "dataKind": "scala-main-class",
  "data": {
    "class": "hello",
    "arguments": [],
    "jvmOptions": [],
    "environmentVariables": [
      "FOO\u003dBAR"
    ]
  }
}

When "debugging" MySuite the parameters value is

{
  "targets": [
    {
      "uri": "file:/Users/medmison/programming/scala/sandbox3/?id\u003droot-test"
    }
  ],
  "dataKind": "scala-test-suites",
  "data": [
    "MySuite"
  ]
}

so it appears that metals is not including all arguments to the build server. Taking a look at the metals code, I think this is happening here.

What's not clear to me is that there seems to be a test that tests this and it passes 🤔 (I still need to have a deeper look at some of the test server functionality though).

At any rate, I'd be more than happy to put up a PR for this if you all agree that this is an issue that should be fixed :). The fix seems like it should be relatively trivial but I'm not sure how to test it (again, since there appears to be an existing test that passes).

tgodzik commented 4 weeks ago

Thanks for reporting! It looks like the issue is here https://github.com/scalameta/metals/blob/main/metals/src/main/scala/scala/meta/internal/metals/debug/DebugDiscovery.scala#L278

where we use SCALA_TEST_SUITES instead of SCALA_TEST_SUITES_SELECTION, which has more parameters to define.

An example of how that's being done is here https://github.com/scalameta/metals/blob/main/metals/src/main/scala/scala/meta/internal/metals/debug/DebugProvider.scala#L536

I would be happy to guide you on how to fix this issue. The tests that are passing are actually using a different path, and you should be able to add a check in https://github.com/scalameta/metals/blob/main/tests/unit/src/test/scala/tests/DebugDiscoverySuite.scala#L138

masonedmison commented 4 weeks ago

Hi @tgodzik! I should have a few hours this afternoon to take to start working on this :). Thanks for the hints; I'll reach out if I get stuck!

masonedmison commented 4 weeks ago

Just opened a PR for this. I can't quite say I understand why I need to make the kind SCALA_TEST_SUITES_SELECTION; I just followed your advice above :).