Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Tests should get executed using gradle/maven directly #138

Closed
steffsommer opened this issue Aug 30, 2024 · 6 comments
Closed

Tests should get executed using gradle/maven directly #138

steffsommer opened this issue Aug 30, 2024 · 6 comments

Comments

@steffsommer
Copy link

steffsommer commented Aug 30, 2024

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

  • to perform actions at certain points in the lifecycle
  • to customize test execution commands, etc

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

  • Due to its highly optimized nature and caching, gradle only does what is necessary. That means it might detect that there is no need to recompile certain test classes, which makes testing much, much faster
  • Gradle can automatically download + run apps/tests with the specified Java Version
  • I saw in some PR that direct JUnit execution was chosen, because it was the fastest. If that is true, then I suspect there should be a break-even point that comes fairly quickly. I currently work with a Monorepo with 30 Java/Gradle projects and compiling the entire repo for every test invocation would be super slow.

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

  • mvnw test
  • gradlew test

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

  • The setup functionality can be removed so that no external JUnit jar has to be downloaded
  • Java is basically never used withouth a Build Tool, so the build tool test functions are enough in 99% of cases. Build tools handle invocation of JUnit already under the hood
  • Overall it would simplify the code base

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
Copy link

awelless commented Sep 2, 2024

It seems this was the behavior before #68.
Shall we keep both and provide a config option to specify the preferred method?

@steffsommer
Copy link
Author

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
Copy link
Owner

rcasia commented Sep 3, 2024

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:

  • Running * JUST * tests
  • Receiving just one possible format on the test reports
  • Getting a easier integration with the debugger (nvim-dap)

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:

  • 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.
  • In order to avoid the overhead of actually having compile all the classes with the bugs that may happen, yes I would use maven and gradle

@steffsommer
Copy link
Author

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
Copy link
Owner

rcasia commented Sep 6, 2024

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 gradle/gradle#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
Copy link
Author

steffsommer commented Sep 8, 2024

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

  • massive performance difference even in single projects
  • properly working incremental compilation + caching (as you say, this has never worked using Maven at all). The need for incremental comilation is why big projects have started to adopt gradle some time ago, e.g. Spring Boot. This is also the reason why neotest-java will be too unperformant or just not work on big projects (even though it is faster on smaller ones)

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.

Repository owner locked and limited conversation to collaborators Sep 13, 2024
@rcasia rcasia converted this issue into discussion #149 Sep 13, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants