fwcd / kotlin-debug-adapter

Kotlin/JVM debugging for any editor/IDE using the Debug Adapter Protocol
MIT License
110 stars 19 forks source link

Include mavens test-classes folder to resolved project classpaths #68

Closed Mgenuit closed 1 year ago

Mgenuit commented 1 year ago

Allow maven test debugging

While working on a plugin setting up your debug-adapter for neovim I ran into some issues debugging tests.

Turns out while the ProjectClassesResolver does resolve a test directory for Gradle builds the test-classes folder used by maven seems to be left out.

Refrained from doing anything fancy to smooth things over. I was thinking that it might be nice to add something along the lines of includeTestResourses to the launch config next. As resolving the test folder does take a little bit of time you might want to save when debugging the main process. Don't you think?

Let me know what you think.

themkat commented 1 year ago

Sorry for taking some time to look at this PR! 🙁

Took a quick look, and your reasoning is logical. I tried running both your PR and the main-branch, and I can attach to Maven test runs just fine in both. I use mvn test -Dmaven.surefire.debug to create a debug process on port 5005, then attach to it with Emacs dap-mode. (it's a plain Spring Boot app I'm testing with). Could you write how you run your tests? Then I can maybe take a look at it and see if there are something I'm missing 🙂

Mgenuit commented 1 year ago

Yea sure, I'll see how i can dress the PR up a bit more (tests and better description) to better represent what it is I want to get done.

Until I get that done, to give a little bit of context. I'm working on a neovim plugin with configuration for debugging.

From your comment I understand that you launch the debug adapter in attach mode and attach it to an existing debug session you started as a separate process, correct?

My approach so far is a little different. I launch the adapter in launch mode and give it the Console Launcher as entry point. It scans the class-path for testcases and runs them. (you can see my config here)

In the current state this is where the issue arises because the test-classes aren't added to the class path.

I would like to avoid having to manage two processes in the plugin, but if you would do things differently I'm open to suggestions :) 0 I'll get back to expanding this MR a bit more to clarify the use-case in code/tests better.

themkat commented 1 year ago

Yea sure, I'll see how i can dress the PR up a bit more (tests and better description) to better represent what it is I want to get done.

Until I get that done, to give a little bit of context. I'm working on a neovim plugin with configuration for debugging.

From your comment I understand that you launch the debug adapter in attach mode and attach it to an existing debug session you started as a separate process, correct?

Yup, that is the way I have done it so far.

My approach so far is a little different. I launch the adapter in launch mode and give it the Console Launcher as entry point. It scans the class-path for testcases and runs them. (you can see my config here)

In the current state this is where the issue arises because the test-classes aren't added to the class path.

I would like to avoid having to manage two processes in the plugin, but if you would do things differently I'm open to suggestions :) 0 I'll get back to expanding this MR a bit more to clarify the use-case in code/tests better.

Ah! I have never tried it that way before. Maybe we could be better at documenting these kinds of ways of running in the README somewhere? Maybe a separate markdown file? I can do that, just wanted to have the suggestion in writing so I don't forget 😆

Got it working, and confirmed that your fix is needed. Your way was way more convenient once I set up a debug template in Emacs 🙂 Think I will use this from now on, thanks!

Btw, one quick question. this means we would have to add a relevant console-launcher dependencies to Maven projects to be able to debug this way? I added this for a quick test:

<dependency>
  <groupId>org.junit.platform</groupId>
  <artifactId>junit-platform-console-standalone</artifactId>
  <version>1.9.2</version>
  <scope>test</scope>
 </dependency>
Mgenuit commented 1 year ago

Thank you so much. Glad to hear you like the setup. Took me quite some time to figure out how to do it and why the debug adapter was not playing nicely :smiling_face_with_tear:.

As per your question, yes it needs to be present on the class-path. Although I'm 99.99% sure i got it for free when I was testing from spring-boot-starter-test.

I rebased and pushed. Not going to lie I was kind of sad when I read your message since I was 80% towards extending the testcases to show what I was attempting (and had to wrestle gradle quite a bit to get there) :smiling_face_with_tear:.

If we can merge this MR ill spend some more time getting them to 100% and I can make a separate MR for those.

That way you get to decide whether you like the implementation of those tests separately. I feel this is a better way since I'm kind of overloading the (few) tests that are there a bit, something I usually don't like to do. Its just that I'm not familiar enough with the project to think of a better way to do it :unamused:

themkat commented 1 year ago

Thank you so much. Glad to hear you like the setup. Took me quite some time to figure out how to do it and why the debug adapter was not playing nicely 🥲.

As per your question, yes it needs to be present on the class-path. Although I'm 99.99% sure i got it for free when I was testing from spring-boot-starter-test.

I rebased and pushed. Not going to lie I was kind of sad when I read your message since I was 80% towards extending the testcases to show what I was attempting (and had to wrestle gradle quite a bit to get there) 🥲.

If we can merge this MR ill spend some more time getting them to 100% and I can make a separate MR for those.

That way you get to decide whether you like the implementation of those tests separately. I feel this is a better way since I'm kind of overloading the (few) tests that are there a bit, something I usually don't like to do. Its just that I'm not familiar enough with the project to think of a better way to do it 😒

Then I say we merge it, as having this in place would be awesome. (I will also try to write some additional docs this week for various ways to configure etc. Might help future plugin devs for other editors). I would be very happy to read the tests you have written, so I'm looking forward to your next PR/MR 🙂