Skip to content

improvement: Add location to failure to help mark it in VS Code #834

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package ch.epfl.scala.debugadapter

import java.nio.file.Path
import java.io.File
import ch.epfl.scala.debugadapter.internal.SourceLookUpProvider

import java.io.Closeable
import java.io.File
import java.nio.file.Path

trait Debuggee {
def name: String
Expand All @@ -19,4 +21,10 @@ trait Debuggee {
def classPath: Seq[Path] = classPathEntries.map(_.absolutePath)
def classEntries: Seq[ClassEntry] = classPathEntries ++ javaRuntime
def classPathString: String = classPath.mkString(File.pathSeparator)

@volatile private[debugadapter] var sourceLookUpProvider: Option[SourceLookUpProvider] = None
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gets a bit hacky unfortunately, so not sure if we want to go that way. Alternatively, we could just add the name and resolve it in metals when forwarding the event.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should not be a var but a normal parameter in the run method.


private[debugadapter] def setSourceLookUpProvider(provider: SourceLookUpProvider): Unit = {
sourceLookUpProvider = Some(provider)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ private class ClassEntryLookUp(
sourceUriToSourceFile: Map[SourceFileKey, SourceFile],
sourceUriToClassFiles: Map[SourceFileKey, Seq[ClassFile]],
classNameToSourceFile: Map[String, SourceFile],
sourceNameToSourceFile: Map[String, Seq[SourceFile]],
missingSourceFileClassFiles: Seq[ClassFile],
val orphanClassFiles: Seq[ClassFile],
logger: Logger
Expand All @@ -50,6 +51,10 @@ private class ClassEntryLookUp(
missingSourceFileClassFiles.map(_.fullyQualifiedName)
}

def uriFromFilename(filename: String): Option[URI] = {
sourceNameToSourceFile.get(filename).flatMap(_.headOption).map(_.uri)
}
Comment on lines +54 to +56
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need this new method. You should be able to use getSourceFileUri instead which takes a class name, instead of a file name.


def classesByScalaName: Map[String, Iterable[String]] =
fullyQualifiedNames.groupBy[String](NameTransformer.scalaClassName)

Expand Down Expand Up @@ -257,6 +262,7 @@ private object ClassEntryLookUp {
sourceUriToSourceFile,
sourceUriToClassFiles.toMap,
classNameToSourceFile.toMap,
sourceNameToSourceFile,
missingSourceFileClassFiles.toSeq,
orphanClassFiles.toSeq,
logger
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import scala.concurrent.Promise
import scala.util.Failure
import scala.util.Success
import scala.util.Try
import com.microsoft.java.debug.core.adapter.ISourceLookUpProvider

/**
* This debug adapter maintains the lifecycle of the debuggee in separation from JDI.
Expand Down Expand Up @@ -152,6 +153,10 @@ private[debugadapter] final class DebugSession private (
val debugConfig =
if (launchArgs.scalaStepFilters == null) config else config.copy(stepFilters = launchArgs.scalaStepFilters)
context.configure(tools, debugConfig)
context.getProvider(classOf[ISourceLookUpProvider]) match {
case provider: SourceLookUpProvider =>
debuggee.setSourceLookUpProvider(provider)
}
// launch request is implemented by spinning up a JVM
// and sending an attach request to the java DapServer
launchedRequests.add(requestId)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@ private[debugadapter] final class SourceLookUpProvider(
getSourceFileByClassname(fqcn).map(_.toString).orNull
}

def getSourceFileURI(fileName: String): Option[URI] = {
classPathEntries.iterator.map(_.uriFromFilename(fileName)).collectFirst { case Some(value) =>
value
}
}

override def getSourceContents(uri: String): String = {
val sourceUri = URI.create(uri)
sourceUriToClassPathEntry
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package ch.epfl.scala.debugadapter.testing

import sbt.testing.Status
import scala.jdk.CollectionConverters.*
import ch.epfl.scala.debugadapter.internal.SourceLookUpProvider

trait TestSuiteEventHandler {
def handle(testSuiteEvent: TestSuiteEvent): Unit
Expand All @@ -25,7 +26,8 @@ object TestSuiteEventHandler {
* Provide a summary of test suite execution based on passed TestSuiteEvent.Results parameter.
*/
def summarizeResults(
testSuiteResult: TestSuiteEvent.Results
testSuiteResult: TestSuiteEvent.Results,
sourceLookUpProviderOpt: Option[SourceLookUpProvider]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bloop will not be able to provide this, so this becomes again more difficult. We could make it all public, but then it becomes a bit unmanagable without a a large refactor

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure to understand why Bloop could not provide an instance of SourceLookUpProvider. Bloop starts a debug server, which always creates an instance of SourceLookUpProvider.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would need to reimplement it in Bloop, or make SourceLookUpProvider not private.

): TestSuiteSummary = {
val results = testSuiteResult.events.map { e =>
val name = TestUtils.printSelector(e.selector).getOrElse("")
Expand All @@ -35,13 +37,24 @@ object TestSuiteEventHandler {
case Status.Failure =>
val failedMsg =
TestUtils.printThrowable(e.throwable()).getOrElse("")
val location = if (e.throwable().isDefined()) {
val throwable = e.throwable().get.getStackTrace().headOption
for {
stackElement <- throwable
sourceLookUpProvider <- sourceLookUpProviderOpt
uri <- sourceLookUpProvider.getSourceFileURI(stackElement.getFileName())
} yield new SingleTestResult.Location(uri.toString(), stackElement.getLineNumber())

} else {
None
}
val formatted =
TestSuiteEventHandler.formatError(
name,
failedMsg,
indentSize = 0
)
SingleTestResult.Failed(name, e.duration, formatted)
SingleTestResult.Failed(name, e.duration, formatted, location)
case _ =>
SingleTestResult.Skipped(name)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,20 @@ object SingleTestResult {
def apply(testName: String): Skipped = new Skipped("skipped", testName)
}

final class Location(
val file: String,
val line: Int
)

final class Failed private (
val kind: String,
val testName: String,
val duration: Long,
val error: String
val error: String,
val location: Option[Location]
) extends SingleTestSummary
object Failed {
def apply(testName: String, duration: Long, error: String): Failed =
new Failed("failed", testName, duration, error)
def apply(testName: String, duration: Long, error: String, location: Option[Location] = None): Failed =
new Failed("failed", testName, duration, error, location)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -472,8 +472,8 @@ object DebugAdapterPlugin extends sbt.AutoPlugin {
val address = new DebugServer.Address()
jobService.runInBackground(scope, state) { (logger, _) =>
try {
val debuggee = debuggeeF(logger)
val resolver = resolverF(logger)
val debuggee = debuggeeF(logger)
// if there is a server for this target then close it
debugServers.get(target).foreach(_.close())
val server = DebugServer(debuggee, resolver, new LoggerAdapter(logger), address, config)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ private[debugadapter] final class TestSuitesDebuggee(
s"${getClass.getSimpleName}(${target.uri}, [${tests.mkString(", ")}])"

override def run(listener: DebuggeeListener): CancelableFuture[Unit] = {
val eventHandler = new SbtTestSuiteEventHandler(listener)
val eventHandler = new SbtTestSuiteEventHandler(listener, () => sourceLookUpProvider)

Comment on lines 70 to 72
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really hacky.

It would be clearer to pass the sourceLookupProvider as a parameter of run.

override def run(listener: DebuggeeListener, sourceLookup: SourceLookupProvider): CancelableFuture[Unit] = {
  val eventHandler = new SbtTestSuiteEventHandler(listener, sourceLookup)
  
    ???

run is called by the debugger itself which always has an instance of SourceLookupProvider.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I remember that run is called too early, before we construct the instance of SourceLokkupProvider. Maybe we should wait for the launch request before we start the debuggee process.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about the other PR then? Might be simpler for us to calculate it in Metals

@annotation.tailrec
def receiveLogs(is: ObjectInputStream, os: ObjectOutputStream): Unit = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,22 @@ package ch.epfl.scala.debugadapter.sbtplugin.internal
import ch.epfl.scala.debugadapter.DebuggeeListener
import ch.epfl.scala.debugadapter.testing.TestSuiteEventHandler
import ch.epfl.scala.debugadapter.testing.TestSuiteEvent
import ch.epfl.scala.debugadapter.internal.SourceLookUpProvider

/**
* Extracts information about tests execution and send it to the DebuggeeListener.
* Then DebugeeListener forwards it to the DAP client.
*/
class SbtTestSuiteEventHandler(listener: DebuggeeListener) extends TestSuiteEventHandler {
class SbtTestSuiteEventHandler(listener: DebuggeeListener, sourceLookUpProvider: () => Option[SourceLookUpProvider])
extends TestSuiteEventHandler {

def handle(event: TestSuiteEvent): Unit =
event match {
case TestSuiteEvent.Info(s) => listener.out(s)
case TestSuiteEvent.Warn(s) => listener.out(s)
case TestSuiteEvent.Error(s) => listener.err(s)
case results: TestSuiteEvent.Results =>
val testResults = TestSuiteEventHandler.summarizeResults(results)
val testResults = TestSuiteEventHandler.summarizeResults(results, sourceLookUpProvider())
listener.testResult(testResults)
case _ => ()
}
Expand Down
Loading