Support running classes from specific Gradle source sets in Gradle actions

[Achal1607]

• When a mainClass is defined in a source set other than main in a Gradle project, the NetBeans Gradle action was unable to run or debug that class. This has been fixed by introducing a new project configuration parameter, runSourceSetName, similar to runSelectedClassName.

• Additionally, in the LSP launch configuration, specifying a mainClass from a non-main source set in launch.json previously caused it to run as if it were part of the test source set. This behavior has been corrected so that the specified mainClass is now executed properly instead of running tests.

Before fix:

JAVA_HOME="JDK_PATH"
cd PATH_TO_PROJECT/GradleTestingProject/app; ../gradlew --configure-on-demand -PrunClassName=org.yourcompany.yourproject.Main -s -x check run
Configuration on demand is an incubating feature.
> Task :app:compileJava UP-TO-DATE
> Task :app:processResources NO-SOURCE
> Task :app:classes UP-TO-DATE

> Task :app:run FAILED
Error: Could not find or load main class org.yourcompany.yourproject.Main
Caused by: java.lang.ClassNotFoundException: org.yourcompany.yourproject.Main

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':app:run'.
> Process 'command 'JDK_PATH/bin/java'' finished with non-zero exit value 1

After fix:

JAVA_HOME="JDK_PATH"
cd PATH_TO_PROJECT/GradleTestingProject/app; ../gradlew --configure-on-demand -PrunClassName=org.yourcompany.yourproject.Main -PrunSourceSetName=test -s -x check run
Configuration on demand is an incubating feature.
> Task :app:compileJava UP-TO-DATE
> Task :app:processResources NO-SOURCE
> Task :app:classes UP-TO-DATE
> Task :app:compileTestJava UP-TO-DATE
> Task :app:processTestResources NO-SOURCE
> Task :app:testClasses UP-TO-DATE

> Task :app:run
Hello World MAIN!!

BUILD SUCCESSFUL in 282ms
3 actionable tasks: 1 executed, 2 up-to-date

Related issue: oracle/javavscode#353

[lkishalmi]

Thank you!

[mbien]

the LSP org.netbeans.modules.lsp.client.debugger.DebuggerTest keeps failing here. The LSP test job is known to be unreliable, but this might be a real issue since its always the same test on this PR.

[Achal1607]

I tried running the same tests on the master branch in my local machine, but for me it fails even there as well with the same error. @mbien can you also verify it with the master branch if possible?

[mbien]

can you also verify it with the master branch if possible?

the LSP job does run when a PR is merged, it seems to work on master https://github.com/apache/netbeans/actions?query=branch%3Amaster, last success was 4h ago

[mbien]

java/java.lsp.server is the module with the unreliable tests (even the retry script is often not enough). ide/lsp.client does usually pass I believe (isn't wrapped in the retry script).

[Achal1607]

ide/lsp.client does usually pass I believe (isn't wrapped in the retry script).

I will try to check then more on the tests. Thanks much for verifying it.

[lahodaj]

I was looking at the test. The test is testing the DAP client, by running the java.lsp.server's Java DAP server and stepping using that.

The reason why the test fails is, I think, this: the test does not specify any testRun parameter, and hence the autodetect is used. The autodetect asks if a the given file is a main class, and uses the test actions if it is not. And, when run in this tests, isMainClass returns false, so the test actions are used, but there are no test actions available for the file. And hence the debugger backend will fail to start.

There are multiple aspects to this:

• maybe the test should explicitly say it does not want a test run:

diff --git a/ide/lsp.client/test/unit/src/org/netbeans/modules/lsp/client/debugger/DebuggerTest.java b/ide/lsp.client/test/unit/src/org/netbeans/modules/lsp/client/debugger/DebuggerTest.java
index 3718f1f2ed..8736be5755 100644
--- a/ide/lsp.client/test/unit/src/org/netbeans/modules/lsp/client/debugger/DebuggerTest.java
+++ b/ide/lsp.client/test/unit/src/org/netbeans/modules/lsp/client/debugger/DebuggerTest.java
@@ -125,7 +125,8 @@ public class DebuggerTest extends NbTestCase {
                         .addConfiguration(Map.of("type", "java+",
                                                  "request", "launch",
                                                  "file", FileUtil.toFile(testFile).getAbsolutePath(),
-                                                 "classPaths", List.of("any")))
+                                                 "classPaths", List.of("any"),
+                                                 "testRun", false))
                         .launch();
         waitFor(true, () -> DebuggerManager.getDebuggerManager().getSessions().length > 0);
         assertEquals(1, DebuggerManager.getDebuggerManager().getSessions().length);
@@ -198,7 +199,8 @@ public class DebuggerTest extends NbTestCase {
                         .addConfiguration(Map.of("type", "java+",
                                                  "request", "launch",
                                                  "file", FileUtil.toFile(testFile).getAbsolutePath(),
-                                                 "classPaths", List.of("any")))
+                                                 "classPaths", List.of("any"),
+                                                 "testRun", false))
                         .launch();
         waitFor(true, () -> DebuggerManager.getDebuggerManager().getSessions().length > 0);
         assertEquals(1, DebuggerManager.getDebuggerManager().getSessions().length);

• when autodetection runs, ignoring the UnitTestForSourceQuery feels a bit suspicious to me. Not sure how things work for Gradle, but if anything goes wrong with the isMainClass, the launcher will default to use the test actions instead of non-test actions, and that is, I think, likely to fail outside of test roots for "normal" projects. In other words, it errs on the side of use of the test actions, rather than the non-test actions. I still think the test actions should never be used outside of test roots (is there something specific in Gradle that would require running files in ordinary non-test roots using test actions?):

diff --git a/java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/debugging/launch/NbLaunchDelegate.java b/java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/debugging/launch/NbLaunchDelegate.java
index a488f1bca3..93cbc81339 100644
--- a/java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/debugging/launch/NbLaunchDelegate.java
+++ b/java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/debugging/launch/NbLaunchDelegate.java
@@ -60,6 +60,7 @@ import org.netbeans.api.extexecution.base.ExplicitProcessParameters;
 import org.netbeans.api.extexecution.ExecutionDescriptor;
 import org.netbeans.api.extexecution.ExecutionService;
 import org.netbeans.api.java.classpath.ClassPath;
+import org.netbeans.api.java.queries.UnitTestForSourceQuery;
 import org.netbeans.api.java.source.ClasspathInfo;
 import org.netbeans.api.java.source.SourceUtils;
 import org.netbeans.api.project.FileOwnerQuery;
@@ -578,11 +579,13 @@ public abstract class NbLaunchDelegate {
             if (sourceCP != null) {
                 FileObject root = sourceCP.findOwnerRoot(toRun);
                 if (root != null) {
-                    String relativePath = FileUtil.getRelativePath(root, toRun);
-                    if (relativePath != null && relativePath.toLowerCase(Locale.ENGLISH).endsWith(JAVA_FILE_EXT)) {
-                        String className = relativePath.substring(0, relativePath.length() - JAVA_FILE_EXT.length()).replace('/', '.');
-                        ClasspathInfo cpi = ClasspathInfo.create(toRun);
-                        mainSource = SourceUtils.isMainClass(className, cpi, true);
+                    if (UnitTestForSourceQuery.findSources(root).length > 0) {
+                        String relativePath = FileUtil.getRelativePath(root, toRun);
+                        if (relativePath != null && relativePath.toLowerCase(Locale.ENGLISH).endsWith(JAVA_FILE_EXT)) {
+                            String className = relativePath.substring(0, relativePath.length() - JAVA_FILE_EXT.length()).replace('/', '.');
+                            ClasspathInfo cpi = ClasspathInfo.create(toRun);
+                            mainSource = SourceUtils.isMainClass(className, cpi, true);
+                        }
                     }
                 }
             }

• the reason why isMainClass returns false in this test is, I think, that the indexing is not run on the source root. In theory, that would be solvable, but in practice, it probably should not matter much, and (if needed) using an explicit "testRun": false as above would be easier and more reliable.

[Achal1607]

@lahodaj, thank you so much for looking into it. I also believe that using "testRun": false is a more reliable approach. If you agree, should I go ahead and make the changes and update the commit?

[sdedic]

Does this change mean the behaviour of the service has changed in an incompatible way ?

[Achal1607]

Earlier, the logic checked if a file wasn’t a test file by trying to find the source of the root file of that test file. If no source was found, it would assume the file was a mainSource: true. However, this approach was a bit hacky for determining whether a file is a mainClass. As a result, in some cases, files inside the test sources that were actually mainClass files were incorrectly classified as test files, causing tests to run instead of executing the mainClass.

The new approach uses sourceCP directly to check whether a file is a mainClass.
However, since indexing wasn’t run in the tests scenario , it fails to identify mainClass files correctly in the failing test scenarios.
If there are any changes required in the changed behaviour, please let me know.

[lahodaj]

I suspect the current proposed state goes a little bit too far. Unless there's a proof the given class is a main class/has a main method, it will use the test actions to run the file. As I said, I don't know what is the state with Gradle, but for other project types, this is likely to fail, as they don't support test actions outside of the test roots.

I suspect a hybrid approach would work better: for stuff outside of test roots, use the non-test actions. For stuff in test roots, look at whether the given class has a main method.

[Achal1607]

Sorry, I didn’t understand your comment at first, @lahodaj. I’ve now made the changes according to your suggestions.

[lahodaj]

NbLaunchDelegate seems good to me now. Thanks!