rcasia / neotest-java

Neotest adapter for Java.
MIT License
44 stars 24 forks source link

Tests should get executed using gradle/maven directly #138

Closed steffsommer closed 2 months ago

steffsommer commented 2 months ago

Hi,

first of all thank you very much for providing this plugin. It truly contributes to closing the gap to properly working with Java in NeoVim.


Proposed change

The plugin should favor execution of tests through gradle and maven directly and only use the JUnit jar as a fallback or even remove direct JUnit invocation for the sake of simplicity.

Reasons

1. Both Gradle and Maven can hook into the build and test execution process

So if a projects' build settings perform test modifications through build tools, this plugin won't work.

2. The plugin misses out on build optimizations + build functionality

3. Simplicity Since builds tools already handle efficient compilation and invocation of JUnit, Neotest-Java can leverage their simple CLI interfaces in the fashion <tool> test <test_names> --output <output>. I haven't read enough of the code to understand #100, but I think this issue should not even arise when executing tests through build tools.

Proposed methods

So in essence it would be highly beneficial to use the respective test commands

I actually think that it would do this plugin no harm to not support direct JUnit invocation at all:

Code reuse potential

I have done a very small contribution to someone elses attempt to provide a neotest-java adapter in the past. The approach to directly invoke the build tools was used there, so there is likely reusable code in the repo: https://github.com/andy-bell101/neotest-java/tree/main.

awelless commented 2 months ago

It seems this was the behavior before https://github.com/rcasia/neotest-java/issues/68. Shall we keep both and provide a config option to specify the preferred method?

steffsommer commented 2 months ago

If there is a preferred system, I would advocate for gradle->maven->Junit. Usually there will only be one build tool active at a time, but there might be a small intersection when migrating from one to another.

It would be helpful if @rcasia could give insights into when direct invocation performs better than build tools. I suspect the sweet spot might be single project monoliths. Gradle performing a bit worse in that scenario makes sense, because additionally to running the tests, it has to check if tests have to be run at all due to file changes. For Monorepo scenarios I think build tools should outperform direct invocation always starting from a project count of probably just 2, because then the file checks usually come with fruitful savings.

rcasia commented 2 months ago

The reason I removed the use of mvn test was there were many other extra steps that where done besides compiling and testing in real world projects.

I found that using the Junit5 executor would make 3 main improvements:

This brought a different headache: we still need to compile all the classes. mvn compile still does not only compile, in my real projects it actually does a lot of work everytime is was executed and there was no way neotest-java could handle a codebase like https://github.com/google/guava.

While exploring in my free time different solutions for #100 I have realized there is the option to use both mvn compiler:compile and gradle compileJava to JUST compile.

In summary:

steffsommer commented 2 months ago

Thank your for providing your insights.

Running JUST tests

Could you please elaborate what is run additionally to the tests using the test command of the builds tools?

Receiving just one possible format on the test reports

This is interesting. How do the formats differ? I had assumed that if JUnit was used under the hood, the output would always be the JUnit XML files

I would never go back to use mvn test and gradle test as they don't just test. That's why IDEs as IntelliJ are not using it for their test runners

Actually IntelliJ is using gradle test for executing unit tests and attaches a debugger to it. This is the default behavior I get using IntelliJ 2024.1. I haven't tested the behavior with maven though.

rcasia commented 2 months ago

Could you please elaborate what is run additionally to the tests using the test command of the builds tools?

Any plugin execution bound to the compile goal, which is slow.

Also digging more I found that mvn compiler:compile (which is also used by mvn compile) has its incremental build broken. See https://issues.apache.org/jira/browse/MCOMPILER-209.

This is interesting. How do the formats differ? I had assumed that if JUnit was used under the hood, the output would always be the JUnit XML files

See https://github.com/gradle/gradle/issues/23324. I had to read and merge information from both html and xml because of this issue. Now using the Junit5 Standalone Jar the problem was gone.The reason for the discrepancy between Maven and Gradle reports is unclear, but the reports differ slightly.

Actually IntelliJ is using gradle test for executing unit tests and attaches a debugger to it. This is the default behavior I get using IntelliJ 2024.1. I haven't tested the behavior with maven though.

I don’t use Gradle regularly in my projects, but there is a noticeable difference in timing between the IntelliJ test runner and the Maven test runner in the terminal. Additionally, IntelliJ allows you to view the long Java command at the beginning of the test logs.

steffsommer commented 2 months ago

For me it is the other way around, I use gradle way more than maven, so I know the big advantages that it has:

I don't say this this to bash maven, but gradle has simply evolved as the better tool. The XML/HTML merge issue is a nuisance, but I think bypassing the gradle build is not justified, because it brings worse disadvantages.


I want to suggest two potential approaches to go forward:

  1. Keep the current functionality and make it clear in the documentation/readme that this project focuses on performance, but does not work for composite builds, because it bypasses build tools and does full recompilation every time tests are executed. This is an important information for someone choosing this plugin. However, this will keep limit the scope of the plugin. The readme could also tell people to use neotest-gradle for bigger projects, although that is not in a working state as of now.
  2. I forked neotest-gradle and after some rudimentary fixing of treesitter node id handlings, i got it to work on a simple test suite at least (just haven't tested more). Since the original author does not want to maintain the project as it says in the readme, I would suggest copying the source code into this project. Then we could have 2 strategies:
    • invoke gradle(w) test for gradle builds
    • use the existing direct invocation of Junit for non-gradle builds

I would favor approach 2, because it means to provide a single neotest adapter for working with Java in Neovim. So no conflicts with other build tool specific adapters can arise. This should also fix #100.