improvement: Add location to failure to help mark it in VS Code #834
Conversation
|
Thanks, that looks great!
If you use the Also I would appreciate if you could add a scripted test for it, similarly to this is one. |
| @@ -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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think it should not be a var but a normal parameter in the run method.
| @@ -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] | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We would need to reimplement it in Bloop, or make SourceLookUpProvider not private.
|
Or maybe I could just switch to attaching stack trace? We already have utilities in Metals that can change that to an exact location. This would make this PR much simpler and we can intercept the message in Metals. |
|
So the alternative is in #837 and it will require only minor changes in Metals |
| override def run(listener: DebuggeeListener): CancelableFuture[Unit] = { | ||
| val eventHandler = new SbtTestSuiteEventHandler(listener) | ||
| val eventHandler = new SbtTestSuiteEventHandler(listener, () => sourceLookUpProvider) | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
What do you think about the other PR then? Might be simpler for us to calculate it in Metals
| @@ -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 | |||
There was a problem hiding this comment.
I think it should not be a var but a normal parameter in the run method.
| @@ -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] | |||
There was a problem hiding this comment.
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.
| def uriFromFilename(filename: String): Option[URI] = { | ||
| sourceNameToSourceFile.get(filename).flatMap(_.headOption).map(_.uri) | ||
| } |
There was a problem hiding this comment.
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.
|
Closing this one, since it's much more involved than I can work on right now. Alternative should be reasonably quick |
I did a similar thing in scalameta/metals-vscode#1571 but I wonder if it would safer here.
@adpi2 any idea if we can get a full path easily instead of just the name? Otherwise, it seems the regex is an ok solution.