Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for running and testing via Debug Adapter Protocol #942

Merged
merged 34 commits into from Oct 4, 2019

Conversation

@marek1840
Copy link
Collaborator

marek1840 commented Sep 24, 2019

Previously there was no way to launch a main class or test suite from within vscode. Now, we provide code lenses for those.
Using this lenses starts a debug adapter within bloop and creates a proxy in metals.

run-and-test

This is a continuation of #923. It turns out github didn't handle a rebase very well

@olafurpg olafurpg changed the title Add dap proxy Add support for running, testing and debugging via Debug Adapter Protocol Sep 25, 2019
import scala.reflect.ClassTag
import scala.reflect.classTag
import scala.util.Try

object JsonParser {
private val gson = new Gson()

implicit class Parsable(json: JsonElement) {

This comment has been minimized.

Copy link
@olafurpg

olafurpg Sep 25, 2019

Member

By convention, implicit classes start with the Xtension prefix. Even if Parsable didn't follow the convention here the new Serialized should. I would call it XtensionParseJson, the more specific the name the less likely it is to conflict with other implicit classes in scope.

)
val expected = ClientCommands.ReloadDoctor.id :: ClientCommands.RunDoctor.id :: Nil
val actual = client.workspaceClientCommands
assert(actual.startsWith(expected))

This comment has been minimized.

Copy link
@olafurpg

olafurpg Sep 25, 2019

Member

What is the motivation for this change? The ScalaTest error message inside asserts for large lists is quite difficult to read. The output format from assertNoDiff is easier to debug in my experience (although it may take some time to get used to the output)

This comment has been minimized.

Copy link
@marek1840

marek1840 Sep 25, 2019

Author Collaborator

because now the client.workspaceClientCommands contain also ClientCommands.RefreshModel, so the noDiff ` actually won't do.

@marek1840 marek1840 requested a review from tgodzik Sep 25, 2019
Copy link
Collaborator

tgodzik left a comment

Couple of comments, but mostly LGTM

private def assertCodeLenses(
filename: String,
expected: String
): Future[Unit] =
for {
_ <- server.didOpen(filename)
_ <- server.didSave(filename)(x => x) // first compilation could have not yet persisted analysis

This comment has been minimized.

Copy link
@tgodzik

tgodzik Sep 25, 2019

Collaborator

Maybe we should report it as a bug in Bloop?

@marek1840

This comment has been minimized.

Copy link
Collaborator Author

marek1840 commented Sep 26, 2019

The tests fail because bloop is unable to launch the debuggee:

ERROR Error looking up function 'Java_java_lang_ProcessImpl_forkAndExec':
/usr/lib/jvm/zulu-8-azure-amd64/jre/lib/amd64/libjava.so: undefined symbol: Java_java_lang_ProcessImpl_forkAndExec

which are likely due to those lines in nuprocess

Copy link
Member

olafurpg left a comment

This is super exciting! I haven't reviewed the most interesting bits yet, only a few minor nitpicking comments for now. I'll try to dig in closer in the coming week :)

@marek1840 marek1840 force-pushed the marek1840:add-dap-proxy branch from 6473e46 to a5f0594 Sep 30, 2019
@olafurpg

This comment has been minimized.

Copy link
Member

olafurpg commented Sep 30, 2019

The Maven CI is frequently failing with

INFO  [ERROR] Plugin ch.epfl.scala:maven-bloop_2.10:1.3.2+252-b4d82fc9 or one of its dependencies could not be resolved: Failure to find ch.epfl.scala:maven-bloop_2.10:jar:1.3.2+252-b4d82fc9 in https://maven-central.storage-download.googleapis.com/repos/central/data/ was cached in the local repository, resolution will not be reattempted until the update interval of google-maven-central has elapsed or updates are forced -> [Help 1]

@tgodzik Any idea what might be causing this error?

@tgodzik

This comment has been minimized.

Copy link
Collaborator

tgodzik commented Sep 30, 2019

The Maven CI is frequently failing with

INFO  [ERROR] Plugin ch.epfl.scala:maven-bloop_2.10:1.3.2+252-b4d82fc9 or one of its dependencies could not be resolved: Failure to find ch.epfl.scala:maven-bloop_2.10:jar:1.3.2+252-b4d82fc9 in https://maven-central.storage-download.googleapis.com/repos/central/data/ was cached in the local repository, resolution will not be reattempted until the update interval of google-maven-central has elapsed or updates are forced -> [Help 1]

@tgodzik Any idea what might be causing this error?

The bloopMaven plugin's snapshot is not published to maven central, so this will not work until we have a full release.

@marek1840 marek1840 force-pushed the marek1840:add-dap-proxy branch from 93c3aaa to 920724b Sep 30, 2019
@marek1840 marek1840 force-pushed the marek1840:add-dap-proxy branch from 3650d2a to 4eabe7d Oct 1, 2019
@marek1840 marek1840 force-pushed the marek1840:add-dap-proxy branch from 4eabe7d to c9bee5e Oct 1, 2019
@marek1840 marek1840 requested a review from olafurpg Oct 1, 2019
Copy link
Member

olafurpg left a comment

This is super exciting 😸 Being able to run and test from VS Code/Metals is a huge milestone!

I took the liberty to push a small change to fix the mapping of JVM classnames to SemanticDB symbols. This made the "test" and "run" code-lenses appear for me while testing locally using the ScalaTest test framewrok. I wasn't able to get it working for the utest test framework however since utest suites are written as Scala objects instead of Scala classes.

Screenshot 2019-10-03 at 10 17 22

The test class names from Bloop don't distinguish between two

[Trace - 10:13:43 AM] Received response 'buildTarget/scalaTestClasses - (42)' in 162ms
Result: {
  "items": [
    {
      "target": {
        "uri": "file:/Users/lgeirsson/dev/test-workspace/?id\u003dtest-workspace-test"
      },
      "classes": [
        "example.FooSuite",
        "example.UserTest"
      ]
    }
  ]
}

Would it be possible to somehow get this information from the build tool whether a test suite is an object or a class? cc/ @jvican

@marek1840

This comment has been minimized.

Copy link
Collaborator Author

marek1840 commented Oct 3, 2019

Would it be possible to somehow get this information from the build tool whether a test suite is an object or a class?

Bloop is creating the list of test frameworks using isModule flag, so it could be propagated.
Currently, we use:

trait ScalaTestClassesItem {
  /** The build target that contains the test classes. */
  def target: BuildTargetIdentifier

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

we could use:

trait ScalaTestClassesItem {
  /** The build target that contains the test classes. */
  def target: BuildTargetIdentifier

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

or something like that?

@marek1840

This comment has been minimized.

Copy link
Collaborator Author

marek1840 commented Oct 3, 2019

Alternatively, we could put the code lenses on both classes and objects without distinguishing them. Even if as a temporary solution only.

@olafurpg

This comment has been minimized.

Copy link
Member

olafurpg commented Oct 3, 2019

Alternatively, we could put the code lenses on both classes and objects without distinguishing them. Even if as a temporary solution only.

This is an acceptable workaround to unblock this PR. We can follow up by opening an issue in either BSP or Bloop to discuss how to extract the object test classes

@marek1840 marek1840 changed the title Add support for running, testing and debugging via Debug Adapter Protocol Add support for running and testing via Debug Adapter Protocol Oct 4, 2019
@marek1840 marek1840 requested a review from olafurpg Oct 4, 2019
@marek1840 marek1840 merged commit 570c506 into scalameta:master Oct 4, 2019
2 checks passed
2 checks passed
build
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jvican

This comment has been minimized.

Copy link
Collaborator

jvican commented Oct 4, 2019

@marek1840 Does restarting a debug session send a compile request to Bloop?

@marek1840

This comment has been minimized.

Copy link
Collaborator Author

marek1840 commented Oct 4, 2019

@marek1840 Does restarting a debug session send a compile request to Bloop?

Metals start completely new debug session when receiving a restart request.This, in turn, compiles projects.

@jvican

This comment has been minimized.

Copy link
Collaborator

jvican commented Oct 4, 2019

Sounds good, excited to try this out 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.