fwcd / kotlin-debug-adapter

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

Add tests for inclusion of test-classes on the jvm class-path #72

Closed Mgenuit closed 9 months ago

Mgenuit commented 1 year ago

As promised in #68 I adapted the tests to validate inclusion of test-classes in the jvm class-path.

I also added a Maven test project to the test resources to have that system supported in tests as wel.

As a side note there were some things I would have done differently if it was my own project, but I tried to stay as close to the current setup as possible.

Let me know what you think. :smile:

themkat commented 1 year ago

I tried removing your previous fix, sequenceOf(resolveIfExists(projectRoot, "target", "test-classes")), and running the tests. They still pass. A little weird, because I was expecting a classnotfound exception to happen 🤔 (especially after it was needed to get ConsoleLauncher to work with breakpoints when I tested your last PR).

Mgenuit commented 1 year ago

Whof that is embarrassing :sweat_smile: especially since I had checked it (only not after the final refactor). Good that you double checked. I found out what happened. After the test project worked and the test proved the issue I did some refactoring to split it into 2 separate tests. (one for breakpoints and variables one for inclusion of test classes)

However I did not verify the "broken" case after the refactor and because it no longer hits the break-point it just launches the program forgets about it and reports the test successful. (not what I expected)

I can fix this the "old" way by making it one test but would like to spend some more time thinking of a nicer way to do it. (spend some time today but that only got me so far).

Mgenuit commented 1 year ago

Ok, let me start off this big update apologizing for:

  1. My opinion of the current test setup
  2. The scope creep of this commit
  3. Letting go of my "keep it close to current implementation" attitude
  4. The length of this and future comments

Now that that is done, here goes. So I was working on fixing the tests, but struggled with fixing it elegantly without reverting back to "one test to rule them all". I then came up with a way I would like to do it (using mockk) and figured why not try it and see if they like it.

That lead me to refactor some other parts of the testcases that worked, but did not make much sense anymore in my opinion.

Feel free to tell me no, in that case I will rollback and probably go back to one testcase testing it all. Otherwise all other comments are much appreciated

A little introduction on what changed.

Changing the IDebugClient

Instead of the TestFixture class serving as the debug client, I added a mock that serves as the client. This means that besides the shared logic (like logging all session info to the console, and unlocking the latch whenever the debugging session ends) each tests can define how the client responds to the events that happen.

The most used one right now is for example is in the continueOnBeak method. Whenever a breakpoint is hit the client just stores the threadId of the breakpoint that was hit and tells the server to continue that thread. But the variable tests needs to also ask the server for some information so it does not use that method it divines its own behavior when a breakpoint is hit.

Replacing Testfixture with GlobalWorkspaceTests

I noticed I would like to repeat most tests in both work-spaces (although I'm not 100% sure we need to) so I moved the duplicate tests tot the old test-fixture and renamed it because the name did not really feel accurate anymore.

Other niceties of Mockk

One thing that was not intentional but I did really appreciate from mockk is the hints it gives when a tests fails. Take for example the error when I remove the bugfix im trying to tests. image

It shows exactly what event it got that it did not expect to happen. And lets say I expect some event to happen once but it happens multiple times. It will give you a log of what happened: image

ExceptionBreakpoint tests

I think I found a bug in the ExceptionBreakpoint setup so I (potentially) fixed it and added some tests for that use-case as well (although not exhaustive).

There is more but ill add it in comments within the MR.

Mgenuit commented 1 year ago

Another thing is that I fear changes to the kotlin-languange-server broke our build again. I was able to work around it but not really sure how to fix it, and would not like to overload this MR even more :sweat_smile:.

Mgenuit commented 9 months ago

Looking back at it this MR was to big and unwieldy. I'm closing it to keep things clean.