scalameta / metals-feature-requests

Issue tracker for Metals feature requests
37 stars 4 forks source link

Use JVM opts in test called by code lens #239

Open GuillaumeDesforges opened 2 years ago

GuillaumeDesforges commented 2 years ago

I'd like to launch tests using the code lens given by metal, however I need options set in .jvmopts:

--add-exports=java.base/sun.nio.ch=ALL-UNNAMED

This applies when I run sbt test and test pass, but not when I use Metal's code lens. How can I fix that?

tgodzik commented 2 years ago

Thanks for reporting! It's not possible to do it currently, best to open an issue here https://github.com/scalameta/metals-feature-requests/issues

We can also just move the issue there if you prefer.

GuillaumeDesforges commented 2 years ago

Okay thanks, please move this issue if possible :)

tgodzik commented 2 years ago

First action would be to add an ability for BSP to specify jvm options for running test classes

olafurpg commented 2 years ago

Is this a Bloop issue? Does sbt BSP have the same problem?

adpi2 commented 2 years ago

Bloop and sbt have the same problem. To run the tests, the current implementation in Bloop and sbt only accepts a list of test suites, as List[String], in the data field of the DebugRequest.

However I don't think we need to add anything to BSP. We already have ScalaTestParams which contains a jvmOptions field. We can pass it in the data field of the DebugRequest.

So we just need to adapt the implementation in Bloop, sbt and Metals.

olafurpg commented 2 years ago

Does sbt use those JVM options when running sbt test? If so, then I would expect the flags to be picked up by default when running tests via Metals together with sbt BSP server. This should be an implementation detail of the build server and Metals shouldn't need to guess that a .jvmoptions file should be forwarded to the jvmOptions field of BSP.

adpi2 commented 2 years ago

Agreed that the .jvmOptions file is an implementation detail of the build server and that Metals should not forward them.

However I think that jvmOptions should be transmitted by the build server to Metals in the buildTarget/scalaTestClasses request. It already works like that in the buildTarget/scalaMainClasses request, whose response contains:

export interface ScalaMainClassesItem {
  /** The build target that contains the test classes. */
  target: BuildTargetIdentifier;

  /** The main class item. */
  classes: ScalaMainClass[];
}

export interface ScalaMainClass {
  /** The main class to run. */
  class: String;

  /** The user arguments to the main entrypoint. */
  arguments: String[];

  /** The jvm options for the application. */
  jvmOptions: String[];

  /** The environment variables for the application. */
  environmentVariables?: String[];
}

Unfortunately ScalaTestClassesItem is:

export interface ScalaTestClassesItem {
  /** The build target that contains the test classes. */
  target: BuildTargetIdentifier;

  /** The fully qualified names of the test classes in this target */
  classes: String[];
}

And I think it should be:

export interface ScalaTestClassesItem {
  /** The build target that contains the test classes. */
  target: BuildTargetIdentifier;

  /** The jvm options for the application. */
  jvmOptions: String[];

  /** The environment variables for the application. */
  environmentVariables?: String[];

  /** The fully qualified names of the test classes in this target */
  classes: String[];
}

Assuming Metals receives those jvmOptions should it systematically forward them back? Symetrically, when server receives some jvmOptions field in the debugSession/start request, what should it do?

  1. Add them to the pre-existing one. (That means Metals does not need to forward everything)
  2. Override the pre-existing one. (That means Metals should forward them most of the time)

Option 1 is easier to use in Metals but it won't be possible to override the existing options. And option 2 is a bit cumbersome but more powerful.

I am slightly more in favor of 1 because in any case the user can change the jvmOptions in the build definition.

GuillaumeDesforges commented 2 years ago

Until this feature is implemented, do you think there a workaround?

adpi2 commented 2 years ago

Until this feature is implemented, do you think there a workaround?

Do you use sbt or Bloop as the build server? If sbt you can try to set Test / fork := true in your project.

GuillaumeDesforges commented 2 years ago

Thanks, will try asap (blocked by https://github.com/scalameta/metals-vscode/issues/758 atm...)

GuillaumeDesforges commented 2 years ago

Test / fork := true did not solve my issue, the same error still appears.

tonyrusignuolo commented 4 months ago

Has this been solved? I'm still seeing this issue and can't seem to get my Scala tests to respect the JVM args for add-opens